PPAPI: Make TrackedCallback more threadsafe

PPAPI: Make TrackedCallback more threadsafe

In particular, make it possible to invoke Run() and friends from any thread
even without holding the ProxyLock. See:
https://codereview.chromium.org/869883003/
for more context. Summary: We need to be able to Run the callback from the IO
thread, but we have to never acquire the ProxyLock on the IO thread.
(See bug for still more context).

BUG=439588
Committed: https://crrev.com/342e88099292432d7b33988a82609ea906f75863
Cr-Commit-Position: refs/heads/master@{#323133}

[dmichael (off chromium), 2015-02-13 23:46:36]

Patchset #1 (id:1) has been deleted

[dmichael (off chromium), 2015-02-13 23:48:17]

dmichael@chromium.org changed reviewers:
+ bbudge@chromium.org

[dmichael (off chromium), 2015-02-13 23:48:18]

I think this is ready for at least a first-look. Thanks/sorry

[dmichael (off chromium), 2015-02-23 23:14:44]

ping?

[bbudge, 2015-02-25 23:51:32]

Bunch of random comments.

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
File ppapi/shared_impl/callback_tracker.cc (right):

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.cc:47: CHECK(resource_id != 0);
Why do we CHECK for 0 but fail silently for other invalid resource_id's?

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
File ppapi/shared_impl/callback_tracker.h (right):

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.h:33: //    instance) will be aborted.
This comment is confusing since we're talking about module (so what instance?)
and also because we recommend against implementing PPP_ShutdownModule at all:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/ppp.h&l=75

Perhaps we should say before an instance is destroyed?

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.h:81: //        necessary.
Looking at TrackedCallback::PostAbort / PostRun, it seems the callback could be
run (blocking callback signalled) rather than posted as a task.

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.cc:114: if (result == PP_ERROR_ABORTED)
Would a DCHECK(!aborted_) be useful here?

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.cc:165: if (!callback.get())
While you're here, could we eliminate all the .get()'s? They hurt readability a
tiny bit IMO.

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.cc:196: // Protect us from being deleted to
ensure operation_completd_condvar_ is
sp/completd

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.cc:203: // Note that the condvar has
re-acquired the ProxyLock for us, and we will
I had to think about this a bit before understanding: perhaps say "signaling the
condvar has re-acquired the ProxyLock"?

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.cc:244: aborted_ = true;
DCHECK(!aborted_) ?

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
File ppapi/shared_impl/tracked_callback.h (right):

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.h:59: // without the ProxyLock. This is used
to allow signalling the callback from
It might not be clear to readers what "signalling the callback" means.

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.h:151: bool is_optional() {
This doesn't seem to be used anywhere. Can we eliminate it?

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.h:164: // to check the type of callback,
block, or mark them complete explicitly.
Some of our resources do check whether callbacks are blocking. Is that what this
comment means by "type"?

https://codereview.chromium.org/923263003/diff/40001/ppapi/shared_impl/tracke...
File ppapi/shared_impl/tracked_callback.h (right):

https://codereview.chromium.org/923263003/diff/40001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.h:107: base::AutoLock acquire(lock_);
As we discussed, I don't see the point of protecting boolean fields with the
lock.

https://codereview.chromium.org/923263003/diff/40001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.h:158: void PostRunImpl(int32_t result);
PostRunWithLock?

[dmichael (off chromium), 2015-02-27 22:04:41]

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
File ppapi/shared_impl/callback_tracker.cc (right):

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.cc:47: CHECK(resource_id != 0);
On 2015/02/25 23:51:31, bbudge wrote:
> Why do we CHECK for 0 but fail silently for other invalid resource_id's?
0 is invalid and would imply a coding error in ResourceTracker when we call
this. The other cases are for when there was a valid resource that just had no
callbacks. I added some explanatory comments.

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
File ppapi/shared_impl/callback_tracker.h (right):

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.h:33: //    instance) will be aborted.
On 2015/02/25 23:51:31, bbudge wrote:
> This comment is confusing since we're talking about module (so what instance?)
> and also because we recommend against implementing PPP_ShutdownModule at all:
> https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/ppp.h&l=75
> 
> Perhaps we should say before an instance is destroyed?
Good point. I replaced module with instance...  is that roughly what you wanted?

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.h:81: //        necessary.
On 2015/02/25 23:51:31, bbudge wrote:
> Looking at TrackedCallback::PostAbort / PostRun, it seems the callback could
be
> run (blocking callback signalled) rather than posted as a task.
I think we want PostAbort because if we happen to be on the "right" thread for
an async callback, we would invoke the callback immediately, even though the
plugin is in the middle of PPB_Core.ReleaseResource. Using PostAbort ensures we
don't re-enter them.

Sorry I left a FIXME; I meant to clean these up. I think the answer here is that
we *could* require the ProxyLock, since we only call it with the ProxyLock. But
there's nothing in the method that requires it. It looks like the same is true
of AbortAll(). (I think at one point I thought I'd make the "PostRun" family
safe without the proxy lock, but require the ProxyLock for "Run". But I had to
make Run and Abort also OK without the ProxyLock...  so CallbackTracker now
doesn't care about the proxy lock, I think.

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.cc:114: if (result == PP_ERROR_ABORTED)
On 2015/02/25 23:51:31, bbudge wrote:
> Would a DCHECK(!aborted_) be useful here?
No :(

aborted_ can be set in PostRunWithLock. The idea is that any attempt to Abort()
or PostAbort() the callback should take precedence over any other value sent to
Run() or PostRun(). So imagine that something like this happens:
1) PostRun(PP_OK)
2) Resource is deleted, so we do PostAbort()
3) The "Run(PP_OK)" task runs before the RUN(PP_ERROR_ABORTED)

In step 2, we will have set aborted_, to make sure when we get to step 3 we can
override the result (see comment just below here).

Sorry, it's kind of confusing. There might be a nicer way to have done that.

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.cc:165: if (!callback.get())
On 2015/02/25 23:51:31, bbudge wrote:
> While you're here, could we eliminate all the .get()'s? They hurt readability
a
> tiny bit IMO.

Done, thanks

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.cc:203: // Note that the condvar has
re-acquired the ProxyLock for us, and we will
On 2015/02/25 23:51:31, bbudge wrote:
> I had to think about this a bit before understanding: perhaps say "signaling
the
> condvar has re-acquired the ProxyLock"?
I tried rephrasing it; let me know if it's still confusing.

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.cc:244: aborted_ = true;
On 2015/02/25 23:51:31, bbudge wrote:
> DCHECK(!aborted_) ?
Some APIs will call PostAbort (e.g. if there's a "Close()" operation). The
callback could also get PostAbort if the resource is subsequently released. So I
don't think we can safely assert that.

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
File ppapi/shared_impl/tracked_callback.h (right):

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.h:151: bool is_optional() {
On 2015/02/25 23:51:31, bbudge wrote:
> This doesn't seem to be used anywhere. Can we eliminate it?

Done.

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.h:164: // to check the type of callback,
block, or mark them complete explicitly.
On 2015/02/25 23:51:31, bbudge wrote:
> Some of our resources do check whether callbacks are blocking. Is that what
this
> comment means by "type"?
Right. I tried improving & updating the callback. I think I wrote that comment
before the FileIO optimization you did.

https://codereview.chromium.org/923263003/diff/40001/ppapi/shared_impl/tracke...
File ppapi/shared_impl/tracked_callback.h (right):

https://codereview.chromium.org/923263003/diff/40001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.h:107: base::AutoLock acquire(lock_);
On 2015/02/25 23:51:31, bbudge wrote:
> As we discussed, I don't see the point of protecting boolean fields with the
> lock.
Removed the method, as we discussed.

https://codereview.chromium.org/923263003/diff/40001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.h:158: void PostRunImpl(int32_t result);
On 2015/02/25 23:51:31, bbudge wrote:
> PostRunWithLock?
Yeah, that's better, thanks

https://codereview.chromium.org/923263003/diff/60001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.cc (right):

https://codereview.chromium.org/923263003/diff/60001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:140: // FIXME, just delete this whole file
when merging.
When I can rebase this on top of removing FileMapping, this change will go
away...  this just makes it compile/pass by ignoring the very special case :)

[bbudge, 2015-03-05 01:29:35]

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
File ppapi/shared_impl/callback_tracker.cc (right):

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.cc:47: CHECK(resource_id != 0);
On 2015/02/27 22:04:40, dmichael wrote:
> On 2015/02/25 23:51:31, bbudge wrote:
> > Why do we CHECK for 0 but fail silently for other invalid resource_id's?
> 0 is invalid and would imply a coding error in ResourceTracker when we call
> this. The other cases are for when there was a valid resource that just had no
> callbacks. I added some explanatory comments.

OK

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
File ppapi/shared_impl/callback_tracker.h (right):

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.h:81: //        necessary.
On 2015/02/27 22:04:40, dmichael wrote:
> On 2015/02/25 23:51:31, bbudge wrote:
> > Looking at TrackedCallback::PostAbort / PostRun, it seems the callback could
> be
> > run (blocking callback signalled) rather than posted as a task.
> I think we want PostAbort because if we happen to be on the "right" thread for
> an async callback, we would invoke the callback immediately, even though the
> plugin is in the middle of PPB_Core.ReleaseResource. Using PostAbort ensures
we
> don't re-enter them.
> 
> Sorry I left a FIXME; I meant to clean these up. I think the answer here is
that
> we *could* require the ProxyLock, since we only call it with the ProxyLock.
But
> there's nothing in the method that requires it. It looks like the same is true
> of AbortAll(). (I think at one point I thought I'd make the "PostRun" family
> safe without the proxy lock, but require the ProxyLock for "Run". But I had to
> make Run and Abort also OK without the ProxyLock...  so CallbackTracker now
> doesn't care about the proxy lock, I think.

OK, that is better.

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/923263003/diff/20001/ppapi/shared_impl/tracke...
ppapi/shared_impl/tracked_callback.cc:114: if (result == PP_ERROR_ABORTED)
On 2015/02/27 22:04:40, dmichael wrote:
> On 2015/02/25 23:51:31, bbudge wrote:
> > Would a DCHECK(!aborted_) be useful here?
> No :(
> 
> aborted_ can be set in PostRunWithLock. The idea is that any attempt to
Abort()
> or PostAbort() the callback should take precedence over any other value sent
to
> Run() or PostRun(). So imagine that something like this happens:
> 1) PostRun(PP_OK)
> 2) Resource is deleted, so we do PostAbort()
> 3) The "Run(PP_OK)" task runs before the RUN(PP_ERROR_ABORTED)
> 
> In step 2, we will have set aborted_, to make sure when we get to step 3 we
can
> override the result (see comment just below here).
> 
> Sorry, it's kind of confusing. There might be a nicer way to have done that. 

OK, makes sense.

https://codereview.chromium.org/923263003/diff/60001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.cc (right):

https://codereview.chromium.org/923263003/diff/60001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:140: // FIXME, just delete this whole file
when merging.
On 2015/02/27 22:04:40, dmichael wrote:
> When I can rebase this on top of removing FileMapping, this change will go
> away...  this just makes it compile/pass by ignoring the very special case :)

