PPAPI: Get TrackedCallback ready for running on non-main threads.

PPAPI: Get TrackedCallback ready for running on non-main threads.

When CompletionCallbacks can run on background threads, ClearAndRun doesn't make sense anymore. Because the call may bounce to a different thread, we can't guarantee we'll run it right away, so setting the callback pointer to NULL is no longer a good way to indicate the callback has been run. Instead, callers should just use Run(), and rely on IsPending() to tell them if the call is still pending.

TrackedCallback also can not use WeakPtrs, because those DCHECK if they are dereferenced on a different thread than they were created on. In particular, if a PPB implementation calls callback_->Run(PP_OK), that almost always happens on the main thread, and creates a WeakPtr<TrackedCallback> there. But Run will sometimes have to schedule a task on a non-main thread, and the WeakPtr will fail there. Note that because all this happens behind the proxy lock, it actually would be safe. But I just went with a bool flag to do the same checking as before, rather than try to subvert the WeakPtr checks.

The CL for the Callbacks-on-background-threads feature is basically working and is here:
https://chromiumcodereview.appspot.com/10910099

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

[dmichael (off chromium), 2012-09-14 21:30:36]

http://codereview.chromium.org/10909244/diff/1/ppapi/shared_impl/tracked_call...
File ppapi/shared_impl/tracked_callback.cc (left):

http://codereview.chromium.org/10909244/diff/1/ppapi/shared_impl/tracked_call...
ppapi/shared_impl/tracked_callback.cc:64: // resource.
It turns out it does make sense...  ppapi_plugin_instance.cc will abort
instance-level callbacks like MouseLock on instance shutdown. So this check was
taken out.

[dmichael (off chromium), 2012-09-19 18:04:02]

Ahoy, matey. I've maaaarrrrged the CL. 'Tis not as haaarrd as it looks, and
sends several lines of cod[e] to Davy Jones's locker. If ye be looking for a
good way to perfcrastinate, here she blows!

[viettrungluu, 2012-09-25 00:18:11]

https://chromiumcodereview.appspot.com/10909244/diff/17001/ppapi/proxy/file_c...
File ppapi/proxy/file_chooser_resource.cc (right):

https://chromiumcodereview.appspot.com/10909244/diff/17001/ppapi/proxy/file_c...
ppapi/proxy/file_chooser_resource.cc:129: callback_->Run(params.result());
I don't see how you can leave the value of |callback_| in place in the face of
the usage of |TrackedCallback::IsPending(callback_);| -- am I missing something?

https://chromiumcodereview.appspot.com/10909244/diff/17001/ppapi/shared_impl/...
File ppapi/shared_impl/tracked_callback.cc (right):

https://chromiumcodereview.appspot.com/10909244/diff/17001/ppapi/shared_impl/...
ppapi/shared_impl/tracked_callback.cc:121: DCHECK(result == PP_ERROR_ABORTED ||
!is_scheduled_);
Is |is_scheduled_| checked anywhere else?

https://chromiumcodereview.appspot.com/10909244/diff/17001/ppapi/shared_impl/...
File ppapi/shared_impl/tracked_callback.h (right):

https://chromiumcodereview.appspot.com/10909244/diff/17001/ppapi/shared_impl/...
ppapi/shared_impl/tracked_callback.h:78: // callback will be schedule to run
later on target_loop_.
s/schedule/&d/

https://chromiumcodereview.appspot.com/10909244/diff/17001/ppapi/shared_impl/...
ppapi/shared_impl/tracked_callback.h:79: // TODO(dmichael): Make the above part
about different threads actually true.
Haha.

[dmichael (off chromium), 2012-10-24 21:40:52]

Thanks for the review! I'm back from leave now, sorry for the delay in
responding.

https://chromiumcodereview.appspot.com/10909244/diff/17001/ppapi/proxy/file_c...
File ppapi/proxy/file_chooser_resource.cc (right):

https://chromiumcodereview.appspot.com/10909244/diff/17001/ppapi/proxy/file_c...
ppapi/proxy/file_chooser_resource.cc:129: callback_->Run(params.result());
On 2012/09/25 00:18:11, viettrungluu wrote:
> I don't see how you can leave the value of |callback_| in place in the face of
> the usage of |TrackedCallback::IsPending(callback_);| -- am I missing
something?
IsPending(callback_) will return false if:
callback_ is valid
AND
callback_ has not yet completed.

Since we're doing Run(), the callback will complete. The downside with this
approach is the callback will never really be destroyed until it's replaced or
its owning resource goes away. But that seems pretty harmless.

The reason we can't use ClearAndRun anymore is because Run will (in the next CL)
potentially post a task to run the Callback on a different thread (i.e., the
thread that initiated the request). So this situation could happen:
1) Plugin makes a request (e.g., to show a FileChooser), but makes the request
from a background thread (which has an associated PPB_MessageLoop_Dev where the
callback should run).
2) At some time later, a response message comes back on the main thread, and we
have to schedule the callback to run on the background thread.
Suppose that another request comes in from the background thread before the
callback has a chance to fire. If we did ClearAndRun, then callback_ would be
NULL, so it would look like there was no callback pending. The new request would
be allowed, even though we haven't completely finished servicing the previous
request.

This way, IsPending will still show that there's a pending callback until the
callback actually runs and is put in the "completed" state.

https://chromiumcodereview.appspot.com/10909244/diff/17001/ppapi/shared_impl/...
File ppapi/shared_impl/tracked_callback.cc (right):

https://chromiumcodereview.appspot.com/10909244/diff/17001/ppapi/shared_impl/...
ppapi/shared_impl/tracked_callback.cc:121: DCHECK(result == PP_ERROR_ABORTED ||
!is_scheduled_);
On 2012/09/25 00:18:11, viettrungluu wrote:
> Is |is_scheduled_| checked anywhere else?
Nope. I was trying to keep parity with the weak_ptr_factory_ implementation,
which DCHECKs that there are no pending calls (but doesn't in PostAbort). It's
not really necessary, just a sanity check to catch callers who PostRun more than
necessary.

The previous implementation had the same behavior (at least, I intend the new to
match old), but I was trying to simplify the code a little bit (compare old
PostAbort to new PostAbort). This will be more important when PostRun gets
slightly more complicated by possibly having to post to a different thread:
http://codereview.chromium.org/10910099/diff/18001/ppapi/shared_impl/tracked_...

[dmichael (off chromium), 2012-10-31 16:03:05]

Ping?

[viettrungluu, 2012-11-02 21:29:24]

LGTM I think, but you break it you buy it. :)

Assuming that everyone is using IsPending() correctly (rather than looking at
the scoped_refptr), I think it should be okay. (I had to double-check that the
TrackedCallback is marked as completed before it actually calls the callback, in
case the callback does something to schedule another callback of the same type.
Too many "callbacks" in the preceding sentence; good luck making sense of it.)

I do wonder whether we should wrap scoped_refptr<TrackedCallback> (similar to
what's done in ppb_graphics_2d_impl.*) to prevent someone from checking that
it's null rather than using IsPending().

[commit-bot: I haz the power, 2012-11-05 17:43:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10909244/38001

[commit-bot: I haz the power, 2012-11-05 20:08:26]

Change committed as 166009