Pepper quota fix: fire callbacks asynchronously to avoid crash in write callback chain

webkit/plugins/ppapi/quota_file_io.cc

 // Copyright (c) 2011 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/plugins/ppapi/quota_file_io.h"
 
 #include "base/stl_util.h"
 #include "base/message_loop_proxy.h"
 #include "base/task.h"
 #include "webkit/plugins/ppapi/ppapi_plugin_instance.h"
@@ skipping 82 matching lines @@
     if (!base::FileUtilProxy::Write(
             quota_io_->instance_->delegate()->GetFileThreadMessageLoopProxy(),
             quota_io_->file_, offset_, buffer_.get(), bytes_to_write_,
             callback_factory_.NewCallback(&WriteOperation::DidFinish))) {
       DidFail(base::PLATFORM_FILE_ERROR_FAILED);
       return;
     }
   }
 
   virtual void DidFail(PlatformFileError error) OVERRIDE {
     base::MessageLoopProxy::CreateForCurrentThread()->PostTask(

[yzshen1, 2011/08/04 20:27:38]

Would you please explain why this needs to be a posted task?
(and also line 86, 166, 181)

It seems you added some PostTask calls after people LGTM-ed in the last code
review. In such case, I think it might be good to ask for another review before
submitting.

[kinuko, 2011/08/05 18:29:56]

The reason for these added post-tasks is: Run() is called synchronously in a
loop where we iterate on pending_operations, while we try popping from the same
queue in post-operation callbacks.  So if we call DidFinish here synchronously
it alters the queue in a dangerous way.

I reworked on this change to use (once again) two separate queues for pre- and
inflight-/post- operations, so that we can get rid of these bunch PostTasks in
an easier way.  Could you PTAL?
Very true, I will do so next time.

[yzshen1, 2011/08/05 20:31:46]

Thanks for explaining.

On 2011/08/05 18:29:56, kinuko wrote:

         FROM_HERE, runnable_factory_.NewRunnableMethod(
             &WriteOperation::DidFinish, error, 0));
   }
 
   bool finished() const { return finished_; }
 
+  void RunCallbackSoon() {
+    base::MessageLoopProxy::CreateForCurrentThread()->PostTask(
+        FROM_HERE, runnable_factory_.NewRunnableMethod(
+            &WriteOperation::RunCallback));
+  }
+
+ private:
   void RunCallback() {
     DCHECK(callback_.get());
     callback_->Run(status_, bytes_written_);
     callback_.reset();
     delete this;
   }
 
- private:
   void DidFinish(PlatformFileError status, int bytes_written) {
     finished_ = true;
     status_ = status;
     bytes_written_ = bytes_written;
     int64_t max_offset =
         (status != base::PLATFORM_FILE_OK) ? 0 : offset_ + bytes_written;
     // This may delete itself by calling RunCallback.
     quota_io_->DidWrite(this, max_offset);
   }
 
   const int64_t offset_;
   scoped_array<char> buffer_;
   const int32_t bytes_to_write_;
   scoped_ptr<WriteCallback> callback_;
   bool finished_;
   PlatformFileError status_;
   int64_t bytes_written_;
   base::ScopedCallbackFactory<QuotaFileIO::WriteOperation> callback_factory_;
   ScopedRunnableMethodFactory<QuotaFileIO::WriteOperation> runnable_factory_;
 };
 
 class QuotaFileIO::SetLengthOperation : public PendingOperationBase {
  public:
   SetLengthOperation(QuotaFileIO* quota_io,
                      bool is_will_operation,
                      int64_t length,
                      StatusCallback* callback)
       : PendingOperationBase(quota_io, is_will_operation),
         length_(length),
+        status_(base::PLATFORM_FILE_OK),
         callback_(callback),
         callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
         runnable_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {}
   virtual ~SetLengthOperation() {}
 
   virtual void Run() OVERRIDE {
     DCHECK(quota_io_);
     if (quota_io_->CheckIfExceedsQuota(length_)) {
       DidFail(base::PLATFORM_FILE_ERROR_NO_SPACE);
       return;
     }
     if (is_will_operation_) {
       base::MessageLoopProxy::CreateForCurrentThread()->PostTask(
           FROM_HERE, runnable_factory_.NewRunnableMethod(
-              &SetLengthOperation::DidFinish,
-              base::PLATFORM_FILE_OK));
+              &SetLengthOperation::DidFinish, base::PLATFORM_FILE_OK));
       return;
     }
     if (!base::FileUtilProxy::Truncate(
             quota_io_->instance_->delegate()->GetFileThreadMessageLoopProxy(),
             quota_io_->file_, length_,
             callback_factory_.NewCallback(&SetLengthOperation::DidFinish))) {
       DidFail(base::PLATFORM_FILE_ERROR_FAILED);
       return;
     }
   }
 
   virtual void DidFail(PlatformFileError error) OVERRIDE {
     base::MessageLoopProxy::CreateForCurrentThread()->PostTask(
         FROM_HERE, runnable_factory_.NewRunnableMethod(
             &SetLengthOperation::DidFinish, error));
   }
 
  private:
   void DidFinish(PlatformFileError status) {
     quota_io_->DidSetLength(status, length_);
+    status_ = status;
+    base::MessageLoopProxy::CreateForCurrentThread()->PostTask(
+        FROM_HERE, runnable_factory_.NewRunnableMethod(
+            &SetLengthOperation::RunCallback));
+  }
+
+  void RunCallback() {
     DCHECK(callback_.get());
-    callback_->Run(status);
+    callback_->Run(status_);
     callback_.reset();
     delete this;
   }
 
   int64_t length_;
+  PlatformFileError status_;
   scoped_ptr<StatusCallback> callback_;
   base::ScopedCallbackFactory<QuotaFileIO::SetLengthOperation>
       callback_factory_;
   ScopedRunnableMethodFactory<QuotaFileIO::SetLengthOperation>
       runnable_factory_;
 };
 
 // QuotaFileIO --------------------------------------------------------------
 
 QuotaFileIO::QuotaFileIO(
@@ skipping 132 matching lines @@
                            int64_t written_offset_end) {
   max_written_offset_ = std::max(max_written_offset_, written_offset_end);
   DCHECK_GT(inflight_operations_, 0);
   DCHECK(!pending_operations_.empty());
   // Fire callbacks for finished operations.
   while (!pending_operations_.empty()) {
     WriteOperation* op = static_cast<WriteOperation*>(
         pending_operations_.front());
     if (!op->finished())
       break;
-    op->RunCallback();
     pending_operations_.pop_front();
+    op->RunCallbackSoon();
   }
   // If we have no more pending writes, notify the browser that we did
   // update the file.
   if (--inflight_operations_ == 0) {
     DCHECK(pending_operations_.empty());
     int64_t growth = max_written_offset_ - cached_file_size_;
     growth = growth < 0 ? 0 : growth;
     instance_->delegate()->DidUpdateFile(file_url_, growth);
     max_written_offset_ = 0;
   }
 }
 
 void QuotaFileIO::DidSetLength(PlatformFileError error, int64_t new_file_size) {
   DCHECK_EQ(1, inflight_operations_);
   pending_operations_.pop_front();
   DCHECK(pending_operations_.empty());
   int64_t delta = (error != base::PLATFORM_FILE_OK) ? 0 :
       new_file_size - cached_file_size_;
   instance_->delegate()->DidUpdateFile(file_url_, delta);
   inflight_operations_ = 0;
 }
 
 }  // namespace ppapi
 }  // namespace webkit