Reland "Fix various issues in RedirectToFileResourceHandler."

It got reverted due to ASan issues. Fix child_id registration in
ResourceDispatcherHostTests so that they happen on ForwardingFilter creation,
rather than ad-hoc on a per-request basis.

Original description:

> - Handle errors in creating temporary files.
> - Cancel on write failure instead of resuming. This used to work, but got
>   lost in some refactoring in r142108.
> - Fix the OnResponseCompleted resume logic to account for partial writes.
> - Don't lose write errors which occur after OnResponseCompleted is received.
> - WeakPtrFactory goes after other members.
> - OnWillStart was never forwarded to downstream handlers.
> - Make sure the file is closed before deleting it.
>
> Also add a lot of machinery so we can better unit test this stack.

Original Review URL: https://codereview.chromium.org/82273002

BUG=316634,347663
TEST=ResourceDispatcherHostTest.DownloadToFile
     ResourceDispatcherHostTest.RegisterDownloadedTempFile
     ResourceLoaderRedirectToFileTest.*
     TemporaryFileStreamTest.Basic

Review URL: https://codereview.chromium.org/199453002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@257147 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 346 insertions, 9 deletions

diff --git a/content/browser/loader/resource_loader_unittest.cc b/content/browser/loader/resource_loader_unittest.cc
index 363c9ff..c4183d0 100644
--- a/content/browser/loader/resource_loader_unittest.cc
+++ b/content/browser/loader/resource_loader_unittest.cc
@@ -4,21 +4,33 @@
 
 #include "content/browser/loader/resource_loader.h"
 
+#include "base/file_util.h"
+#include "base/files/file.h"
+#include "base/message_loop/message_loop_proxy.h"
+#include "base/platform_file.h"
 #include "base/run_loop.h"
 #include "content/browser/browser_thread_impl.h"
+#include "content/browser/loader/redirect_to_file_resource_handler.h"
 #include "content/browser/loader/resource_loader_delegate.h"
 #include "content/public/browser/resource_request_info.h"
+#include "content/public/common/resource_response.h"
 #include "content/public/test/mock_resource_context.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/test/test_content_browser_client.h"
 #include "ipc/ipc_message.h"
+#include "net/base/mock_file_stream.h"
 #include "net/base/request_priority.h"
 #include "net/cert/x509_certificate.h"
 #include "net/ssl/client_cert_store.h"
 #include "net/ssl/ssl_cert_request_info.h"
 #include "net/url_request/url_request.h"
