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

webkit/fileapi/file_writer_delegate.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 "webkit/fileapi/file_writer_delegate.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/file_util_proxy.h"
 #include "base/message_loop.h"
@@ skipping 73 matching lines @@
 };
 
 }  // namespace (anonymous)
 
 FileWriterDelegate::FileWriterDelegate(
     FileSystemOperation* file_system_operation,
     const FileSystemPath& path,
     int64 offset,
     scoped_refptr<base::MessageLoopProxy> proxy)
     : file_system_operation_(file_system_operation),
+      state_(FILE_WRITER_STATE_CREATED),
       file_(base::kInvalidPlatformFileValue),
       path_(path),
       offset_(offset),
       proxy_(proxy),
       bytes_written_backlog_(0),
       bytes_written_(0),
       bytes_read_(0),
       total_bytes_written_(0),
       allowed_bytes_to_write_(0),
       io_buffer_(new net::IOBufferWithSize(kReadBufSize)),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
 }
 
 FileWriterDelegate::~FileWriterDelegate() {
 }
 
 void FileWriterDelegate::OnGetFileInfoAndCallStartUpdate(
     base::PlatformFileError error,
     const base::PlatformFileInfo& file_info) {
+  if (state_ == FILE_WRITER_STATE_CANCELED) {

[ericu, 2012/04/10 22:10:32]

If we cancel while starting up, we call OnError(ABORT) here.  But if we cancel
later, we don't call OnError.  Is that right?

[kinuko, 2012/04/11 01:11:46]

We will eventually call OnError (or OnProgress(complete=true)) in either case.

+    OnError(base::PLATFORM_FILE_ERROR_ABORT);
+    return;
+  }
   if (error) {
     OnError(error);
     return;
   }
   int64 allowed_bytes_growth =
       file_system_operation_context()->allowed_bytes_growth();
   if (allowed_bytes_growth < 0)
     allowed_bytes_growth = 0;
   int64 overlap = file_info.size - offset_;
   allowed_bytes_to_write_ = allowed_bytes_growth;
   if (kint64max - overlap > allowed_bytes_growth)
     allowed_bytes_to_write_ += overlap;
   size_ = file_info.size;
   file_stream_.reset(new net::FileStream(
       file_,
       base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_WRITE |
       base::PLATFORM_FILE_ASYNC,
       NULL));
+  DCHECK_EQ(FILE_WRITER_STATE_CREATED, state_);
+  state_ = FILE_WRITER_STATE_REQUEST_STARTED;
   request_->Start();
 }
 
 void FileWriterDelegate::Start(base::PlatformFile file,
-                               net::URLRequest* request) {
+                               scoped_ptr<net::URLRequest> request) {
+  DCHECK(!request_.get());
+  DCHECK_EQ(FILE_WRITER_STATE_CREATED, state_);
+
   file_ = file;
-  request_ = request;
+  request_ = request.Pass();
 
   scoped_refptr<InitializeTask> relay = new InitializeTask(
       file_, path_,
       file_system_operation_context(),
       base::Bind(&FileWriterDelegate::OnGetFileInfoAndCallStartUpdate,
                  weak_factory_.GetWeakPtr()));
   relay->Start(proxy_, FROM_HERE);
 }
 
+void FileWriterDelegate::Cancel() {
+  if (state_ == FILE_WRITER_STATE_REQUEST_STARTED) {
+    DCHECK(request_.get());
+    // This halts any callbacks on this delegate.
+    request_->Cancel();
+  }
+  state_ = FILE_WRITER_STATE_CANCELED;

[ericu, 2012/04/10 22:10:32]

This doesn't call quota_util()->proxy()->EndUpdateOrigin(path_.origin(),
path_.type()); should it?

What about file_system_operation->didWrite()?

[kinuko, 2012/04/11 01:11:46]

We'll eventually call it.

If we've already started the request state_ must be
FILE_WRITER_STATE_REQUEST_STARTED (if not _CANCELED) and we call
request_->Cancel() which will in turn calls FWD::OnReadCompleted() where we hook
the quota code in OnError if request_->state().is_success() is false (and it
is).

If we're cancelled between Write() and OnDataWritten() we update the quota in
OnProgress (and bail out).

[ericu, 2012/04/11 03:24:13]

The URLRequest header comments say
  // ...It is guaranteed that no
  // methods of the delegate will be called after the request has been
  // cancelled, except that this may call the delegate's OnReadCompleted()
  // during the call to Cancel itself.

