Fix various issues in RedirectToFileResourceHandler.

content/browser/loader/redirect_to_file_resource_handler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "net/base/mime_sniffer.h"
 #include "net/base/net_errors.h"
 #include "webkit/common/blob/shareable_file_reference.h"
 
 using webkit_blob::ShareableFileReference;
 
 namespace {
@@ skipping 20 matching lines @@
   scoped_refptr<net::IOBuffer> backing_;
 };
 
 }  // namespace
 
 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(
     int request_id,
     ResourceResponse* response,
     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);
 }
 
 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(
     int request_id,
     scoped_refptr<net::IOBuffer>* buf,
     int* buf_size,
     int min_size) {
   DCHECK_EQ(-1, min_size);
 
   if (buf_->capacity() < next_buffer_size_)
@@ skipping 31 matching lines @@
   }
 
   return WriteMore();
 }
 
 void RedirectToFileResourceHandler::OnResponseCompleted(
     int request_id,
     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;
     did_defer_ = true;
     *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_ = 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;
   if (result > 0) {
     next_handler_->OnDataDownloaded(request_id, result);
     write_cursor_ += result;
     failed = !WriteMore();
   } else {
     failed = true;
   }
 
   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_,
                                        completed_security_info_,
                                        &defer);
     if (!defer)
       ResumeIfDeferred();
   }
 }
 
 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
       // appending more data to the buffer.
       if (!buf_write_pending_) {
         if (BufIsFull())
           ResumeIfDeferred();
         buf_->set_offset(0);
         write_cursor_ = 0;

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

Random comment:  Is it just me, or does this seem weird/dangerous?  We're
relying on ResumeIfDeferred resulting in calling OnWillRead asynchronously.  I
could walk through the code to figure out the details of this dependency, but
it's certainly not immediately obvious to me why we can rely on this.

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

Hrm. Well, the assumption is true right now. I suppose anyone in the chain could
override Resume to make it re-entrant, though that does seem a little rude.

I think we can just reorder those lines. I'm going to do that separately because
this CL is too big.

       }
       return true;
     }
-    if (write_callback_pending_)
+    if (writer_->write_callback_pending())
       return true;
     DCHECK(write_cursor_ < buf_->offset());
 
     // Create a temporary buffer pointing to a subsection of the data buffer so
     // that it can be passed to Write.  This code makes some crazy scary
     // assumptions about object lifetimes, thread sharing, and that buf_ will
     // not realloc durring the write due to how the state machine in this class
     // works.
     // Note that buf_ is also shared with the code that writes data into the
     // cache, so modifying it can cause some pretty subtle race conditions:
     // https://code.google.com/p/chromium/issues/detail?id=152076
     // We're using DependentIOBuffer instead of DrainableIOBuffer to dodge some
     // of these issues, for the moment.
     // TODO(ncbray) make this code less crazy scary.
     // Also note that Write may increase the refcount of "wrapped" deep in the
     // bowels of its implementation, the use of scoped_refptr here is not
     // spurious.
     scoped_refptr<DependentIOBuffer> wrapped = new DependentIOBuffer(
         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);
     write_cursor_ += rv;
   }
 }
 
 bool RedirectToFileResourceHandler::BufIsFull() const {
   // This is a hack to workaround BufferedResourceHandler's inability to
   // deal with a ResourceHandler that returns a buffer size of less than
   // 2 * net::kMaxBytesToSniff from its OnWillRead method.
   // TODO(darin): Fix this retardation!
   return buf_->RemainingCapacity() <= (2 * net::kMaxBytesToSniff);
 }
 
 void RedirectToFileResourceHandler::ResumeIfDeferred() {
   if (did_defer_) {
     did_defer_ = false;
     controller()->Resume();
   }
 }
 
 }  // namespace content