+#include "net/url_request/url_request_job_factory_impl.h"
+#include "net/url_request/url_request_test_job.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "webkit/common/blob/shareable_file_reference.h"
+
+using webkit_blob::ShareableFileReference;
 
 namespace content {
 namespace {
@@ -69,16 +81,34 @@ class ClientCertStoreStub : public net::ClientCertStore {
 // initialize ResourceLoader.
 class ResourceHandlerStub : public ResourceHandler {
  public:
-  ResourceHandlerStub()
-      : ResourceHandler(NULL), defer_request_on_will_start_(false) {}
+  explicit ResourceHandlerStub(net::URLRequest* request)
+      : ResourceHandler(request),
+        defer_request_on_will_start_(false),
+        received_response_completed_(false),
+        total_bytes_downloaded_(0) {
+  }
 
   void set_defer_request_on_will_start(bool defer_request_on_will_start) {
     defer_request_on_will_start_ = defer_request_on_will_start;
   }
 
+  const GURL& start_url() const { return start_url_; }
+  ResourceResponse* response() const { return response_.get(); }
+  bool received_response_completed() const {
+    return received_response_completed_;
+  }
+  const net::URLRequestStatus& status() const { return status_; }
+  int total_bytes_downloaded() const { return total_bytes_downloaded_; }
+
+  void Resume() {
+    controller()->Resume();
+  }
+
+  // ResourceHandler implementation:
   virtual bool OnUploadProgress(int request_id,
                                 uint64 position,
                                 uint64 size) OVERRIDE {
+    NOTREACHED();
     return true;
   }
 
@@ -86,18 +116,23 @@ class ResourceHandlerStub : public ResourceHandler {
                                    const GURL& url,
                                    ResourceResponse* response,
                                    bool* defer) OVERRIDE {
+    NOTREACHED();
     return true;
   }
 
   virtual bool OnResponseStarted(int request_id,
                                  ResourceResponse* response,
                                  bool* defer) OVERRIDE {
+    EXPECT_FALSE(response_);
+    response_ = response;
     return true;
   }
 
   virtual bool OnWillStart(int request_id,
                            const GURL& url,
                            bool* defer) OVERRIDE {
+    EXPECT_TRUE(start_url_.is_empty());
+    start_url_ = url;
     *defer = defer_request_on_will_start_;
     return true;
   }
@@ -112,26 +147,40 @@ class ResourceHandlerStub : public ResourceHandler {
                           scoped_refptr<net::IOBuffer>* buf,
                           int* buf_size,
                           int min_size) OVERRIDE {
-    return true;
+    NOTREACHED();
+    return false;
   }
 
   virtual bool OnReadCompleted(int request_id,
                                int bytes_read,
                                bool* defer) OVERRIDE {
-    return true;
+    NOTREACHED();
+    return false;
   }
 
   virtual void OnResponseCompleted(int request_id,
                                    const net::URLRequestStatus& status,
                                    const std::string& security_info,
                                    bool* defer) OVERRIDE {
+    // TODO(davidben): This DCHECK currently fires everywhere. Fix the places in
+    // ResourceLoader where OnResponseCompleted is signaled twice.
+    // DCHECK(!received_response_completed_);
+    received_response_completed_ = true;
+    status_ = status;
   }
 
   virtual void OnDataDownloaded(int request_id,
-                                int bytes_downloaded) OVERRIDE {}
+                                int bytes_downloaded) OVERRIDE {
+    total_bytes_downloaded_ += bytes_downloaded;
+  }
 
  private:
   bool defer_request_on_will_start_;
+  GURL start_url_;
+  scoped_refptr<ResourceResponse> response_;
+  bool received_response_completed_;
+  net::URLRequestStatus status_;
+  int total_bytes_downloaded_;
 };
 
 // Test browser client that captures calls to SelectClientCertificates and
@@ -180,6 +229,16 @@ class ResourceContextStub : public MockResourceContext {
   scoped_ptr<net::ClientCertStore> dummy_cert_store_;
 };
 
+// Fails to create a temporary file with the given error.
+void CreateTemporaryError(
+    base::File::Error error,
+    const CreateTemporaryFileStreamCallback& callback) {
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(callback, error, base::Passed(scoped_ptr<net::FileStream>()),
+                 scoped_refptr<ShareableFileReference>()));
+}
+
 }  // namespace
 
 class ResourceLoaderTest : public testing::Test,
@@ -187,7 +246,26 @@ class ResourceLoaderTest : public testing::Test,
  protected:
   ResourceLoaderTest()
     : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
