FileWriterDelegate should not call URLRequest::Start() after Cancel().

- This should also fix one of the flaky crashes in FileWriterDelegate::OnDataWritten after cancel
- This patch also removes Start/End quota update from FileWriterDelegate since now we do that in FileSystemOperation

BUG=122160,116639
TEST=file-writer-abort-continue.html

Review URL: https://chromiumcodereview.appspot.com/10008047

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@136513 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 96 insertions, 66 deletions

diff --git a/webkit/fileapi/file_system_operation.cc b/webkit/fileapi/file_system_operation.cc
index 663bace..da2a94d 100644
--- a/webkit/fileapi/file_system_operation.cc
+++ b/webkit/fileapi/file_system_operation.cc
@@ -290,13 +290,14 @@ void FileSystemOperation::Write(
   file_writer_delegate_.reset(new FileWriterDelegate(
           this, src_path_, offset));
   set_write_callback(callback);
-  blob_request_.reset(
+  scoped_ptr<net::URLRequest> blob_request(
       new net::URLRequest(blob_url, file_writer_delegate_.get()));
-  blob_request_->set_context(url_request_context);
+  blob_request->set_context(url_request_context);
 
   GetUsageAndQuotaThenRunTask(
       src_path_.origin(), src_path_.type(),
-      base::Bind(&FileSystemOperation::DoWrite, base::Unretained(this)),
+      base::Bind(&FileSystemOperation::DoWrite, weak_factory_.GetWeakPtr(),
+                 base::Passed(&blob_request)),
       base::Bind(callback, base::PLATFORM_FILE_ERROR_FAILED, 0, true));
 }
 
@@ -389,23 +390,23 @@ void FileSystemOperation::OpenFile(const GURL& path_url,
 void FileSystemOperation::Cancel(const StatusCallback& cancel_callback) {
   if (file_writer_delegate_.get()) {
     DCHECK_EQ(kOperationWrite, pending_operation_);
+
     // Writes are done without proxying through FileUtilProxy after the initial
     // opening of the PlatformFile.  All state changes are done on this thread,
-    // so we're guaranteed to be able to shut down atomically.  We do need to
-    // check that the file has been opened [which means the blob_request_ has
-    // been created], so we know how much we need to do.
-    if (blob_request_.get())
-      // This halts any calls to file_writer_delegate_ from blob_request_.
-      blob_request_->Cancel();
+    // so we're guaranteed to be able to shut down atomically.
+    const bool delete_now = file_writer_delegate_->Cancel();
 
     if (!write_callback_.is_null()) {
       // Notify the failure status to the ongoing operation's callback.
       write_callback_.Run(base::PLATFORM_FILE_ERROR_ABORT, 0, false);
-      // Do not delete this FileSystemOperation object yet. As a result of
-      // abort, this->DidWrite is called later and there we delete this object.
     }
     cancel_callback.Run(base::PLATFORM_FILE_OK);
     write_callback_.Reset();
+
+    if (delete_now) {
+      delete this;
+      return;
+    }
   } else {
     DCHECK_EQ(kOperationTruncate, pending_operation_);
     // We're cancelling a truncate operation, but we can't actually stop it
@@ -450,7 +451,8 @@ FileSystemOperation::FileSystemOperation(
       src_util_(NULL),
       dest_util_(NULL),
       peer_handle_(base::kNullProcessHandle),
-      pending_operation_(kOperationNone) {
+      pending_operation_(kOperationNone),
+      weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
 }
 
 void FileSystemOperation::GetUsageAndQuotaThenRunTask(
@@ -540,15 +542,21 @@ void FileSystemOperation::DoMove(const StatusCallback& callback) {
                  base::Owned(this), callback));
 }
 
-void FileSystemOperation::DoWrite() {
+void FileSystemOperation::DoWrite(scoped_ptr<net::URLRequest> blob_request) {
   int file_flags = base::PLATFORM_FILE_OPEN |
                    base::PLATFORM_FILE_WRITE |
                    base::PLATFORM_FILE_ASYNC;
 
+  // We may get deleted on the way so allocate a new operation context
+  // to keep it alive.
+  FileSystemOperationContext* write_context = new FileSystemOperationContext(
+      operation_context_);
   FileSystemFileUtilProxy::CreateOrOpen(
-      &operation_context_, src_util_, src_path_, file_flags,
+      write_context, src_util_, src_path_, file_flags,
       base::Bind(&FileSystemOperation::OnFileOpenedForWrite,
-                 base::Unretained(this)));
+                 weak_factory_.GetWeakPtr(),
+                 base::Passed(&blob_request),
+                 base::Owned(write_context)));
 }
 
 void FileSystemOperation::DoTruncate(const StatusCallback& callback,
@@ -664,6 +672,8 @@ void FileSystemOperation::DidOpenFile(
 }
 
 void FileSystemOperation::OnFileOpenedForWrite(
+    scoped_ptr<net::URLRequest> blob_request,
+    FileSystemOperationContext* unused,
     base::PlatformFileError rv,
     base::PassPlatformFile file,
     bool created) {
@@ -673,7 +683,7 @@ void FileSystemOperation::OnFileOpenedForWrite(
     delete this;
     return;
   }
-  file_writer_delegate_->Start(file.ReleaseValue(), blob_request_.get());
+  file_writer_delegate_->Start(file.ReleaseValue(), blob_request.Pass());
 }
 
 base::PlatformFileError FileSystemOperation::SetUpFileSystemPath(
diff --git a/webkit/fileapi/file_system_operation.h b/webkit/fileapi/file_system_operation.h
index dde3158..c615f7b8 100644
--- a/webkit/fileapi/file_system_operation.h
+++ b/webkit/fileapi/file_system_operation.h
@@ -173,7 +173,7 @@ class FileSystemOperation : public FileSystemOperationInterface {
                          bool recursive);
   void DoCopy(const StatusCallback& callback);
   void DoMove(const StatusCallback& callback);
-  void DoWrite();
+  void DoWrite(scoped_ptr<net::URLRequest> blob_request);
   void DoTruncate(const StatusCallback& callback, int64 length);
   void DoOpenFile(const OpenFileCallback& callback, int file_flags);
 
@@ -218,7 +218,9 @@ class FileSystemOperation : public FileSystemOperationInterface {
                    bool created);
 
   // Helper for Write().
-  void OnFileOpenedForWrite(base::PlatformFileError rv,
+  void OnFileOpenedForWrite(scoped_ptr<net::URLRequest> blob_request,
+                            FileSystemOperationContext* context_unused,
+                            base::PlatformFileError rv,
                             base::PassPlatformFile file,
                             bool created);
 
@@ -249,7 +251,6 @@ class FileSystemOperation : public FileSystemOperationInterface {
   // These are all used only by Write().
   friend class FileWriterDelegate;
   scoped_ptr<FileWriterDelegate> file_writer_delegate_;
-  scoped_ptr<net::URLRequest> blob_request_;
 
   // write_callback is kept in this class for so that we can dispatch it when
   // the operation is cancelled. calcel_callback is kept for canceling a
@@ -268,6 +269,10 @@ class FileSystemOperation : public FileSystemOperationInterface {
   // A flag to make sure we call operation only once per instance.
   OperationType pending_operation_;
 
+  // FileSystemOperation instance is usually deleted upon completion but
+  // could be deleted while it has inflight callbacks when Cancel is called.
+  base::WeakPtrFactory<FileSystemOperation> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(FileSystemOperation);
 };
 
diff --git a/webkit/fileapi/file_system_operation_write_unittest.cc b/webkit/fileapi/file_system_operation_write_unittest.cc
index 2af53ec..6654e1a 100644
--- a/webkit/fileapi/file_system_operation_write_unittest.cc
+++ b/webkit/fileapi/file_system_operation_write_unittest.cc
@@ -382,6 +382,6 @@ TEST_F(FileSystemOperationWriteTest, TestImmediateCancelFailingWrite) {
   EXPECT_TRUE(complete());
 }
 
-// TODO(ericu,dmikurube): Add more tests for Cancel.
+// TODO(ericu,dmikurube,kinuko): Add more tests for cancel cases.
 
 }  // namespace fileapi
diff --git a/webkit/fileapi/file_writer_delegate.cc b/webkit/fileapi/file_writer_delegate.cc
index 1f8a086..31214ae 100644
--- a/webkit/fileapi/file_writer_delegate.cc
+++ b/webkit/fileapi/file_writer_delegate.cc
@@ -32,14 +32,10 @@ class InitializeTask : public base::RefCountedThreadSafe<InitializeTask> {
  public:
   InitializeTask(
       base::PlatformFile file,
-      const FileSystemPath& path,
-      FileSystemOperationContext* context,
       const InitializeTaskCallback& callback)
       : original_loop_(base::MessageLoopProxy::current()),
         error_code_(base::PLATFORM_FILE_OK),
         file_(file),
-        path_(path),
-        context_(*context),
         callback_(callback) {
     DCHECK_EQ(false, callback.is_null());
   }
@@ -60,13 +56,6 @@ class InitializeTask : public base::RefCountedThreadSafe<InitializeTask> {
   }
 
   void ProcessOnTargetThread() {
-    DCHECK(context_.file_system_context());
-    FileSystemQuotaUtil* quota_util = context_.file_system_context()->
-        GetQuotaUtil(path_.type());
-    if (quota_util) {
-      DCHECK(quota_util->proxy());
-      quota_util->proxy()->StartUpdateOrigin(path_.origin(), path_.type());
-    }
     if (!base::GetPlatformFileInfo(file_, &file_info_))
       error_code_ = base::PLATFORM_FILE_ERROR_FAILED;
     original_loop_->PostTask(
@@ -78,8 +67,6 @@ class InitializeTask : public base::RefCountedThreadSafe<InitializeTask> {
   base::PlatformFileError error_code_;
 
   base::PlatformFile file_;
-  FileSystemPath path_;
-  FileSystemOperationContext context_;
   InitializeTaskCallback callback_;
 
   base::PlatformFileInfo file_info_;
@@ -95,6 +82,7 @@ FileWriterDelegate::FileWriterDelegate(
       file_(base::kInvalidPlatformFileValue),
       path_(path),
       offset_(offset),
+      has_pending_write_(false),
       bytes_written_backlog_(0),
       bytes_written_(0),
       bytes_read_(0),
@@ -107,10 +95,11 @@ FileWriterDelegate::FileWriterDelegate(
 FileWriterDelegate::~FileWriterDelegate() {
 }
 
-void FileWriterDelegate::OnGetFileInfoAndCallStartUpdate(
+void FileWriterDelegate::OnGetFileInfoAndStartRequest(
+    scoped_ptr<net::URLRequest> request,
     base::PlatformFileError error,
     const base::PlatformFileInfo& file_info) {
-  if (error) {
+  if (error != base::PLATFORM_FILE_OK) {
     OnError(error);
     return;
   }
@@ -128,22 +117,34 @@ void FileWriterDelegate::OnGetFileInfoAndCallStartUpdate(
       base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_WRITE |
       base::PLATFORM_FILE_ASYNC,
       NULL));
+  DCHECK(!request_.get());
+  request_ = request.Pass();
   request_->Start();
 }
 
 void FileWriterDelegate::Start(base::PlatformFile file,
-                               net::URLRequest* request) {
+                               scoped_ptr<net::URLRequest> request) {
   file_ = file;
-  request_ = request;
 
   scoped_refptr<InitializeTask> relay = new InitializeTask(
-      file_, path_,
-      file_system_operation_context(),
-      base::Bind(&FileWriterDelegate::OnGetFileInfoAndCallStartUpdate,
-                 weak_factory_.GetWeakPtr()));
+      file_,
+      base::Bind(&FileWriterDelegate::OnGetFileInfoAndStartRequest,
+                 weak_factory_.GetWeakPtr(), base::Passed(&request)));
   relay->Start(file_system_operation_context()->file_task_runner(), FROM_HERE);
 }
 
+bool FileWriterDelegate::Cancel() {
+  if (request_.get()) {
+    // This halts any callbacks on this delegate.
+    request_->set_delegate(NULL);
+    request_->Cancel();
+  }
+
+  // Return true to finish immediately if we're not writing.
+  // Otherwise we'll do the final cleanup in the write callback.
+  return !has_pending_write_;
+}
+
 void FileWriterDelegate::OnReceivedRedirect(net::URLRequest* request,
                                             const GURL& new_url,
                                             bool* defer_redirect) {
@@ -172,7 +173,7 @@ void FileWriterDelegate::OnSSLCertificateError(net::URLRequest* request,
 }
 
 void FileWriterDelegate::OnResponseStarted(net::URLRequest* request) {
-  DCHECK_EQ(request_, request);
+  DCHECK_EQ(request_.get(), request);
   // file_stream_->Seek() blocks the IO thread.
   // See http://crbug.com/75548.
   base::ThreadRestrictions::ScopedAllowIO allow_io;
@@ -190,7 +191,7 @@ void FileWriterDelegate::OnResponseStarted(net::URLRequest* request) {
 
 void FileWriterDelegate::OnReadCompleted(net::URLRequest* request,
                                          int bytes_read) {
-  DCHECK_EQ(request_, request);
+  DCHECK_EQ(request_.get(), request);
   if (!request->status().is_success()) {
     OnError(base::PLATFORM_FILE_ERROR_FAILED);
     return;
@@ -241,6 +242,7 @@ void FileWriterDelegate::Write() {
   if (bytes_to_write > allowed_bytes_to_write_ - total_bytes_written_)
     bytes_to_write = allowed_bytes_to_write_ - total_bytes_written_;
 
+  has_pending_write_ = true;
   int write_response =
       file_stream_->Write(cursor_,
                           static_cast<int>(bytes_to_write),
@@ -256,7 +258,12 @@ void FileWriterDelegate::Write() {
 }
 
 void FileWriterDelegate::OnDataWritten(int write_response) {
+  has_pending_write_ = false;
   if (write_response > 0) {
+    if (request_->status().status() == net::URLRequestStatus::CANCELED) {
+      OnProgress(write_response, true);
+      return;
+    }
     OnProgress(write_response, false);
     cursor_->DidConsume(write_response);
     bytes_written_ += write_response;
@@ -271,11 +278,10 @@ void FileWriterDelegate::OnDataWritten(int write_response) {
 }
 
 void FileWriterDelegate::OnError(base::PlatformFileError error) {
-  request_->set_delegate(NULL);
-  request_->Cancel();
-
-  if (quota_util())
-    quota_util()->proxy()->EndUpdateOrigin(path_.origin(), path_.type());
+  if (request_.get()) {
+    request_->set_delegate(NULL);
+    request_->Cancel();
+  }
 
   file_system_operation_->DidWrite(error, 0, true);
 }
diff --git a/webkit/fileapi/file_writer_delegate.h b/webkit/fileapi/file_writer_delegate.h
index 4b37af0..3b58ae0 100644
--- a/webkit/fileapi/file_writer_delegate.h
+++ b/webkit/fileapi/file_writer_delegate.h
@@ -30,10 +30,15 @@ class FileWriterDelegate : public net::URLRequest::Delegate {
   virtual ~FileWriterDelegate();
 
   void Start(base::PlatformFile file,
-             net::URLRequest* request);
-  base::PlatformFile file() {
-    return file_;
-  }
+             scoped_ptr<net::URLRequest> request);
+
+  // Cancels the current write operation.  Returns true if it is ok to
+  // delete this instance immediately.  Otherwise this will call
+  // |write_operation|->DidWrite() with complete=true to let the operation
+  // perform the final cleanup.
+  bool Cancel();
+
+  base::PlatformFile file() const { return file_; }
 
   virtual void OnReceivedRedirect(net::URLRequest* request,
                                   const GURL& new_url,
@@ -51,7 +56,8 @@ class FileWriterDelegate : public net::URLRequest::Delegate {
                                int bytes_read) OVERRIDE;
 
  private:
-  void OnGetFileInfoAndCallStartUpdate(
+  void OnGetFileInfoAndStartRequest(
+      scoped_ptr<net::URLRequest> request,
       base::PlatformFileError error,
       const base::PlatformFileInfo& file_info);
   void Read();
@@ -69,6 +75,7 @@ class FileWriterDelegate : public net::URLRequest::Delegate {
   FileSystemPath path_;
   int64 size_;
   int64 offset_;
+  bool has_pending_write_;
   base::Time last_progress_event_time_;
   int bytes_written_backlog_;
   int bytes_written_;
@@ -78,7 +85,7 @@ class FileWriterDelegate : public net::URLRequest::Delegate {
   scoped_refptr<net::IOBufferWithSize> io_buffer_;
   scoped_refptr<net::DrainableIOBuffer> cursor_;
   scoped_ptr<net::FileStream> file_stream_;
-  net::URLRequest* request_;
+  scoped_ptr<net::URLRequest> request_;
   base::WeakPtrFactory<FileWriterDelegate> weak_factory_;
 };
 
diff --git a/webkit/fileapi/file_writer_delegate_unittest.cc b/webkit/fileapi/file_writer_delegate_unittest.cc
index 31c95db..574dd7d 100644
--- a/webkit/fileapi/file_writer_delegate_unittest.cc
+++ b/webkit/fileapi/file_writer_delegate_unittest.cc
@@ -123,13 +123,15 @@ class FileWriterDelegateTest : public PlatformTest {
 
   static net::URLRequest::ProtocolFactory Factory;
 
+  // This should be alive until the very end of this instance.
+  MessageLoop loop_;
+
   scoped_ptr<QuotaFileUtil> quota_file_util_;
   scoped_ptr<FileWriterDelegate> file_writer_delegate_;
   scoped_ptr<net::URLRequest> request_;
   scoped_ptr<Result> result_;
   FileSystemTestOriginHelper test_helper_;
 
-  MessageLoop loop_;
   ScopedTempDir dir_;
   FilePath file_path_;
   PlatformFile file_;
@@ -229,7 +231,7 @@ TEST_F(FileWriterDelegateTest, WriteSuccessWithoutQuotaLimit) {
   PrepareForWrite(kBlobURL, 0, QuotaFileUtil::kNoLimit);
 
   ASSERT_EQ(0, test_helper_.GetCachedOriginUsage());
-  file_writer_delegate_->Start(file_, request_.get());
+  file_writer_delegate_->Start(file_, request_.Pass());
   MessageLoop::current()->Run();
   ASSERT_EQ(kDataSize, test_helper_.GetCachedOriginUsage());
   EXPECT_EQ(ComputeCurrentOriginUsage(), test_helper_.GetCachedOriginUsage());
@@ -248,7 +250,7 @@ TEST_F(FileWriterDelegateTest, WriteSuccessWithJustQuota) {
   PrepareForWrite(kBlobURL, 0, kAllowedGrowth);
 
   ASSERT_EQ(0, test_helper_.GetCachedOriginUsage());
-  file_writer_delegate_->Start(file_, request_.get());
+  file_writer_delegate_->Start(file_, request_.Pass());
   MessageLoop::current()->Run();
   ASSERT_EQ(kAllowedGrowth, test_helper_.GetCachedOriginUsage());
   EXPECT_EQ(ComputeCurrentOriginUsage(), test_helper_.GetCachedOriginUsage());
@@ -267,7 +269,7 @@ TEST_F(FileWriterDelegateTest, WriteFailureByQuota) {
   PrepareForWrite(kBlobURL, 0, kAllowedGrowth);
 
   ASSERT_EQ(0, test_helper_.GetCachedOriginUsage());
-  file_writer_delegate_->Start(file_, request_.get());
+  file_writer_delegate_->Start(file_, request_.Pass());
   MessageLoop::current()->Run();
   ASSERT_EQ(kAllowedGrowth, test_helper_.GetCachedOriginUsage());
   EXPECT_EQ(ComputeCurrentOriginUsage(), test_helper_.GetCachedOriginUsage());
@@ -286,7 +288,7 @@ TEST_F(FileWriterDelegateTest, WriteZeroBytesSuccessfullyWithZeroQuota) {
   PrepareForWrite(kBlobURL, 0, kAllowedGrowth);
 
   ASSERT_EQ(0, test_helper_.GetCachedOriginUsage());
-  file_writer_delegate_->Start(file_, request_.get());
+  file_writer_delegate_->Start(file_, request_.Pass());
   MessageLoop::current()->Run();
   ASSERT_EQ(kAllowedGrowth, test_helper_.GetCachedOriginUsage());
   EXPECT_EQ(ComputeCurrentOriginUsage(), test_helper_.GetCachedOriginUsage());
@@ -330,8 +332,8 @@ TEST_F(FileWriterDelegateTest, WriteSuccessWithoutQuotaLimitConcurrent) {
   request2.reset(new net::URLRequest(kBlobURL2, file_writer_delegate2.get()));
 
   ASSERT_EQ(0, test_helper_.GetCachedOriginUsage());
-  file_writer_delegate_->Start(file_, request_.get());
-  file_writer_delegate2->Start(file2, request2.get());
+  file_writer_delegate_->Start(file_, request_.Pass());
+  file_writer_delegate2->Start(file2, request2.Pass());
   MessageLoop::current()->Run();
   if (!result_->complete() || !result2->complete())
     MessageLoop::current()->Run();
@@ -363,7 +365,7 @@ TEST_F(FileWriterDelegateTest, WritesWithQuotaAndOffset) {
   PrepareForWrite(kBlobURL, offset, allowed_growth);
 
   ASSERT_EQ(0, test_helper_.GetCachedOriginUsage());
-  file_writer_delegate_->Start(file_, request_.get());
+  file_writer_delegate_->Start(file_, request_.Pass());
   MessageLoop::current()->Run();
   ASSERT_EQ(kDataSize, test_helper_.GetCachedOriginUsage());
   EXPECT_EQ(ComputeCurrentOriginUsage(), test_helper_.GetCachedOriginUsage());
@@ -376,7 +378,7 @@ TEST_F(FileWriterDelegateTest, WritesWithQuotaAndOffset) {
   allowed_growth = 20;
   PrepareForWrite(kBlobURL, offset, allowed_growth);
 
-  file_writer_delegate_->Start(file_, request_.get());
+  file_writer_delegate_->Start(file_, request_.Pass());
   MessageLoop::current()->Run();
   EXPECT_EQ(kDataSize, test_helper_.GetCachedOriginUsage());
   EXPECT_EQ(ComputeCurrentOriginUsage(), test_helper_.GetCachedOriginUsage());
@@ -390,7 +392,7 @@ TEST_F(FileWriterDelegateTest, WritesWithQuotaAndOffset) {
   allowed_growth = 55;
   PrepareForWrite(kBlobURL, offset, allowed_growth);
 
-  file_writer_delegate_->Start(file_, request_.get());
+  file_writer_delegate_->Start(file_, request_.Pass());
   MessageLoop::current()->Run();
   EXPECT_EQ(offset + kDataSize, test_helper_.GetCachedOriginUsage());
   EXPECT_EQ(ComputeCurrentOriginUsage(), test_helper_.GetCachedOriginUsage());
@@ -404,7 +406,7 @@ TEST_F(FileWriterDelegateTest, WritesWithQuotaAndOffset) {
   PrepareForWrite(kBlobURL, offset, allowed_growth);
 
   int64 pre_write_usage = ComputeCurrentOriginUsage();
-  file_writer_delegate_->Start(file_, request_.get());
+  file_writer_delegate_->Start(file_, request_.Pass());
   MessageLoop::current()->Run();
   EXPECT_EQ(pre_write_usage, test_helper_.GetCachedOriginUsage());
   EXPECT_EQ(ComputeCurrentOriginUsage(), test_helper_.GetCachedOriginUsage());
@@ -419,7 +421,7 @@ TEST_F(FileWriterDelegateTest, WritesWithQuotaAndOffset) {
   allowed_growth = 10;
   PrepareForWrite(kBlobURL, offset, allowed_growth);
 
-  file_writer_delegate_->Start(file_, request_.get());
+  file_writer_delegate_->Start(file_, request_.Pass());
   MessageLoop::current()->Run();
   EXPECT_EQ(pre_write_usage + allowed_growth,
             test_helper_.GetCachedOriginUsage());