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

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

The older code could screw up pending_operations_ in QuotaFileIO when another Write is issued in WriteCallback.  This patch is to fix the issue by always firing callbacks asynchronously.

BUG=86556
TEST=test_shell_tests:QuotaFileIOTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96154

[kinuko, 2011-08-04 12:26:09]

I have brought a bug in the new pepper quota code.  It crashes when another
Write is issued in a WriteCallback (since the new write could screw up the
pending_operations queue).  This patch is to fix the issue-- could you PTAL?

[yzshen1, 2011-08-04 20:27:37]

http://codereview.chromium.org/7508010/diff/7001/webkit/plugins/ppapi/quota_f...
File webkit/plugins/ppapi/quota_file_io.cc (right):

http://codereview.chromium.org/7508010/diff/7001/webkit/plugins/ppapi/quota_f...
webkit/plugins/ppapi/quota_file_io.cc:103:
base::MessageLoopProxy::CreateForCurrentThread()->PostTask(
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]

http://codereview.chromium.org/7508010/diff/7001/webkit/plugins/ppapi/quota_f...
File webkit/plugins/ppapi/quota_file_io.cc (right):

http://codereview.chromium.org/7508010/diff/7001/webkit/plugins/ppapi/quota_f...
webkit/plugins/ppapi/quota_file_io.cc:103:
base::MessageLoopProxy::CreateForCurrentThread()->PostTask(
On 2011/08/04 20:27:38, yzshen1 wrote:
> Would you please explain why this needs to be a posted task?
> (and also line 86, 166, 181)

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?

> 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.

Very true, I will do so next time.

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

LGTM

http://codereview.chromium.org/7508010/diff/7001/webkit/plugins/ppapi/quota_f...
File webkit/plugins/ppapi/quota_file_io.cc (right):

http://codereview.chromium.org/7508010/diff/7001/webkit/plugins/ppapi/quota_f...
webkit/plugins/ppapi/quota_file_io.cc:103:
base::MessageLoopProxy::CreateForCurrentThread()->PostTask(
Thanks for explaining.

On 2011/08/05 18:29:56, kinuko wrote:
> On 2011/08/04 20:27:38, yzshen1 wrote:
> > Would you please explain why this needs to be a posted task?
> > (and also line 86, 166, 181)
> 
> 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?
> 
> > 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.
> 
> Very true, I will do so next time.

http://codereview.chromium.org/7508010/diff/15006/webkit/plugins/ppapi/quota_...
webkit/plugins/ppapi/quota_file_io.cc:151: : PendingOperationBase(quota_io,
is_will_operation),
Is this 'protected' intended for all the methods below such as Run/DidFail?
IMO, it might be more clear to use the same access level as the definition in
the base class.

[brettw, 2011-08-06 00:13:51]

LGTM rubberstamp (I'm assuming Yuzhu went over the details).

[kinuko, 2011-08-08 11:21:12]

http://codereview.chromium.org/7508010/diff/15006/webkit/plugins/ppapi/quota_...
File webkit/plugins/ppapi/quota_file_io.cc (right):

http://codereview.chromium.org/7508010/diff/15006/webkit/plugins/ppapi/quota_...
webkit/plugins/ppapi/quota_file_io.cc:151: protected:
On 2011/08/05 20:31:46, yzshen1 wrote:
> Is this 'protected' intended for all the methods below such as Run/DidFail?
> IMO, it might be more clear to use the same access level as the definition in
> the base class.

Oops, this was a remnant of the other (reverted) change... removed.

Will be submitting without this line.