Fix various issues in RedirectToFileResourceHandler.

content/browser/loader/redirect_to_file_resource_handler.cc

Index: content/browser/loader/redirect_to_file_resource_handler.cc
diff --git a/content/browser/loader/redirect_to_file_resource_handler.cc b/content/browser/loader/redirect_to_file_resource_handler.cc
index 37372afbbd913d436e69e1ed86eed879cd81ae18..42cf6bc08b4f5f329a0ac1fcc96c6a1eceed0683 100644
--- a/content/browser/loader/redirect_to_file_resource_handler.cc
+++ b/content/browser/loader/redirect_to_file_resource_handler.cc
@@ -5,14 +5,11 @@
 #include "content/browser/loader/redirect_to_file_resource_handler.h"
 
 #include "base/bind.h"
-#include "base/files/file_util_proxy.h"
 #include "base/logging.h"
-#include "base/message_loop/message_loop_proxy.h"
 #include "base/platform_file.h"
 #include "base/threading/thread_restrictions.h"
-#include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/loader/resource_request_info_impl.h"
-#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/resource_controller.h"
 #include "content/public/common/resource_response.h"
 #include "net/base/file_stream.h"
 #include "net/base/io_buffer.h"
@@ -53,23 +50,103 @@ namespace content {
 static const int kInitialReadBufSize = 32768;
 static const int kMaxReadBufSize = 524288;
 
+// A separate object to manage the lifetime of the net::FileStream and the
+// ShareableFileReference. When the handler is destroyed, it asynchronously
+// closes net::FileStream after all pending writes complete. Only after the
+// stream is closed is the ShareableFileReference released, to ensure the
+// temporary is not deleted before it is closed.
+class RedirectToFileResourceHandler::Writer {
+ public:
+  Writer(RedirectToFileResourceHandler* handler,
+         scoped_ptr<net::FileStream> file_stream,
+         ShareableFileReference* deletable_file)
+      : handler_(handler),
+        file_stream_(file_stream.Pass()),
+        write_callback_pending_(false),
+        deletable_file_(deletable_file) {
+  }
+
+  bool write_callback_pending() const { return write_callback_pending_; }
+  const base::FilePath& path() const { return deletable_file_->path(); }
+
+  int Write(net::IOBuffer* buf, int buf_len) {
+    DCHECK(!write_callback_pending_);
+    DCHECK(handler_);
+    int result = file_stream_->Write(
+        buf, buf_len,
+        base::Bind(&Writer::DidWriteToFile, base::Unretained(this)));
+    if (result == net::ERR_IO_PENDING)
+      write_callback_pending_ = true;
+    return result;
+  }
+
+  void Close() {
+    handler_ = NULL;
+    if (!write_callback_pending_)
+      CloseAndDelete();
+  }
+
+ private:
+  // Only DidClose can delete this.
+  ~Writer() {
+  }
+
+  void DidWriteToFile(int result) {
+    DCHECK(write_callback_pending_);
+    write_callback_pending_ = false;
+    if (handler_) {
+      handler_->DidWriteToFile(result);
+    } else {
+      CloseAndDelete();
+    }
+  }
+
+  void CloseAndDelete() {
+    DCHECK(!write_callback_pending_);
+    int result = file_stream_->Close(base::Bind(&Writer::DidClose,
+                                                base::Unretained(this)));
+    if (result != net::ERR_IO_PENDING)
+      DidClose(result);
+  }
+
+  void DidClose(int result) {
+    delete this;
+  }
+
+  RedirectToFileResourceHandler* handler_;
+
+  scoped_ptr<net::FileStream> file_stream_;
+  bool write_callback_pending_;
+
+  // We create a ShareableFileReference that's deletable for the temp file
+  // created as a result of the download.
+  scoped_refptr<webkit_blob::ShareableFileReference> deletable_file_;
+
+  DISALLOW_COPY_AND_ASSIGN(Writer);
+};
+
 RedirectToFileResourceHandler::RedirectToFileResourceHandler(
     scoped_ptr<ResourceHandler> next_handler,
     net::URLRequest* request,
-    ResourceDispatcherHostImpl* host)
+    const RedirectToFileResourceHandler::CreateTemporaryCB& create_temporary_cb)
     : LayeredResourceHandler(request, next_handler.Pass()),
