Add CompletionTask to PPAPI TrackedCallback.

Add CompletionTask to PPAPI TrackedCallback.
TrackedCallback handles re-entry into the Plugin. Add
a way to insert a task into this process so resources
can perform any completion work on the proper thread
without having to duplicate this re-entry work. 

BUG=
R=dmichael@chromium.org, yzshen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216789

[bbudge, 2013-08-07 20:58:00]

I don't need this yet, but David and I discussed this a way to simplify
completing asynchronous operations by taking advantage of the TrackedCallback's
knowledge of which thread to re-enter.

I haven't actually tried this to see if it actually helps for FileIO plugin-side
operations. I'm trying to land a slightly different approach there that only
optimizes Query and Read with blocking callbacks at first.

[bbudge, 2013-08-07 21:01:54]

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
File ppapi/shared_impl/tracked_callback.h (right):

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
ppapi/shared_impl/tracked_callback.h:88: // Task to perform any cleanup or
output before calling back into the plugin.
I should add more comments here:
// Task to perform cleanup or write output before calling back into the plugin.
// The |result| parameter can be queried to see if the callback has been
// aborted. The task should return the final result code.  The task is always
// called on the same  thread as the callback to the plugin.

[yzshen1, 2013-08-07 21:09:10]

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
ppapi/shared_impl/tracked_callback.cc:132: result = RunCompletionTask(result);
One thing that may be tricky is that this completion task may do things such as
running the tracked callback again. I think we should handle that case
carefully.

For example, if the completion task run the tracked callback, we will probably
end up running the PP_CompletionCallback twice. (Because at this point we don't
test whether the callback is in completed state anymore.)

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
File ppapi/shared_impl/tracked_callback.h (right):

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
ppapi/shared_impl/tracked_callback.h:93: void SetCompletionTask(const
CompletionTask& completion_task);
nit: I think it should be named set_completion_task().

[teravest, 2013-08-08 16:49:52]

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
ppapi/shared_impl/tracked_callback.cc:226: return result;
nit: I think you want to call completion_task_.Reset() here.

While it's strange, there's nothing stopping someone from calling
BlockUntilComplete() multiple times for a given TrackedCallback, but I think you
only want to run the completion_task_ once.

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
File ppapi/shared_impl/tracked_callback.h (right):

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
ppapi/shared_impl/tracked_callback.h:88: // Task to perform any cleanup or
output before calling back into the plugin.
On 2013/08/07 21:01:54, bbudge1 wrote:
> I should add more comments here:
> // Task to perform cleanup or write output before calling back into the
plugin.
> // The |result| parameter can be queried to see if the callback has been
> // aborted. The task should return the final result code.  The task is always
> // called on the same  thread as the callback to the plugin.

Sounds good.

[dmichael (off chromium), 2013-08-08 17:29:58]

https://chromiumcodereview.appspot.com/22606005/diff/3001/ppapi/shared_impl/t...
File ppapi/shared_impl/tracked_callback.cc (right):

https://chromiumcodereview.appspot.com/22606005/diff/3001/ppapi/shared_impl/t...
ppapi/shared_impl/tracked_callback.cc:132: result = RunCompletionTask(result);
I think the answer is "don't do that". The completion task needs to do fairly
simple things like write output parameters. IMHO, a comment would suffice.

That said, we should probably move this RunCompletionTask until after
MarkAsCompleted, so that it's consistent with the blocking case. That would make
doing "Run()" a no-op, if a CompletionTask tries to do Run() and a DCHECK if it
does PostRun.

[bbudge, 2013-08-08 18:33:59]

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
ppapi/shared_impl/tracked_callback.cc:132: result = RunCompletionTask(result);
On 2013/08/07 21:09:10, yzshen1 wrote:
> One thing that may be tricky is that this completion task may do things such
as
> running the tracked callback again. I think we should handle that case
> carefully.
> 
> For example, if the completion task run the tracked callback, we will probably
> end up running the PP_CompletionCallback twice. (Because at this point we
don't
> test whether the callback is in completed state anymore.)

I moved the task invocation to after MarkAsCompleted. We first have to copy the
task to a local given the comment below. However I'm not sure it's safe to
access object state in BlockUntilComplete after receiving the signal to wake up.

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
ppapi/shared_impl/tracked_callback.cc:226: return result;
On 2013/08/08 16:49:52, teravest wrote:
> nit: I think you want to call completion_task_.Reset() here.
> 
> While it's strange, there's nothing stopping someone from calling
> BlockUntilComplete() multiple times for a given TrackedCallback, but I think
you
> only want to run the completion_task_ once.