-      resource_context_(&test_url_request_context_) {
+      resource_context_(&test_url_request_context_),
+      raw_ptr_resource_handler_(NULL),
+      raw_ptr_to_request_(NULL) {
+    job_factory_.SetProtocolHandler(
+        "test", net::URLRequestTestJob::CreateProtocolHandler());
+    test_url_request_context_.set_job_factory(&job_factory_);
+  }
+
+  GURL test_url() const {
+    return net::URLRequestTestJob::test_url_1();
+  }
+
+  std::string test_data() const {
+    return net::URLRequestTestJob::test_data_1();
+  }
+
+  virtual scoped_ptr<ResourceHandler> WrapResourceHandler(
+      scoped_ptr<ResourceHandlerStub> leaf_handler,
+      net::URLRequest* request) {
+    return leaf_handler.PassAs<ResourceHandler>();
   }
 
   virtual void SetUp() OVERRIDE {
@@ -195,7 +273,7 @@ class ResourceLoaderTest : public testing::Test,
     const int kRenderViewId = 2;
 
     scoped_ptr<net::URLRequest> request(
-        new net::URLRequest(GURL("dummy"),
+        new net::URLRequest(test_url(),
                             net::DEFAULT_PRIORITY,
                             NULL,
                             resource_context_.GetRequestContext()));
@@ -207,10 +285,13 @@ class ResourceLoaderTest : public testing::Test,
                                             kRenderViewId,
                                             MSG_ROUTING_NONE,
                                             false);
-    scoped_ptr<ResourceHandlerStub> resource_handler(new ResourceHandlerStub());
+    scoped_ptr<ResourceHandlerStub> resource_handler(
+        new ResourceHandlerStub(request.get()));
     raw_ptr_resource_handler_ = resource_handler.get();
     loader_.reset(new ResourceLoader(
-        request.Pass(), resource_handler.PassAs<ResourceHandler>(), this));
+        request.Pass(),
+        WrapResourceHandler(resource_handler.Pass(), raw_ptr_to_request_),
+        this));
   }
 
   // ResourceLoaderDelegate:
@@ -231,6 +312,7 @@ class ResourceLoaderTest : public testing::Test,
 
   content::TestBrowserThreadBundle thread_bundle_;
 
+  net::URLRequestJobFactoryImpl job_factory_;
   net::TestURLRequestContext test_url_request_context_;
   ResourceContextStub resource_context_;
 
@@ -322,4 +404,259 @@ TEST_F(ResourceLoaderTest, ResumeCancelledRequest) {
   static_cast<ResourceController*>(loader_.get())->Resume();
 }
 
