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/browser/loader/temporary_file_stream.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.

[mmenke, 2014/03/03 21:10:11]

May be worth mentioning this lives on the same thread as the
RedirectToFileResourceHandler

[davidben, 2014/03/03 23:20:18]

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) {
+    DCHECK(!deletable_file_->path().empty());
+  }
+
+  bool write_callback_pending() const { return write_callback_pending_; }

[mmenke, 2014/03/03 21:10:11]

Optional:  is_writing maybe be clearer.  If you prefer this (Which is more
accurate), feel free to keep it as-is.

[davidben, 2014/03/03 23:20:18]

Done.

+  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)
+    net::URLRequest* request)
     : LayeredResourceHandler(request, next_handler.Pass()),
-      weak_factory_(this),
-      host_(host),
       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;

[mmenke, 2014/03/03 21:10:11]

Optional:  Do we get anything out of NULLing this out?  Close() shouldn't call
into |this|...Suppose you could argue it's safer to NULL it out, just wondering
because we're in the destructor, and it's a raw pointer.

[davidben, 2014/03/03 23:20:18]

Not especially. Mostly just makes it clear that we're releasing our reference to
that object; after Close(), it could be deleted at any time. I think a previous
version did a scoped_ptr<>::release() and I was just mirroring that.

+  }
+}
+
+void RedirectToFileResourceHandler::
+    SetCreateTemporaryFileStreamFunctionForTesting(
+        const CreateTemporaryFileStreamFunction& create_temporary_file_stream) {
+  create_temporary_file_stream_ = create_temporary_file_stream;
 }
 
 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_);
+    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,
+  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;
+  if (create_temporary_file_stream_.is_null()) {
+    CreateTemporaryFileStream(
         base::Bind(&RedirectToFileResourceHandler::DidCreateTemporaryFile,
                    weak_factory_.GetWeakPtr()));
-    return true;
+  } else {
+    create_temporary_file_stream_.Run(
+        base::Bind(&RedirectToFileResourceHandler::DidCreateTemporaryFile,
+                   weak_factory_.GetWeakPtr()));
   }
-  return next_handler_->OnWillStart(request_id, url, defer);
+  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()) {
     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::File::Error /*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::File::Error error_code,
+    scoped_ptr<net::FileStream> file_stream,
+    ShareableFileReference* deletable_file) {
+  DCHECK(!writer_);
+  if (error_code != base::File::FILE_OK) {
+    controller()->CancelWithError(net::FileErrorToNetError(error_code));

[mmenke, 2014/03/03 21:10:11]

optional:  Should we set did_defer_ to false here?  It doesn't currently matter,
but given the spaghetti nature of the behavior of this class, just seems like
one more place that things could go wrong.

[davidben, 2014/03/03 23:20:18]

*shrug* I don't think it matters? I think we want cancel and deferral to be
orthogonal anyway, so having one not muck with the others' state seems
reasonable.

[mmenke, 2014/03/04 18:06:52]

Only issue here is that you can defer handling of OnResponseCompleted, which
cancellation sends...  Anyways, fine as-is.

+    return;
+  }
+
+  writer_ = new Writer(this, file_stream.Pass(), deletable_file);
+
+  // Resume the request.
+  DCHECK(did_defer_);
+  bool defer = false;
+  if (!next_handler_->OnWillStart(GetRequestID(), will_start_url_, &defer)) {
+    controller()->Cancel();
+  } else if (!defer) {
+    ResumeIfDeferred();
+  }
+  did_defer_ = false;

[mmenke, 2014/03/03 21:10:11]

This still makes me nervous...  Yes, if we call ResumeIfDeferred,
OnResponseStarted *shouldn't* be synchronously called, and there's probably even
at least one guarantee of it somewhere, possibly more...  But I'd like not to
depend on that.

[davidben, 2014/03/03 23:20:18]

"This" being the did_defer_ outside of the else? Moved to an else block.

Resumption has enough sharp edges and this CL's large enough that I'd rather
leave the serious refactoring to a follow-up. This was originally just to the
small local fixes before trying to fix the troubles with the overall framework.

[mmenke, 2014/03/04 18:06:52]

Yea, "this" being setting did_defer_ to false after calling ResumeIfDeferred.

I'm happy with your change.

+  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) {
-    ResumeIfDeferred();
-  } else if (completed_during_write_) {
+    DCHECK(!writer_->write_callback_pending());
+    // 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);
+    }
+    if (!completed_during_write_)
+      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();
+    did_defer_ = false;

[mmenke, 2014/03/03 21:10:11]

Maybe put this in an else?  We don't be deferring again, since no one will be
calling into us, but...

[davidben, 2014/03/03 23:20:18]

Done.

   }
 }
 
 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;
       }
       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