OK.

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
File ppapi/proxy/tracked_callback_unittest.cc (right):

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
ppapi/proxy/tracked_callback_unittest.cc:33: CallbackThread(PP_Instance
instance)
nit: explicit

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
ppapi/proxy/tracked_callback_unittest.cc:37: loop_ = 0;
Could we use nullptr here?

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
ppapi/proxy/tracked_callback_unittest.cc:56: MessageLoopResource* loop() {
return loop_.get(); }
nit: s/loop/message_loop
It confused me first time I read the file.

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
ppapi/proxy/tracked_callback_unittest.cc:79: thread_.Join();
Would it be cleaner to pull the above code into a CallbackThread::Stop() method,
to go with the Start() method?

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
ppapi/proxy/tracked_callback_unittest.cc:90: explicit
CallbackRunInfo(base::ThreadChecker* thread_checker)
I don't see any calls of this. It looks like you have a default ctor that is
getting used.

https://codereview.chromium.org/923263003/diff/80001/ppapi/shared_impl/callba...
File ppapi/shared_impl/callback_tracker.cc (right):

https://codereview.chromium.org/923263003/diff/80001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.cc:30: }
Reading this code, it seems possible for a callback to be added to
pending_callbacks_ on another thread after you drop the lock here. As we
discussed, that isn't possible since the ProxyLock is held, so there should be
an AssertAcquired here.