That "may" doesn't sound like a guarantee, more like a warning.  If we're going
to assume that it's a contract, we should 1) audit the code to make sure it's
always true, and that we won't want to change it; 2) change this comment.
If we Cancel between Read and OnDataReceived, we won't notice the cancel for a
while, though.  Should we check in OnDataReceived as well?

[kinuko, 2012/04/11 09:06:02]

Oh ok, I dived into the code before looking at the comment but probably we
should rather trust what the comment says.  It sounds like we'll need to make
some more changes in the Cancel flow together with this one though...
Sounds good, will do.

+}
+
 void FileWriterDelegate::OnReceivedRedirect(net::URLRequest* request,
                                             const GURL& new_url,
                                             bool* defer_redirect) {
   NOTREACHED();
   OnError(base::PLATFORM_FILE_ERROR_SECURITY);
 }
 
 void FileWriterDelegate::OnAuthRequired(net::URLRequest* request,
                                         net::AuthChallengeInfo* auth_info) {
   NOTREACHED();
   OnError(base::PLATFORM_FILE_ERROR_SECURITY);
 }
 
 void FileWriterDelegate::OnCertificateRequested(
     net::URLRequest* request,
     net::SSLCertRequestInfo* cert_request_info) {
   NOTREACHED();
   OnError(base::PLATFORM_FILE_ERROR_SECURITY);
 }
 
 void FileWriterDelegate::OnSSLCertificateError(net::URLRequest* request,
                                                const net::SSLInfo& ssl_info,
                                                bool fatal) {
   NOTREACHED();
   OnError(base::PLATFORM_FILE_ERROR_SECURITY);
 }
 
 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;
   if (!request->status().is_success() || request->GetResponseCode() != 200) {
     OnError(base::PLATFORM_FILE_ERROR_FAILED);
     return;
   }
   int64 error = file_stream_->Seek(net::FROM_BEGIN, offset_);
   if (error != offset_) {
     OnError(base::PLATFORM_FILE_ERROR_FAILED);
     return;
   }
   Read();
 }
 
 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;
   }
   OnDataReceived(bytes_read);
 }
 
 void FileWriterDelegate::Read() {
   bytes_written_ = 0;
   bytes_read_ = 0;
@@ skipping 46 matching lines @@
     MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(&FileWriterDelegate::OnDataWritten,
                    weak_factory_.GetWeakPtr(), write_response));
   else if (net::ERR_IO_PENDING != write_response)
     OnError(base::PLATFORM_FILE_ERROR_FAILED);
 }
 
 void FileWriterDelegate::OnDataWritten(int write_response) {
   if (write_response > 0) {
+    if (state_ == FILE_WRITER_STATE_CANCELED) {
+      // If the request has been cancelled (we could still be here if Cancel
+      // is made right after the last write) OnProgress will delete this
+      // (in FileWriterDelegate::DidWrite).

[ericu, 2012/04/10 22:10:32]

s/FileWriterDelete/FileSystemOperation/?

[kinuko, 2012/04/11 01:11:46]

Done.

+      OnProgress(write_response, true);
+      return;
+    }
     OnProgress(write_response, false);
     cursor_->DidConsume(write_response);
     bytes_written_ += write_response;
     total_bytes_written_ += write_response;
     if (bytes_written_ == bytes_read_)
       Read();
     else
       Write();
   } else {
     OnError(base::PLATFORM_FILE_ERROR_FAILED);
   }
 }
 
 void FileWriterDelegate::OnError(base::PlatformFileError error) {
-  request_->set_delegate(NULL);
-  request_->Cancel();
+  if (state_ == FILE_WRITER_STATE_REQUEST_STARTED) {
+    DCHECK(request_.get());
+    request_->set_delegate(NULL);
+    request_->Cancel();
+  }
 
   if (quota_util())
     quota_util()->proxy()->EndUpdateOrigin(path_.origin(), path_.type());
 
   file_system_operation_->DidWrite(error, 0, true);
 }
 
 void FileWriterDelegate::OnProgress(int bytes_written, bool done) {
   DCHECK(bytes_written + bytes_written_backlog_ >= bytes_written_backlog_);
   if (quota_util() &&
@@ skipping 33 matching lines @@
 
 FileSystemQuotaUtil* FileWriterDelegate::quota_util() const {
   DCHECK(file_system_operation_);
   DCHECK(file_system_operation_->file_system_context());
   DCHECK(file_system_operation_->file_system_operation_context());
   return file_system_operation_->file_system_context()->GetQuotaUtil(
       path_.type());
 }
 
 }  // namespace fileapi