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 8f962118a79976833f6a2ce31c8aadfc8d28b232..cdaf4fe9431f06272293c0e5272efc161c7c7510 100644
--- a/content/browser/loader/resource_loader_unittest.cc
+++ b/content/browser/loader/resource_loader_unittest.cc
@@ -4,20 +4,31 @@
 
 #include "content/browser/loader/resource_loader.h"
 
+#include "base/file_util.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 "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 {
@@ -68,7 +79,19 @@ class ClientCertStoreStub : public net::ClientCertStore {
 // initialize ResourceLoader.
 class ResourceHandlerStub : public ResourceHandler {
  public:
-  ResourceHandlerStub() : ResourceHandler(NULL) {}
+  ResourceHandlerStub(net::URLRequest* request)

[mmenke, 2013/12/04 20:55:48]

explicit

[davidben, 2013/12/04 22:44:04]

Done.

+      : ResourceHandler(request),
+        received_response_completed_(false),
+        total_bytes_downloaded_(0) {
+  }
+
+  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_; }
 
   virtual bool OnUploadProgress(int request_id,
                                 uint64 position,
@@ -85,11 +108,17 @@ class ResourceHandlerStub : public ResourceHandler {
 
   virtual bool OnResponseStarted(int request_id,
                                  ResourceResponse* response,
-                                 bool* defer) OVERRIDE { return true; }
+                                 bool* defer) OVERRIDE {
+    DCHECK(!response_);

[mmenke, 2013/12/04 20:55:48]

If we want these to trigger in release builds (Which is probably a good idea,
since trybots don't run debug tests), should make these CHECKs or EXPECTs.

[davidben, 2013/12/04 22:44:04]

Done.

+    response_ = response;
+    return true;
+  }
 
   virtual bool OnWillStart(int request_id,
                            const GURL& url,
                            bool* defer) OVERRIDE {
+    DCHECK(start_url_.is_empty());
+    start_url_ = url;
     return true;
   }
 
@@ -97,23 +126,39 @@ 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:
+  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
@@ -162,6 +207,80 @@ class ResourceContextStub : public MockResourceContext {
   scoped_ptr<net::ClientCertStore> dummy_cert_store_;
 };
 
+class MockTemporaryFileStreamFactory : public TemporaryFileStreamFactory {
+ public:
+  MockTemporaryFileStreamFactory()
+      : error_(base::PLATFORM_FILE_OK) {
+    CreateNextTemporary();
+  }
+
+  net::testing::MockFileStream* next_file_stream() const {
+    return next_file_stream_.get();
+  }
+  ShareableFileReference* next_deletable_file() const {
+    return next_deletable_file_.get();
+  }
+
+  void set_next_error(base::PlatformFileError error) {
+    DCHECK_EQ(base::PLATFORM_FILE_OK, error_);
+    error_ = error;
+  }
+
+  // MockTemporaryFileStreamFactory implementation:
+  virtual void CreateTemporary(int child_id,
+                               int request_id,
+                               const Callback& callback) OVERRIDE {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&MockTemporaryFileStreamFactory::DoCallback,
+                   base::Unretained(this), callback));
+  }
+
+ private:
+  void CreateNextTemporary() {
+    DCHECK(!next_file_stream_);
+
+    base::FilePath file_path;
+    ASSERT_TRUE(file_util::CreateTemporaryFile(&file_path));

[mmenke, 2013/12/04 20:55:48]

Believe this is now in the base namespace.

[davidben, 2013/12/04 22:44:04]

Done.

+    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);
+    ASSERT_NE(base::kInvalidPlatformFileValue, platform_file);
+    next_file_stream_.reset(
+        new net::testing::MockFileStream(
+            platform_file,
+            base::PLATFORM_FILE_WRITE | base::PLATFORM_FILE_ASYNC,
+            NULL, base::MessageLoopProxy::current()));
+    next_deletable_file_ =
+        ShareableFileReference::GetOrCreate(
+            file_path,
+            ShareableFileReference::DELETE_ON_FINAL_RELEASE,
+            BrowserThread::GetMessageLoopProxyForThread(
+                BrowserThread::FILE).get());
+  }
+
+  void DoCallback(const Callback& callback) {
+    if (error_ != base::PLATFORM_FILE_OK) {
+      callback.Run(error_, scoped_ptr<net::FileStream>(), NULL);
+      error_ = base::PLATFORM_FILE_OK;
+    } else {
+      DCHECK(next_file_stream_);
+      callback.Run(base::PLATFORM_FILE_OK,
+                   next_file_stream_.PassAs<net::FileStream>(),
+                   next_deletable_file_.get());
+      next_deletable_file_ = NULL;
+    }
+  }
+
+  base::PlatformFileError error_;
+  scoped_ptr<net::testing::MockFileStream> next_file_stream_;
+  scoped_refptr<ShareableFileReference> next_deletable_file_;
+};
+
 }  // namespace
 
 class ResourceLoaderTest : public testing::Test,