Also, below.

https://codereview.chromium.org/923263003/diff/80001/ppapi/shared_impl/callba...
File ppapi/shared_impl/callback_tracker.h (right):

https://codereview.chromium.org/923263003/diff/80001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.h:33: //    instance) will be aborted.
how about s/that instance/every instance of the module/ ?

https://codereview.chromium.org/923263003/diff/80001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.h:51: //    guaranteed that those threads
will no longer write into the buffers.)
s/guaranteed/be guaranteed

https://codereview.chromium.org/923263003/diff/80001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.h:64: // callback is (just about to be)
completed, it is removed from the tracker. We
nit: wording
s/When a callback is (just about to be) completed,/Just before a callback is
completed/

https://codereview.chromium.org/923263003/diff/80001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.h:67: // callbacks associated to a given
resource are aborted when a plugin instance
nit: s/to/with

[dmichael (off chromium), 2015-03-18 15:51:48]

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
File ppapi/proxy/tracked_callback_unittest.cc (right):

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
ppapi/proxy/tracked_callback_unittest.cc:33: CallbackThread(PP_Instance
instance)
On 2015/03/05 01:29:35, bbudge wrote:
> nit: explicit

Done.

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
ppapi/proxy/tracked_callback_unittest.cc:37: loop_ = 0;
On 2015/03/05 01:29:35, bbudge wrote:
> Could we use nullptr here?
That's better, not sure what I was thinking.

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
ppapi/proxy/tracked_callback_unittest.cc:56: MessageLoopResource* loop() {
return loop_.get(); }
On 2015/03/05 01:29:35, bbudge wrote:
> nit: s/loop/message_loop
> It confused me first time I read the file.

