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 06f9216e3751226117cfa74c8554238dab8730ac..482b2fe6219fc4c26a3055f8b031531dbc4e0e06 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,91 @@ 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. This ensures that the file is closed before it is
+// deleted and that |file_stream_->Close()| is not called until
+// |file_stream_->Write()| completes.

[mmenke, 2013/12/05 21:34:03]

Suggest modifying the second sentence to make it clear that the Writer does the
closing (And the writing, though that's kinda expected of the Writer), and the
ShareableFileReference does the deleting.

[davidben, 2013/12/05 22:43:20]

Done.

+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) {

[mmenke, 2013/12/05 21:34:03]

DCHECK(!write_callback_pending_)?
DCHECK(handler_)?

[davidben, 2013/12/05 22:43:20]

Done.

+    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 Orphan() {
+    handler_ = NULL;
+    if (!write_callback_pending_)
+      CloseAndDelete();
+  }
+
+ private:
+  void DidWriteToFile(int result) {

[mmenke, 2013/12/05 21:34:03]

DCHECK(write_callback_pending_)?

[davidben, 2013/12/05 22:43:20]

Done.

+    write_callback_pending_ = false;
+    if (handler_) {
+      handler_->DidWriteToFile(result);
+    } else {
+      CloseAndDelete();
+    }
+  }
+
+  void CloseAndDelete() {

[mmenke, 2013/12/05 21:34:03]

DCHECK(!write_callback_pending_)?

[davidben, 2013/12/05 22:43:20]

Done.

+    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)
+    RedirectToFileResourceHandler::Delegate* delegate)
     : LayeredResourceHandler(request, next_handler.Pass()),
-      weak_factory_(this),
-      host_(host),
+      delegate_(delegate),
       buf_(new net::GrowableIOBuffer()),
       buf_write_pending_(false),
       write_cursor_(0),
-      write_callback_pending_(false),
       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_.release()->Orphan();
 }
 
 bool RedirectToFileResourceHandler::OnResponseStarted(
@@ -78,8 +143,8 @@ 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_ && !writer_->path().empty());

[mmenke, 2013/12/05 21:34:03]

Suggest splitting this into two DCHECKs, while you're here.

[davidben, 2013/12/05 22:43:20]

Done.

+    response->head.download_file_path = writer_->path();
   }
   return next_handler_->OnResponseStarted(request_id, response, defer);
 }
@@ -87,19 +152,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;
+  const ResourceRequestInfo* info = GetRequestInfo();
+  delegate_->CreateTemporary(
+      info->GetChildID(), info->GetRequestID(), weak_factory_.GetWeakPtr());
+  return true;
 }
 
 bool RedirectToFileResourceHandler::OnWillRead(
@@ -151,36 +214,39 @@ void RedirectToFileResourceHandler::OnResponseCompleted(
     const net::URLRequestStatus& status,
     const std::string& security_info,
     bool* defer) {
-  if (write_callback_pending_) {
+  if (writer_ && writer_->write_callback_pending()) {
     completed_during_write_ = true;
     completed_status_ = status;
     completed_security_info_ = security_info;
-    *defer = true;
+    did_defer_ = *defer = true;
     return;
   }
   next_handler_->OnResponseCompleted(request_id, status, security_info, defer);
 }
 
 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_.reset(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;
@@ -193,8 +259,21 @@ void RedirectToFileResourceHandler::DidWriteToFile(int result) {
   }
 
   if (failed) {
-    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_,
@@ -206,7 +285,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
@@ -219,7 +298,7 @@ bool RedirectToFileResourceHandler::WriteMore() {
       }
       return true;
     }
-    if (write_callback_pending_)
+    if (writer_->write_callback_pending())
       return true;
     DCHECK(write_cursor_ < buf_->offset());
 
@@ -241,15 +320,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);