@@ -172,6 +291,31 @@ class ResourceLoaderTest : public testing::Test,
       resource_context_(&test_url_request_context_) {
   }
 
+  virtual void SetUp() OVERRIDE {
+    job_factory_.SetProtocolHandler(
+        "test", net::URLRequestTestJob::CreateProtocolHandler());
+    test_url_request_context_.set_job_factory(&job_factory_);
+  }
+
+  virtual void TearDown() 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(url, net::DEFAULT_PRIORITY, NULL,
+                            resource_context_.GetRequestContext()));
+    ResourceRequestInfo::AllocateForTesting(request.get(),
+                                            ResourceType::MAIN_FRAME,
+                                            &resource_context_,
+                                            kRenderProcessId,
+                                            kRenderViewId,
+                                            false);
+    return request.Pass();
+  }
+
   // ResourceLoaderDelegate:
   virtual ResourceDispatcherHostLoginDelegate* CreateLoginDelegate(
       ResourceLoader* loader,
@@ -200,6 +344,7 @@ class ResourceLoaderTest : public testing::Test,
 
   content::TestBrowserThreadBundle thread_bundle_;
 
+  net::URLRequestJobFactoryImpl job_factory_;
   net::TestURLRequestContext test_url_request_context_;
   ResourceContextStub resource_context_;
 };