Done.

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
ppapi/proxy/tracked_callback_unittest.cc:79: thread_.Join();
On 2015/03/05 01:29:35, bbudge wrote:
> Would it be cleaner to pull the above code into a CallbackThread::Stop()
method,
> to go with the Start() method?
How about I override Join()? That seems like a nice parallel.

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
ppapi/proxy/tracked_callback_unittest.cc:90: explicit
CallbackRunInfo(base::ThreadChecker* thread_checker)
On 2015/03/05 01:29:35, bbudge wrote:
> I don't see any calls of this. It looks like you have a default ctor that is
> getting used.
See the initializer lists for CallbackShutdownTest and CallbackMockResource. You
don't get a default constructor for free if you define any other constructors.

https://codereview.chromium.org/923263003/diff/80001/ppapi/shared_impl/callba...
File ppapi/shared_impl/callback_tracker.cc (right):

https://codereview.chromium.org/923263003/diff/80001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.cc:30: }
On 2015/03/05 01:29:35, bbudge wrote:
> Reading this code, it seems possible for a callback to be added to
> pending_callbacks_ on another thread after you drop the lock here. As we
> discussed, that isn't possible since the ProxyLock is held, so there should be
> an AssertAcquired here.
> 
> Also, below.
Done, thanks.

https://codereview.chromium.org/923263003/diff/80001/ppapi/shared_impl/callba...
File ppapi/shared_impl/callback_tracker.h (right):

https://codereview.chromium.org/923263003/diff/80001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.h:33: //    instance) will be aborted.
On 2015/03/05 01:29:35, bbudge wrote:
> how about s/that instance/every instance of the module/ ?

Ah, good catch, I had "fixed" the comment and made it wrong.