+class ResourceLoaderRedirectToFileTest : public ResourceLoaderTest {
+ public:
+  ResourceLoaderRedirectToFileTest()
+      : file_stream_(NULL),
+        redirect_to_file_resource_handler_(NULL) {
+  }
+
+  base::FilePath temp_path() const { return temp_path_; }
+  ShareableFileReference* deletable_file() const {
+    return deletable_file_.get();
+  }
+  net::testing::MockFileStream* file_stream() const { return file_stream_; }
+  RedirectToFileResourceHandler* redirect_to_file_resource_handler() const {
+    return redirect_to_file_resource_handler_;
+  }
+
+  void ReleaseLoader() {
+    file_stream_ = NULL;
+    deletable_file_ = NULL;
+    loader_.reset();
+  }
+
+  virtual scoped_ptr<ResourceHandler> WrapResourceHandler(
+      scoped_ptr<ResourceHandlerStub> leaf_handler,
+      net::URLRequest* request) OVERRIDE {
+    // Make a temporary file.
+    CHECK(base::CreateTemporaryFile(&temp_path_));
+    base::PlatformFile platform_file =
+        base::CreatePlatformFile(temp_path_,
+                                 base::PLATFORM_FILE_WRITE |
+                                 base::PLATFORM_FILE_TEMPORARY |
+                                 base::PLATFORM_FILE_CREATE_ALWAYS |
+                                 base::PLATFORM_FILE_ASYNC,
+                                 NULL, NULL);
+    CHECK_NE(base::kInvalidPlatformFileValue, platform_file);
+
+    // Create mock file streams and a ShareableFileReference.
+    scoped_ptr<net::testing::MockFileStream> file_stream(
+        new net::testing::MockFileStream(
+            platform_file,
+            base::PLATFORM_FILE_WRITE | base::PLATFORM_FILE_ASYNC,
+            NULL, base::MessageLoopProxy::current()));
+    file_stream_ = file_stream.get();
+    deletable_file_ = ShareableFileReference::GetOrCreate(
+        temp_path_,
+        ShareableFileReference::DELETE_ON_FINAL_RELEASE,
+        BrowserThread::GetMessageLoopProxyForThread(
+            BrowserThread::FILE).get());
+
+    // Inject them into the handler.
+    scoped_ptr<RedirectToFileResourceHandler> handler(
+        new RedirectToFileResourceHandler(
+            leaf_handler.PassAs<ResourceHandler>(), request));
+    redirect_to_file_resource_handler_ = handler.get();
+    handler->SetCreateTemporaryFileStreamFunctionForTesting(
+        base::Bind(&ResourceLoaderRedirectToFileTest::PostCallback,
+                   base::Unretained(this),
+                   base::Passed(file_stream.PassAs<net::FileStream>())));
+    return handler.PassAs<ResourceHandler>();
+  }
+
+ private:
+  void PostCallback(
+      scoped_ptr<net::FileStream> file_stream,
+      const CreateTemporaryFileStreamCallback& callback) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, base::File::FILE_OK,
+                   base::Passed(&file_stream), deletable_file_));
+  }
+
+  base::FilePath temp_path_;
+  scoped_refptr<ShareableFileReference> deletable_file_;
+  // These are owned by the ResourceLoader.
+  net::testing::MockFileStream* file_stream_;
+  RedirectToFileResourceHandler* redirect_to_file_resource_handler_;
+};
+
+// Tests that a RedirectToFileResourceHandler works and forwards everything
+// downstream.
+TEST_F(ResourceLoaderRedirectToFileTest, Basic) {
+  // Run it to completion.
+  loader_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  // Check that the handler forwarded all information to the downstream handler.
+  EXPECT_EQ(temp_path(),
+            raw_ptr_resource_handler_->response()->head.download_file_path);
+  EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
+  EXPECT_TRUE(raw_ptr_resource_handler_->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::SUCCESS,
+            raw_ptr_resource_handler_->status().status());
+  EXPECT_EQ(test_data().size(), static_cast<size_t>(
+      raw_ptr_resource_handler_->total_bytes_downloaded()));
+
+  // Check that the data was written to the file.
+  std::string contents;
+  ASSERT_TRUE(base::ReadFileToString(temp_path(), &contents));
+  EXPECT_EQ(test_data(), contents);
+
+  // Release the loader and the saved reference to file. The file should be gone
+  // now.
+  ReleaseLoader();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(base::PathExists(temp_path()));
+}
+
+// Tests that RedirectToFileResourceHandler handles errors in creating the
+// temporary file.
+TEST_F(ResourceLoaderRedirectToFileTest, CreateTemporaryError) {
+  // Swap out the create temporary function.
+  redirect_to_file_resource_handler()->
+      SetCreateTemporaryFileStreamFunctionForTesting(
+          base::Bind(&CreateTemporaryError, base::File::FILE_ERROR_FAILED));
+
+  // Run it to completion.
+  loader_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  // To downstream, the request was canceled.
+  EXPECT_TRUE(raw_ptr_resource_handler_->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::CANCELED,
+            raw_ptr_resource_handler_->status().status());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->total_bytes_downloaded());
+}
+
+// Tests that RedirectToFileResourceHandler handles synchronous write errors.
+TEST_F(ResourceLoaderRedirectToFileTest, WriteError) {
+  file_stream()->set_forced_error(net::ERR_FAILED);
+
+  // Run it to completion.
+  loader_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  // To downstream, the request was canceled sometime after it started, but
+  // before any data was written.
+  EXPECT_EQ(temp_path(),
+            raw_ptr_resource_handler_->response()->head.download_file_path);
+  EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
+  EXPECT_TRUE(raw_ptr_resource_handler_->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::CANCELED,
+            raw_ptr_resource_handler_->status().status());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->total_bytes_downloaded());
+
+  // Release the loader. The file should be gone now.
+  ReleaseLoader();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(base::PathExists(temp_path()));
+}
+
+// Tests that RedirectToFileResourceHandler handles asynchronous write errors.
+TEST_F(ResourceLoaderRedirectToFileTest, WriteErrorAsync) {
+  file_stream()->set_forced_error_async(net::ERR_FAILED);
+
+  // Run it to completion.
+  loader_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  // To downstream, the request was canceled sometime after it started, but
+  // before any data was written.
+  EXPECT_EQ(temp_path(),
+            raw_ptr_resource_handler_->response()->head.download_file_path);
+  EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
+  EXPECT_TRUE(raw_ptr_resource_handler_->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::CANCELED,
+            raw_ptr_resource_handler_->status().status());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->total_bytes_downloaded());
+
+  // Release the loader. The file should be gone now.
+  ReleaseLoader();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(base::PathExists(temp_path()));
+}
+
+// Tests that RedirectToFileHandler defers completion if there are outstanding
+// writes and accounts for errors which occur in that time.
+TEST_F(ResourceLoaderRedirectToFileTest, DeferCompletion) {
+  // Program the MockFileStream to error asynchronously, but throttle the
+  // callback.
+  file_stream()->set_forced_error_async(net::ERR_FAILED);
+  file_stream()->ThrottleCallbacks();
+
+  // Run it as far as it will go.
+  loader_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  // At this point, the request should have completed.
+  EXPECT_EQ(net::URLRequestStatus::SUCCESS,
+            raw_ptr_to_request_->status().status());
+
+  // However, the resource loader stack is stuck somewhere after receiving the
+  // response.
+  EXPECT_EQ(temp_path(),
+            raw_ptr_resource_handler_->response()->head.download_file_path);
+  EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
+  EXPECT_FALSE(raw_ptr_resource_handler_->received_response_completed());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->total_bytes_downloaded());
+
+  // Now, release the floodgates.
+  file_stream()->ReleaseCallbacks();
+  base::RunLoop().RunUntilIdle();
+
+  // Although the URLRequest was successful, the leaf handler sees a failure
+  // because the write never completed.
+  EXPECT_TRUE(raw_ptr_resource_handler_->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::CANCELED,
+            raw_ptr_resource_handler_->status().status());
+
+  // Release the loader. The file should be gone now.
+  ReleaseLoader();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(base::PathExists(temp_path()));
+}
+
+// Tests that a RedirectToFileResourceHandler behaves properly when the
+// downstream handler defers OnWillStart.
+TEST_F(ResourceLoaderRedirectToFileTest, DownstreamDeferStart) {
+  // Defer OnWillStart.
+  raw_ptr_resource_handler_->set_defer_request_on_will_start(true);
+
+  // Run as far as we'll go.
+  loader_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  // The request should have stopped at OnWillStart.
+  EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
+  EXPECT_FALSE(raw_ptr_resource_handler_->response());
+  EXPECT_FALSE(raw_ptr_resource_handler_->received_response_completed());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->total_bytes_downloaded());
+
+  // Now resume the request. Now we complete.
+  raw_ptr_resource_handler_->Resume();
+  base::RunLoop().RunUntilIdle();
+
+  // Check that the handler forwarded all information to the downstream handler.
+  EXPECT_EQ(temp_path(),
+            raw_ptr_resource_handler_->response()->head.download_file_path);
+  EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
+  EXPECT_TRUE(raw_ptr_resource_handler_->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::SUCCESS,
+            raw_ptr_resource_handler_->status().status());
+  EXPECT_EQ(test_data().size(), static_cast<size_t>(
+      raw_ptr_resource_handler_->total_bytes_downloaded()));
+
+  // Check that the data was written to the file.
+  std::string contents;
+  ASSERT_TRUE(base::ReadFileToString(temp_path(), &contents));
+  EXPECT_EQ(test_data(), contents);
+
+  // Release the loader. The file should be gone now.
+  ReleaseLoader();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(base::PathExists(temp_path()));
+}
+
 }  // namespace content