@@ -209,20 +354,7 @@ class ResourceLoaderTest : public testing::Test,
 // certificates are correctly passed to the content browser client for
 // selection.
 TEST_F(ResourceLoaderTest, ClientCertStoreLookup) {
-  const int kRenderProcessId = 1;
-  const int kRenderViewId = 2;
-
-  scoped_ptr<net::URLRequest> request(
-      new net::URLRequest(GURL("dummy"),
-                          net::DEFAULT_PRIORITY,
-                          NULL,
-                          resource_context_.GetRequestContext()));
-  ResourceRequestInfo::AllocateForTesting(request.get(),
-                                          ResourceType::MAIN_FRAME,
-                                          &resource_context_,
-                                          kRenderProcessId,
-                                          kRenderViewId,
-                                          false);
+  scoped_ptr<net::URLRequest> request = CreateTestRequest(GURL("dummy"));
 
   // Set up the test client cert store.
   net::CertificateList dummy_certs(1, scoped_refptr<net::X509Certificate>(
@@ -239,7 +371,8 @@ TEST_F(ResourceLoaderTest, ClientCertStoreLookup) {
   resource_context_.SetClientCertStore(
       test_store.PassAs<net::ClientCertStore>());
 
-  scoped_ptr<ResourceHandler> resource_handler(new ResourceHandlerStub());
+  scoped_ptr<ResourceHandler> resource_handler(
+      new ResourceHandlerStub(request.get()));
   ResourceLoader loader(request.Pass(), resource_handler.Pass(), this);
 
   // Prepare a dummy certificate request.
@@ -274,26 +407,14 @@ TEST_F(ResourceLoaderTest, ClientCertStoreLookup) {
 // on a platform with a NULL client cert store still calls the content browser
 // client for selection.
 TEST_F(ResourceLoaderTest, ClientCertStoreNull) {
-  const int kRenderProcessId = 1;
-  const int kRenderViewId = 2;
-
-  scoped_ptr<net::URLRequest> request(
-      new net::URLRequest(GURL("dummy"),
-                          net::DEFAULT_PRIORITY,
-                          NULL,
-                          resource_context_.GetRequestContext()));
-  ResourceRequestInfo::AllocateForTesting(request.get(),
-                                          ResourceType::MAIN_FRAME,
-                                          &resource_context_,
-                                          kRenderProcessId,
-                                          kRenderViewId,
-                                          false);
+  scoped_ptr<net::URLRequest> request = CreateTestRequest(GURL("dummy"));
 
   // Ownership of the |request| is about to be turned over to ResourceLoader. We
   // need to keep a raw pointer copy to access this object later.
   net::URLRequest* raw_ptr_to_request = request.get();
 
-  scoped_ptr<ResourceHandler> resource_handler(new ResourceHandlerStub());
+  scoped_ptr<ResourceHandler> resource_handler(
+      new ResourceHandlerStub(request.get()));
   ResourceLoader loader(request.Pass(), resource_handler.Pass(), this);
 
   // Prepare a dummy certificate request.
@@ -320,4 +441,184 @@ TEST_F(ResourceLoaderTest, ClientCertStoreNull) {
   EXPECT_EQ(net::CertificateList(), test_client.passed_certs());
 }
 
+// Tests that a RedirectToFileResourceHandler works and forwards everything
+// downstream.
+TEST_F(ResourceLoaderTest, RedirectToFile) {
+  MockTemporaryFileStreamFactory file_stream_factory;
+  base::FilePath temp_path = file_stream_factory.next_deletable_file()->path();
+
+  // Setup 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.reset(
+      new RedirectToFileResourceHandler(resource_handler.Pass(), request.get(),
+                                        &file_stream_factory));
+  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) {
+  MockTemporaryFileStreamFactory file_stream_factory;
+  file_stream_factory.set_next_error(base::PLATFORM_FILE_ERROR_FAILED);
+
+  // Setup 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.reset(
+      new RedirectToFileResourceHandler(resource_handler.Pass(), request.get(),
+                                        &file_stream_factory));
+  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.
+  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) {
+  MockTemporaryFileStreamFactory file_stream_factory;
+  base::FilePath temp_path = file_stream_factory.next_deletable_file()->path();
+  net::testing::MockFileStream* mock_file_stream =
+      file_stream_factory.next_file_stream();
+  mock_file_stream->set_forced_error(net::ERR_FAILED);
+
+  // Setup 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.reset(
+      new RedirectToFileResourceHandler(resource_handler.Pass(), request.get(),
+                                        &file_stream_factory));
+  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());
+}
+
+// Tests that RedirectToFileResourceHandler handles asynchronous write errors.
+TEST_F(ResourceLoaderTest, RedirectToFileWriteErrorAsync) {
+  MockTemporaryFileStreamFactory file_stream_factory;
+  base::FilePath temp_path = file_stream_factory.next_deletable_file()->path();
+  net::testing::MockFileStream* mock_file_stream =
+      file_stream_factory.next_file_stream();
+  mock_file_stream->set_forced_error_async(net::ERR_FAILED);
+
+  // Setup 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.reset(
+      new RedirectToFileResourceHandler(resource_handler.Pass(), request.get(),
+                                        &file_stream_factory));
+  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());
+}
+
+// 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.
+  MockTemporaryFileStreamFactory file_stream_factory;
+  base::FilePath temp_path = file_stream_factory.next_deletable_file()->path();
+  net::testing::MockFileStream* mock_file_stream =
+      file_stream_factory.next_file_stream();
+  mock_file_stream->set_forced_error_async(net::ERR_FAILED);
+  mock_file_stream->ThrottleCallbacks();
+
+  // Setup 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.reset(
+      new RedirectToFileResourceHandler(resource_handler.Pass(), request.get(),
+                                        &file_stream_factory));
+  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());
+}
+
 }  // namespace content