https://codereview.chromium.org/923263003/diff/80001/ppapi/shared_impl/callba...
ppapi/shared_impl/callback_tracker.h:51: //    guaranteed that those threads
will no longer write into the buffers.)
On 2015/03/05 01:29:35, bbudge wrote:
> s/guaranteed/be guaranteed
I think Trung wrote comment before we did background threads, and we didn't do
it that way. I updated it with a reference to TrackedCallback's completion tasks
instead. (Unfortunately, we're not totally consistent about this and there are
APIs currently that can write to output buffers from a background thread, and
the resource can get deleted in the interim. IIRC, UDP does that currently, but
I think that's one of the things I fix in the followup CL).

[bbudge, 2015-03-19 00:21:11]

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
File ppapi/proxy/tracked_callback_unittest.cc (right):

https://codereview.chromium.org/923263003/diff/80001/ppapi/proxy/tracked_call...
ppapi/proxy/tracked_callback_unittest.cc:90: explicit
CallbackRunInfo(base::ThreadChecker* thread_checker)
On 2015/03/18 15:51:48, dmichael wrote:
> On 2015/03/05 01:29:35, bbudge wrote:
> > I don't see any calls of this. It looks like you have a default ctor that is
> > getting used.
> See the initializer lists for CallbackShutdownTest and CallbackMockResource.
You
> don't get a default constructor for free if you define any other constructors.

Missed those, you're right.

https://codereview.chromium.org/923263003/diff/140001/ppapi/shared_impl/callb...
File ppapi/shared_impl/callback_tracker.cc (right):

https://codereview.chromium.org/923263003/diff/140001/ppapi/shared_impl/callb...
ppapi/shared_impl/callback_tracker.cc:49: CallbackSet callbacks_for_resource;
It seems like we need the proxy lock here for the same reason as AbortAll.

https://codereview.chromium.org/923263003/diff/140001/ppapi/shared_impl/callb...
ppapi/shared_impl/callback_tracker.cc:73: // that no callbacks are added while
we're aborting them all (see AbortAll).
This comment doesn't make sense here. Do we really need to hold the proxy lock?

[dmichael (off chromium), 2015-03-19 17:50:59]

https://codereview.chromium.org/923263003/diff/140001/ppapi/shared_impl/callb...
File ppapi/shared_impl/callback_tracker.cc (right):

https://codereview.chromium.org/923263003/diff/140001/ppapi/shared_impl/callb...
ppapi/shared_impl/callback_tracker.cc:49: CallbackSet callbacks_for_resource;
On 2015/03/19 00:21:10, bbudge wrote:
> It seems like we need the proxy lock here for the same reason as AbortAll.
Hmm. The more I think about it, the more I think the lock doesn't do anything
for us. We're basically talking about "AbortAll" getting called around the same
time as Add on another thread. Forgetting for the moment that that can't happen
due to both holding the ProxyLock, what's the outcome?

Also let's ignore that no Adds should happen after AbortAll just based on the
fact that AbortAll happens at instance destruction.

Ignoring those, imagine that we don't assume the lock is held, and a call to
AbortAll beats the Add. We end up aborting all the callbacks that existed at the
time AbortAll was called. Then, another callback is added that won't be aborted.

Imagine instead we guarantee that the ProxyLock is held. Can't we still get in
the same situation where Add gets the ProxyLock just after AbortAll is done? It
seems like we haven't really gained anything.

If we want to be bullet-proof, maybe we're better off having a "aborted_all_"
kind of flag that we check in Add? That, or just don't worry about it.

https://codereview.chromium.org/923263003/diff/140001/ppapi/shared_impl/callb...
ppapi/shared_impl/callback_tracker.cc:73: // that no callbacks are added while
we're aborting them all (see AbortAll).
On 2015/03/19 00:21:10, bbudge wrote:
> This comment doesn't make sense here. Do we really need to hold the proxy
lock?
If we don't have it, having the proxy lock in AbortAll wouldn't be meaningful,
would it?

Anyway, I think I convinced myself that the lock doesn't accomplish anything
anyway. WDYT?

[dmichael (off chromium), 2015-03-25 17:15:58]

I made a small simplification that helps in a future CL.

https://codereview.chromium.org/923263003/diff/220001/ppapi/shared_impl/proxy...
File ppapi/shared_impl/proxy_lock.h (left):

https://codereview.chromium.org/923263003/diff/220001/ppapi/shared_impl/proxy...
ppapi/shared_impl/proxy_lock.h:180: ProxyLock::AssertAcquired();
I needed to be able to use RunWhileLocked on the IO thread without the
ProxyLock, and I realized that for a while now, VarTracker and ResourceTracker
do their own checking, so we should be protected from mistakes like binding a
ScopedPPVar without the ProxyLock...
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...

[bbudge, 2015-03-27 23:46:20]

https://codereview.chromium.org/923263003/diff/140001/ppapi/shared_impl/callb...
File ppapi/shared_impl/callback_tracker.cc (right):

https://codereview.chromium.org/923263003/diff/140001/ppapi/shared_impl/callb...
ppapi/shared_impl/callback_tracker.cc:73: // that no callbacks are added while
we're aborting them all (see AbortAll).
On 2015/03/19 17:50:58, dmichael wrote:
> On 2015/03/19 00:21:10, bbudge wrote:
> > This comment doesn't make sense here. Do we really need to hold the proxy
> lock?
> If we don't have it, having the proxy lock in AbortAll wouldn't be meaningful,
> would it?
> 
> Anyway, I think I convinced myself that the lock doesn't accomplish anything
> anyway. WDYT?

OK, that makes sense.

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/callb...
File ppapi/shared_impl/callback_tracker.cc (right):

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/callb...
ppapi/shared_impl/callback_tracker.cc:45: CHECK_NE(resource_id, 0);
I think I asked already, but why not just DCHECK here? It's an internal error
and should silently return on line 52 in this case. Or is there a reason to
terminate the process?

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/callb...
ppapi/shared_impl/callback_tracker.cc:71: PP_Resource resource_id =
tracked_callback->resource_id();
A related question is why not DCHECK_NE(resource_id, 0) here? tracked_callback
might not be associated with a resource.

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/callb...
File ppapi/shared_impl/callback_tracker.h (right):

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/callb...
ppapi/shared_impl/callback_tracker.h:59: // The callbacks are thread-safe
ref-counted, so objects which live on different
nit: Slightly clearer to say just "Callbacks are thread-safe ..." since this is
a property of TrackedCallback, not "the callbacks" in the tracker.

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/proxy...
File ppapi/shared_impl/proxy_lock.h (right):

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/proxy...
ppapi/shared_impl/proxy_lock.h:238: ProxyLock::AssertAcquired();
looks like you missed this one.

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/track...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/track...
ppapi/shared_impl/tracked_callback.cc:192: scoped_refptr<TrackedCallback>
thiz(this);
Isn't it possible that when the while scope is exited, this could be deleted?
Shouldn't we add the reference outside the loop?

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/track...
ppapi/shared_impl/tracked_callback.cc:197: // Note that the condvar releases the
ProxyLock during Wait and re-acquires
nit: reacquire is a word, even if it is weird-looking.

[dmichael (off chromium), 2015-03-31 19:38:05]

Thanks!

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/callb...
File ppapi/shared_impl/callback_tracker.cc (right):

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/callb...
ppapi/shared_impl/callback_tracker.cc:45: CHECK_NE(resource_id, 0);
On 2015/03/27 23:46:20, bbudge wrote:
> I think I asked already, but why not just DCHECK here? It's an internal error
> and should silently return on line 52 in this case. Or is there a reason to
> terminate the process?
Sure, I made it a DCHECK and added a small comment of explanation.

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/callb...
ppapi/shared_impl/callback_tracker.cc:71: PP_Resource resource_id =
tracked_callback->resource_id();
On 2015/03/27 23:46:20, bbudge wrote:
> A related question is why not DCHECK_NE(resource_id, 0) here? tracked_callback
> might not be associated with a resource.

Done.

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/callb...
File ppapi/shared_impl/callback_tracker.h (right):

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/callb...
ppapi/shared_impl/callback_tracker.h:59: // The callbacks are thread-safe
ref-counted, so objects which live on different
On 2015/03/27 23:46:20, bbudge wrote:
> nit: Slightly clearer to say just "Callbacks are thread-safe ..." since this
is
> a property of TrackedCallback, not "the callbacks" in the tracker.

Done.

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/proxy...
File ppapi/shared_impl/proxy_lock.h (right):

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/proxy...
ppapi/shared_impl/proxy_lock.h:238: ProxyLock::AssertAcquired();
On 2015/03/27 23:46:20, bbudge wrote:
> looks like you missed this one.
Nice catch, thanks...  that would have confused somebody some day.

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/track...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/track...
ppapi/shared_impl/tracked_callback.cc:192: scoped_refptr<TrackedCallback>
thiz(this);
On 2015/03/27 23:46:20, bbudge wrote:
> Isn't it possible that when the while scope is exited, this could be deleted?
> Shouldn't we add the reference outside the loop?
Good catch, we need to still have a reference so it's safe to read |completed_|

https://codereview.chromium.org/923263003/diff/240001/ppapi/shared_impl/track...
ppapi/shared_impl/tracked_callback.cc:197: // Note that the condvar releases the
ProxyLock during Wait and re-acquires
On 2015/03/27 23:46:20, bbudge wrote:
> nit: reacquire is a word, even if it is weird-looking.

Done.

[bbudge, 2015-03-31 21:18:47]

LGTM
Good luck!

https://codereview.chromium.org/923263003/diff/260001/ppapi/proxy/tracked_cal...
File ppapi/proxy/tracked_callback_unittest.cc (right):

https://codereview.chromium.org/923263003/diff/260001/ppapi/proxy/tracked_cal...
ppapi/proxy/tracked_callback_unittest.cc:118: void WaitUntilCompleted() {
callback_did_run_event_.Wait(); }
nit: Even though it's a one-liner, it would be nice to distinguish it from the
getters below by expanding it to 3 lines.

https://codereview.chromium.org/923263003/diff/260001/ppapi/proxy/tracked_cal...
ppapi/proxy/tracked_callback_unittest.cc:123: private:
nit: space above private:

https://codereview.chromium.org/923263003/diff/260001/ppapi/shared_impl/track...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/923263003/diff/260001/ppapi/shared_impl/track...
ppapi/shared_impl/tracked_callback.cc:195: lock_.Release();
You could use base::AutoUnlock instead of the explicit Release / Acquire calls.

[dmichael (off chromium), 2015-03-31 21:57:27]

Thanks again!

https://codereview.chromium.org/923263003/diff/260001/ppapi/proxy/tracked_cal...
File ppapi/proxy/tracked_callback_unittest.cc (right):

https://codereview.chromium.org/923263003/diff/260001/ppapi/proxy/tracked_cal...
ppapi/proxy/tracked_callback_unittest.cc:118: void WaitUntilCompleted() {
callback_did_run_event_.Wait(); }
On 2015/03/31 21:18:47, bbudge wrote:
> nit: Even though it's a one-liner, it would be nice to distinguish it from the
> getters below by expanding it to 3 lines.

Done.

https://codereview.chromium.org/923263003/diff/260001/ppapi/proxy/tracked_cal...
ppapi/proxy/tracked_callback_unittest.cc:123: private:
On 2015/03/31 21:18:47, bbudge wrote:
> nit: space above private:

Done.

https://codereview.chromium.org/923263003/diff/260001/ppapi/shared_impl/track...
File ppapi/shared_impl/tracked_callback.cc (right):

https://codereview.chromium.org/923263003/diff/260001/ppapi/shared_impl/track...
ppapi/shared_impl/tracked_callback.cc:195: lock_.Release();
On 2015/03/31 21:18:47, bbudge wrote:
> You could use base::AutoUnlock instead of the explicit Release / Acquire
calls.
As discussed over IM, I'll leave this as-is because I think it's a little more
self-documenting about lock order (going along with the comment just below)

[dmichael (off chromium), 2015-03-31 21:57:32]

The CQ bit was checked by dmichael@chromium.org

[dmichael (off chromium), 2015-03-31 21:57:32]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/923263003/#ps280001
(title: "review comments")

[commit-bot: I haz the power, 2015-03-31 21:58:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/923263003/280001

[commit-bot: I haz the power, 2015-03-31 22:48:51]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2015-03-31 22:50:01]

Patchset 14 (id:??) landed as
https://crrev.com/342e88099292432d7b33988a82609ea906f75863
Cr-Commit-Position: refs/heads/master@{#323133}