Fix various issues in RedirectToFileResourceHandler.

content/browser/loader/resource_loader_unittest.cc

Index: content/browser/loader/resource_loader_unittest.cc
diff --git a/content/browser/loader/resource_loader_unittest.cc b/content/browser/loader/resource_loader_unittest.cc
index 363c9ff591d8aad5c4a8b4c9a6e41f379b0c424c..61b68cf9e8c5839ab272039762947f23a979915e 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,81 @@ 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>()));
+}
+
+// Glue code to mock up a RedirectToFileResourceHandler with a MockFileStream
+// and a pre-created temporary file.
+class RedirectToFileMock {
+ public:
+  RedirectToFileMock() {
+    base::FilePath file_path;
+    CHECK(base::CreateTemporaryFile(&file_path));
+    base::PlatformFile platform_file =
+        base::CreatePlatformFile(file_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);
+    file_stream_.reset(
+        new net::testing::MockFileStream(
+            platform_file,
+            base::PLATFORM_FILE_WRITE | base::PLATFORM_FILE_ASYNC,
+            NULL, base::MessageLoopProxy::current()));
+    deletable_file_ =
+        ShareableFileReference::GetOrCreate(
+            file_path,
+            ShareableFileReference::DELETE_ON_FINAL_RELEASE,
+            BrowserThread::GetMessageLoopProxyForThread(
+                BrowserThread::FILE).get());
+  }
+
+  // These methods are only callable before CreateResourceHandler is called.
+  net::testing::MockFileStream* file_stream() const {
+    return file_stream_.get();
+  }
+  ShareableFileReference* deletable_file() const {
+    return deletable_file_.get();
+  }
+
+  scoped_ptr<RedirectToFileResourceHandler> CreateResourceHandler(
+      scoped_ptr<ResourceHandler> next_handler,
+      net::URLRequest* request) {
+    scoped_ptr<RedirectToFileResourceHandler> handler(
+        new RedirectToFileResourceHandler(next_handler.Pass(), request));
+    handler->SetCreateTemporaryFileStreamFunctionForTesting(
+        base::Bind(&RedirectToFileMock::PostCallback,
+                   base::Unretained(this)));
+    return handler.Pass();
+  }
+
+ private:
+  void PostCallback(const CreateTemporaryFileStreamCallback& callback) {
+    CHECK(file_stream_);
+    CHECK(deletable_file_);
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, base::File::FILE_OK,
+                   base::Passed(file_stream_.PassAs<net::FileStream>()),
+                   deletable_file_));
+    deletable_file_ = NULL;
+  }
+
+  scoped_ptr<net::testing::MockFileStream> file_stream_;
+  scoped_refptr<ShareableFileReference> deletable_file_;
+
+  DISALLOW_COPY_AND_ASSIGN(RedirectToFileMock);
+};
+
 }  // namespace
 
 class ResourceLoaderTest : public testing::Test,
@@ -187,19 +311,21 @@ 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_);
   }
 
