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.
+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)
+    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;
+  }
+}
+
+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_);
+    DCHECK(!writer_->path().empty());

[mmenke, 2014/02/14 21:52:49]

Random comment:  Fine to leave as-is, but it's kinda weird to have this DCHECK
here and not, say, where we actually create the writer [either also or instead].

[davidben, 2014/02/25 20:03:07]

Done.

+    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));
+    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();
+  }

[mmenke, 2014/02/14 21:52:49]

Erm...Shouldn't did_defer_ also be set to false if defer is true?  The request
is still deferred, but it's no longer our responsibility to resume it, since
they just call the controller directly.

I suppose this also technically goes for the call in DidWriteToFile, though that
one doesn't really matter.

Hmm...  Can this be reasonably unit tested?

[davidben, 2014/02/25 20:03:07]

Yeah, I don't think this ResumeIfDeferred() pattern actually works very well for
deferrals outside OnWillStart. Really the whole chain with calling
next_handler_->OnWillStart and the like is part of the resume process. I've just
reset did_defer_ for now since this CL's already kinda big. Deferral, especially
around OnResponseCompleted has lots of broken edge cases and we should probably
think about fixing things in LayeredResourceHandler. (Maybe provide
ResumeOnWillStart() functions and the like.)

+  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);
+    }
+    controller()->CancelWithError(net::ERR_FAILED);

[mmenke, 2014/02/14 21:52:49]

Hmm...  Should we really be cancelling with an error if completed_during_write_
is true?

[davidben, 2014/02/25 20:03:07]

I don't think it especially matters? I've gone and guarded it with condition,
but ResourceLoader will need to support canceling after the URLRequest is done
anyway because ResponseCompleted gets called asynchronously after the cancel.

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