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

webkit/fileapi/file_writer_delegate.cc

Index: webkit/fileapi/file_writer_delegate.cc
diff --git a/webkit/fileapi/file_writer_delegate.cc b/webkit/fileapi/file_writer_delegate.cc
index d95e00be3ec1108307db8ec2204d79a80d355e31..6d737026fb1bced0329a8f0edf2ef117cd827f72 100644
--- a/webkit/fileapi/file_writer_delegate.cc
+++ b/webkit/fileapi/file_writer_delegate.cc
@@ -91,6 +91,7 @@ FileWriterDelegate::FileWriterDelegate(
     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),
@@ -110,6 +111,10 @@ 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;
@@ -128,13 +133,18 @@ void FileWriterDelegate::OnGetFileInfoAndCallStartUpdate(
       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_,
@@ -144,6 +154,15 @@ void FileWriterDelegate::Start(base::PlatformFile file,
   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) {
@@ -172,7 +191,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 +209,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;
@@ -257,6 +276,13 @@ void FileWriterDelegate::Write() {
 
 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;
@@ -271,8 +297,11 @@ void FileWriterDelegate::OnDataWritten(int write_response) {
 }
 
 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());