-  virtual void SetUp() OVERRIDE {
+  scoped_ptr<net::URLRequest> CreateTestRequest(const GURL& url) {
     const int kRenderProcessId = 1;
     const int kRenderViewId = 2;
 
     scoped_ptr<net::URLRequest> request(
-        new net::URLRequest(GURL("dummy"),
-                            net::DEFAULT_PRIORITY,
-                            NULL,
+        new net::URLRequest(url, net::DEFAULT_PRIORITY, NULL,
                             resource_context_.GetRequestContext()));
-    raw_ptr_to_request_ = request.get();
     ResourceRequestInfo::AllocateForTesting(request.get(),
                                             ResourceType::MAIN_FRAME,
                                             &resource_context_,
@@ -207,7 +333,14 @@ class ResourceLoaderTest : public testing::Test,
                                             kRenderViewId,
                                             MSG_ROUTING_NONE,
                                             false);
-    scoped_ptr<ResourceHandlerStub> resource_handler(new ResourceHandlerStub());
+    return request.Pass();
+  }
+
+  virtual void SetUp() OVERRIDE {
+    scoped_ptr<net::URLRequest> request = CreateTestRequest(GURL("dummy"));
+    raw_ptr_to_request_ = request.get();
+    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));
@@ -231,6 +364,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 +456,248 @@ TEST_F(ResourceLoaderTest, ResumeCancelledRequest) {
   static_cast<ResourceController*>(loader_.get())->Resume();
 }
 
+// Tests that a RedirectToFileResourceHandler works and forwards everything
+// downstream.
+TEST_F(ResourceLoaderTest, RedirectToFile) {

[darin (slow to review), 2014/03/11 05:15:58]

Hmm, do these new tests really belong in this file? They don't make use of the
|loader_| member variable. They seem more like tests of
RedirectToFileResourceHandler instead of ResourceLoader. Perhaps if you want to
share code with resource_loader_unittest.cc, we should instead do a bit of
refactoring to make that possible?

Would it make sense for any of these tests to be written as unit tests instead
that just operate on RedirectToFileResourceHandler? It seems like we could
separately make some assertions about the behavior of ResourceLoader in this
file, and then based on those assertions, directly invoke ResourceHandler
methods on a RedirectToFileResourceHandler, simulating what ResourceLoader would
do.

[davidben, 2014/03/11 19:50:04]

Yeah, that was what I alluded to in comment #38. This file grew a lot more
machinery recently. I've gone and finished that refactoring.
Hrm, maybe. There's a lot to simulate though; making ResourceResponses and such.
These tests did also expose some bugs in ResourceLoader (see the TODO(davidben)
in OnResponseCompleted earlier in this file), though really that's indicative of
our ResourceLoader test coverage being pretty poor in itself.

+  RedirectToFileMock redirect_to_file_mock;
+  base::FilePath temp_path = redirect_to_file_mock.deletable_file()->path();
+
+  // Set up the request.
+  scoped_ptr<net::URLRequest> request = CreateTestRequest(
+      net::URLRequestTestJob::test_url_1());
+  ResourceHandlerStub* leaf_handler = new ResourceHandlerStub(request.get());
+  scoped_ptr<ResourceHandler> resource_handler(leaf_handler);
+  resource_handler = redirect_to_file_mock.CreateResourceHandler(
+      resource_handler.Pass(), request.get());
+  scoped_ptr<ResourceLoader> loader(new ResourceLoader(
+      request.Pass(), resource_handler.Pass(), this));
+
+  // Run it to completion.
+  loader->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  // Check that the handler forwarded all information to the downstream handler.
+  EXPECT_EQ(temp_path, leaf_handler->response()->head.download_file_path);
+  EXPECT_EQ(net::URLRequestTestJob::test_url_1(), leaf_handler->start_url());
+  EXPECT_TRUE(leaf_handler->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::SUCCESS, leaf_handler->status().status());
+  EXPECT_EQ(net::URLRequestTestJob::test_data_1().size(),
+            static_cast<size_t>(leaf_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(net::URLRequestTestJob::test_data_1(), contents);
+
+  // Release the loader. The file should be gone now.
+  loader.reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(base::PathExists(temp_path));
+}
+
+// Tests that RedirectToFileResourceHandler handles errors in creating the
+// temporary file.
+TEST_F(ResourceLoaderTest, RedirectToFileCreateTemporaryError) {
+  // Set up the request.
+  scoped_ptr<net::URLRequest> request = CreateTestRequest(
+      net::URLRequestTestJob::test_url_1());
+  ResourceHandlerStub* leaf_handler = new ResourceHandlerStub(request.get());
+
+  scoped_ptr<RedirectToFileResourceHandler> resource_handler(
+      new RedirectToFileResourceHandler(
+          scoped_ptr<ResourceHandler>(leaf_handler), request.get()));
+  resource_handler->SetCreateTemporaryFileStreamFunctionForTesting(
+      base::Bind(&CreateTemporaryError, base::File::FILE_ERROR_FAILED));
+
+  scoped_ptr<ResourceLoader> loader(new ResourceLoader(
+      request.Pass(), resource_handler.PassAs<ResourceHandler>(), this));
+
+  // Run it to completion.
+  loader->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  // To downstream, the request was canceled.
+  EXPECT_TRUE(leaf_handler->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::CANCELED, leaf_handler->status().status());
+  EXPECT_EQ(0, leaf_handler->total_bytes_downloaded());
+}
+
+// Tests that RedirectToFileResourceHandler handles synchronous write errors.
+TEST_F(ResourceLoaderTest, RedirectToFileWriteError) {
+  RedirectToFileMock redirect_to_file_mock;
+  base::FilePath temp_path = redirect_to_file_mock.deletable_file()->path();
+  net::testing::MockFileStream* mock_file_stream =
+      redirect_to_file_mock.file_stream();
+  mock_file_stream->set_forced_error(net::ERR_FAILED);
+
+  // Set up the request.
+  scoped_ptr<net::URLRequest> request = CreateTestRequest(
+      net::URLRequestTestJob::test_url_1());
+  ResourceHandlerStub* leaf_handler = new ResourceHandlerStub(request.get());
+  scoped_ptr<ResourceHandler> resource_handler(leaf_handler);
+  resource_handler = redirect_to_file_mock.CreateResourceHandler(
+      resource_handler.Pass(), request.get());
+  scoped_ptr<ResourceLoader> loader(new ResourceLoader(
+      request.Pass(), resource_handler.Pass(), this));
+
+  // 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, leaf_handler->response()->head.download_file_path);
+  EXPECT_EQ(net::URLRequestTestJob::test_url_1(), leaf_handler->start_url());
+  EXPECT_TRUE(leaf_handler->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::CANCELED, leaf_handler->status().status());
+  EXPECT_EQ(0, leaf_handler->total_bytes_downloaded());
+
+  // Release the loader. The file should be gone now.
+  loader.reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(base::PathExists(temp_path));
+}
+
+// Tests that RedirectToFileResourceHandler handles asynchronous write errors.
+TEST_F(ResourceLoaderTest, RedirectToFileWriteErrorAsync) {
+  RedirectToFileMock redirect_to_file_mock;
+  base::FilePath temp_path = redirect_to_file_mock.deletable_file()->path();
+  net::testing::MockFileStream* mock_file_stream =
+      redirect_to_file_mock.file_stream();
+  mock_file_stream->set_forced_error_async(net::ERR_FAILED);
+
+  // Set up the request.
+  scoped_ptr<net::URLRequest> request = CreateTestRequest(
+      net::URLRequestTestJob::test_url_1());
+  ResourceHandlerStub* leaf_handler = new ResourceHandlerStub(request.get());
+  scoped_ptr<ResourceHandler> resource_handler(leaf_handler);
+  resource_handler = redirect_to_file_mock.CreateResourceHandler(
+      resource_handler.Pass(), request.get());
+  scoped_ptr<ResourceLoader> loader(new ResourceLoader(
+      request.Pass(), resource_handler.Pass(), this));
+
+  // 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, leaf_handler->response()->head.download_file_path);
+  EXPECT_EQ(net::URLRequestTestJob::test_url_1(), leaf_handler->start_url());
+  EXPECT_TRUE(leaf_handler->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::CANCELED, leaf_handler->status().status());
+  EXPECT_EQ(0, leaf_handler->total_bytes_downloaded());
+
+  // Release the loader. The file should be gone now.
+  loader.reset();
+  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(ResourceLoaderTest, RedirectToFileDeferCompletion) {
+  // Program the MockFileStream to error asynchronously, but throttle the
+  // callback.
+  RedirectToFileMock redirect_to_file_mock;
+  base::FilePath temp_path = redirect_to_file_mock.deletable_file()->path();
+  net::testing::MockFileStream* mock_file_stream =
+      redirect_to_file_mock.file_stream();
+  mock_file_stream->set_forced_error_async(net::ERR_FAILED);
+  mock_file_stream->ThrottleCallbacks();
+
+  // Set up the request.
+  scoped_ptr<net::URLRequest> request = CreateTestRequest(
+      net::URLRequestTestJob::test_url_1());
+  net::URLRequest* raw_ptr_to_request = request.get();
+  ResourceHandlerStub* leaf_handler = new ResourceHandlerStub(request.get());
+  scoped_ptr<ResourceHandler> resource_handler(leaf_handler);
+  resource_handler = redirect_to_file_mock.CreateResourceHandler(
+      resource_handler.Pass(), request.get());
+  scoped_ptr<ResourceLoader> loader(new ResourceLoader(
+      request.Pass(), resource_handler.Pass(), this));
+
+  // 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, leaf_handler->response()->head.download_file_path);
+  EXPECT_EQ(net::URLRequestTestJob::test_url_1(), leaf_handler->start_url());
+  EXPECT_FALSE(leaf_handler->received_response_completed());
+  EXPECT_EQ(0, leaf_handler->total_bytes_downloaded());
+
+  // Now, release the floodgates.
+  mock_file_stream->ReleaseCallbacks();
+  base::RunLoop().RunUntilIdle();
+
+  // Although the URLRequest was successful, the leaf handler sees a failure
+  // because the write never completed.
+  EXPECT_TRUE(leaf_handler->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::CANCELED, leaf_handler->status().status());
+
+  // Release the loader. The file should be gone now.
+  loader.reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(base::PathExists(temp_path));
+}
+
+// Tests that a RedirectToFileResourceHandler behaves properly when the
+// downstream handler defers OnWillStart.
+TEST_F(ResourceLoaderTest, RedirectToFileDownstreamDeferStart) {
+  RedirectToFileMock redirect_to_file_mock;
+  base::FilePath temp_path =
+      redirect_to_file_mock.deletable_file()->path();
+
+  // Set up the request.
+  scoped_ptr<net::URLRequest> request = CreateTestRequest(
+      net::URLRequestTestJob::test_url_1());
+  ResourceHandlerStub* leaf_handler = new ResourceHandlerStub(request.get());
+  scoped_ptr<ResourceHandler> resource_handler(leaf_handler);
+  resource_handler = redirect_to_file_mock.CreateResourceHandler(
+      resource_handler.Pass(), request.get());
+  scoped_ptr<ResourceLoader> loader(new ResourceLoader(
+      request.Pass(), resource_handler.Pass(), this));
+
+  // Defer OnWillStart.
+  leaf_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(net::URLRequestTestJob::test_url_1(), leaf_handler->start_url());
+  EXPECT_FALSE(leaf_handler->response());
+  EXPECT_FALSE(leaf_handler->received_response_completed());
+  EXPECT_EQ(0, leaf_handler->total_bytes_downloaded());
+
+  // Now resume the request. Now we complete.
+  leaf_handler->Resume();
+  base::RunLoop().RunUntilIdle();
+
+  // Check that the handler forwarded all information to the downstream handler.
+  EXPECT_EQ(temp_path, leaf_handler->response()->head.download_file_path);
+  EXPECT_EQ(net::URLRequestTestJob::test_url_1(), leaf_handler->start_url());
+  EXPECT_TRUE(leaf_handler->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::SUCCESS, leaf_handler->status().status());
+  EXPECT_EQ(net::URLRequestTestJob::test_data_1().size(),
+            static_cast<size_t>(leaf_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(net::URLRequestTestJob::test_data_1(), contents);
+
+  // Release the loader. The file should be gone now.
+  loader.reset();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(base::PathExists(temp_path));
+}
+
 }  // namespace content