-      weak_factory_(this),
-      host_(host),
+      create_temporary_cb_(create_temporary_cb),
       buf_(new net::GrowableIOBuffer()),
       buf_write_pending_(false),
       write_cursor_(0),
-      write_callback_pending_(false),
+      writer_(NULL),
       next_buffer_size_(kInitialReadBufSize),
       did_defer_(false),
-      completed_during_write_(false) {
+      completed_during_write_(false),
+      weak_factory_(this) {
 }
 
 RedirectToFileResourceHandler::~RedirectToFileResourceHandler() {
+  // Orphan the writer to asynchronously close and release the temporary file.
+  if (writer_) {
+    writer_->Close();
+    writer_ = NULL;
+  }
 }
 
 bool RedirectToFileResourceHandler::OnResponseStarted(
@@ -78,8 +155,9 @@ bool RedirectToFileResourceHandler::OnResponseStarted(
     bool* defer) {
   if (response->head.error_code == net::OK ||
       response->head.error_code == net::ERR_IO_PENDING) {
-    DCHECK(deletable_file_.get() && !deletable_file_->path().empty());
-    response->head.download_file_path = deletable_file_->path();
+    DCHECK(writer_);
+    DCHECK(!writer_->path().empty());
+    response->head.download_file_path = writer_->path();
   }
   return next_handler_->OnResponseStarted(request_id, response, defer);
 }
@@ -87,19 +165,17 @@ bool RedirectToFileResourceHandler::OnResponseStarted(
 bool RedirectToFileResourceHandler::OnWillStart(int request_id,
                                                 const GURL& url,
                                                 bool* defer) {
-  if (!deletable_file_.get()) {
-    // Defer starting the request until we have created the temporary file.
-    // TODO(darin): This is sub-optimal.  We should not delay starting the
-    // network request like this.
-    did_defer_ = *defer = true;
-    base::FileUtilProxy::CreateTemporary(
-        BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE).get(),
-        base::PLATFORM_FILE_ASYNC,
-        base::Bind(&RedirectToFileResourceHandler::DidCreateTemporaryFile,
-                   weak_factory_.GetWeakPtr()));
-    return true;
-  }
-  return next_handler_->OnWillStart(request_id, url, defer);
+  DCHECK(!writer_);
+
+  // Defer starting the request until we have created the temporary file.
+  // TODO(darin): This is sub-optimal.  We should not delay starting the
+  // network request like this.
+  will_start_url_ = url;
+  did_defer_ = *defer = true;
+  create_temporary_cb_.Run(
+      base::Bind(&RedirectToFileResourceHandler::DidCreateTemporaryFile,
+                 weak_factory_.GetWeakPtr()));
+  return true;
 }
 
 bool RedirectToFileResourceHandler::OnWillRead(
@@ -151,7 +227,7 @@ void RedirectToFileResourceHandler::OnResponseCompleted(
     const net::URLRequestStatus& status,
     const std::string& security_info,
     bool* defer) {
-  if (write_callback_pending_) {
+  if (writer_ && writer_->write_callback_pending()) {

[mmenke, 2014/01/16 16:43:26]

How can writer be NULL here?  Cancellation while we're still trying to open the
file?  If that happens, will the temporary file be cleanup up properly?  Doesn't
look like it would.  Instead, one option would be to have it always create the
writer, and then check if the object still exists via the weak pointer.

[davidben, 2014/01/29 21:41:50]

Yeah, that's the NULL case. The temporary in this version will actually be
deleted fine because the callback will be nullified, no one will take a
reference to the ShareableFileReference, and the file will be deleted.

(Actually, this was a problem with the old code; if DidCreateTemporaryFile
failed to run because of the WeakPtr, the temporary file just got leaked.)

     completed_during_write_ = true;
     completed_status_ = status;
     completed_security_info_ = security_info;
@@ -163,25 +239,28 @@ void RedirectToFileResourceHandler::OnResponseCompleted(
 }
 
 void RedirectToFileResourceHandler::DidCreateTemporaryFile(
-    base::PlatformFileError /*error_code*/,
-    base::PassPlatformFile file_handle,
-    const base::FilePath& file_path) {
-  deletable_file_ = ShareableFileReference::GetOrCreate(
-      file_path,
-      ShareableFileReference::DELETE_ON_FINAL_RELEASE,
-      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE).get());
-  file_stream_.reset(
-      new net::FileStream(file_handle.ReleaseValue(),
-                          base::PLATFORM_FILE_WRITE | base::PLATFORM_FILE_ASYNC,
-                          NULL));
-  const ResourceRequestInfo* info = GetRequestInfo();
-  host_->RegisterDownloadedTempFile(
-      info->GetChildID(), info->GetRequestID(), deletable_file_.get());
-  ResumeIfDeferred();
+    base::PlatformFileError error_code,
+    scoped_ptr<net::FileStream> file_stream,
+    ShareableFileReference* deletable_file) {
+  DCHECK(!writer_);
+  if (error_code != base::PLATFORM_FILE_OK) {
+    controller()->CancelWithError(net::PlatformFileErrorToNetError(error_code));
+    return;
+  }
+
+  writer_ = new Writer(this, file_stream.Pass(), deletable_file);
+
+  // Resume the request.
+  bool defer = false;
+  if (!next_handler_->OnWillStart(GetRequestID(), will_start_url_, &defer)) {
+    controller()->Cancel();
+  } else if (!defer) {
+    ResumeIfDeferred();
+  }
+  will_start_url_ = GURL();
 }
 
 void RedirectToFileResourceHandler::DidWriteToFile(int result) {
-  write_callback_pending_ = false;
   int request_id = GetRequestID();
 
   bool failed = false;
@@ -194,8 +273,21 @@ void RedirectToFileResourceHandler::DidWriteToFile(int result) {
   }
 
   if (failed) {

[mmenke, 2014/01/16 16:43:26]

Maybe throw in a "DCHECK(!writer_->write_callback_pending())" just to make it
clearer what happens with the next handler in this case?

[davidben, 2014/01/29 21:41:50]

Done.

-    ResumeIfDeferred();
-  } else if (completed_during_write_) {
+    // TODO(davidben): Recover the error code from WriteMore or |result|, as
+    // appropriate.
+    if (completed_during_write_ && completed_status_.is_success()) {
+      // If the request successfully completed mid-write, but the write failed,
+      // convert the status to a failure for downstream.
+      completed_status_.set_status(net::URLRequestStatus::CANCELED);
+      completed_status_.set_error(net::ERR_FAILED);
+    }
+    controller()->CancelWithError(net::ERR_FAILED);
+  }
+
+  if (completed_during_write_ && !writer_->write_callback_pending()) {
+    // Resume shutdown now that all data has been written to disk. Note that
+    // this should run even in the |failed| case above, otherwise a failed write
+    // leaves the handler stuck.
     bool defer = false;
     next_handler_->OnResponseCompleted(request_id,
                                        completed_status_,
@@ -207,7 +299,7 @@ void RedirectToFileResourceHandler::DidWriteToFile(int result) {
 }
 
 bool RedirectToFileResourceHandler::WriteMore() {
-  DCHECK(file_stream_.get());
+  DCHECK(writer_);
   for (;;) {
     if (write_cursor_ == buf_->offset()) {
       // We've caught up to the network load, but it may be in the process of
@@ -220,7 +312,7 @@ bool RedirectToFileResourceHandler::WriteMore() {
       }
       return true;
     }
-    if (write_callback_pending_)
+    if (writer_->write_callback_pending())
       return true;
     DCHECK(write_cursor_ < buf_->offset());
 
@@ -242,15 +334,9 @@ bool RedirectToFileResourceHandler::WriteMore() {
         buf_.get(), buf_->StartOfBuffer() + write_cursor_);
     int write_len = buf_->offset() - write_cursor_;
 
-    int rv = file_stream_->Write(
-        wrapped.get(),
-        write_len,
-        base::Bind(&RedirectToFileResourceHandler::DidWriteToFile,
-                   base::Unretained(this)));
-    if (rv == net::ERR_IO_PENDING) {
-      write_callback_pending_ = true;
+    int rv = writer_->Write(wrapped.get(), write_len);
+    if (rv == net::ERR_IO_PENDING)
       return true;
-    }
     if (rv <= 0)
       return false;
     next_handler_->OnDataDownloaded(GetRequestID(), rv);