Yes, good catch. Another danger is if the task bundles up a ref to this, we have
a cycle and the callback will never be deleted. Done.

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
File ppapi/shared_impl/tracked_callback.h (right):

https://codereview.chromium.org/22606005/diff/3001/ppapi/shared_impl/tracked_...
ppapi/shared_impl/tracked_callback.h:93: void SetCompletionTask(const
CompletionTask& completion_task);
On 2013/08/07 21:09:10, yzshen1 wrote:
> nit: I think it should be named set_completion_task().

Done.

https://codereview.chromium.org/22606005/diff/11001/ppapi/shared_impl/tracked...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/22606005/diff/11001/ppapi/shared_impl/tracked...
ppapi/shared_impl/tracked_callback.cc:211: 
David, is it safe to access member fields here? Could the object have been
deleted where we Signal above?

[dmichael (off chromium), 2013-08-08 20:45:44]

Could you add testing of this to the unit test?

Other than that, I think it looks good.

https://codereview.chromium.org/22606005/diff/11001/ppapi/shared_impl/tracked...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/22606005/diff/11001/ppapi/shared_impl/tracked...
ppapi/shared_impl/tracked_callback.cc:211: 
On 2013/08/08 18:34:00, bbudge1 wrote:
> David, is it safe to access member fields here? Could the object have been
> deleted where we Signal above?
Should be safe.

There are two things that could make it unsafe:
1) If we didn't have the lock. But we came back from the condvar Wait, so we're
good there.
2) If the TrackedCallback could have been deleted. But BlockUntilComplete is
only called from EnterBase::SetResult (we use friendship and keep
BlockUntilPrivate private to help enforce that). EnterBase keeps a
scoped_refptr<TrackedCallback> while it's on the stack. We should be good. But
if you want to be super careful, you could add a local:
 scoped_refptr<TrackedCallback> pin_in_memory(this);

[yzshen1, 2013-08-08 23:55:49]

LGTM

https://codereview.chromium.org/22606005/diff/11001/ppapi/shared_impl/tracked...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/22606005/diff/11001/ppapi/shared_impl/tracked...
ppapi/shared_impl/tracked_callback.cc:137: // shouldn't happen, but avoid
strange failures).
nit: Please update the comment: With completion_task_ now reentrancy may happen
if we don't mark as completed. (so the part in the parentheses is not needed).

[bbudge, 2013-08-09 18:56:25]

https://codereview.chromium.org/22606005/diff/11001/ppapi/shared_impl/tracked...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/22606005/diff/11001/ppapi/shared_impl/tracked...
ppapi/shared_impl/tracked_callback.cc:137: // shouldn't happen, but avoid
strange failures).
On 2013/08/08 23:55:49, yzshen1 wrote:
> nit: Please update the comment: With completion_task_ now reentrancy may
happen
> if we don't mark as completed. (so the part in the parentheses is not needed).

Done.

[dmichael (off chromium), 2013-08-09 21:14:00]

lgtm, thanks for the unit test!

https://chromiumcodereview.appspot.com/22606005/diff/31001/ppapi/shared_impl/...
File ppapi/shared_impl/tracked_callback_unittest.cc (right):

https://chromiumcodereview.appspot.com/22606005/diff/31001/ppapi/shared_impl/...
ppapi/shared_impl/tracked_callback_unittest.cc:196: int32_t
CompletionTask(CallbackRunInfo* info, int32_t result) {
I think it would make sense to do EXPECT_EQ(0, info->run_count);

...just to ensure that the completion task happens prior to running the
callback.

[bbudge, 2013-08-09 21:36:29]

https://codereview.chromium.org/22606005/diff/31001/ppapi/shared_impl/tracked...
File ppapi/shared_impl/tracked_callback_unittest.cc (right):

https://codereview.chromium.org/22606005/diff/31001/ppapi/shared_impl/tracked...
ppapi/shared_impl/tracked_callback_unittest.cc:196: int32_t
CompletionTask(CallbackRunInfo* info, int32_t result) {
On 2013/08/09 21:14:01, dmichael wrote:
> I think it would make sense to do EXPECT_EQ(0, info->run_count);
> 
> ...just to ensure that the completion task happens prior to running the
> callback.

Nice, done!

[commit-bot: I haz the power, 2013-08-09 21:40:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/22606005/39001

[bbudge, 2013-08-10 00:20:21]

Committed patchset #5 manually as r216789 (presubmit successful).