PPAPI: Make TrackedCallback more threadsafe

ppapi/shared_impl/callback_tracker.h

Index: ppapi/shared_impl/callback_tracker.h
diff --git a/ppapi/shared_impl/callback_tracker.h b/ppapi/shared_impl/callback_tracker.h
index f19f27669edaf08b1d4adeba0300502befb650c9..d786aa3fe614920d73dea25c76f748b300992947 100644
--- a/ppapi/shared_impl/callback_tracker.h
+++ b/ppapi/shared_impl/callback_tracker.h
@@ -10,6 +10,7 @@
 
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
 #include "ppapi/c/pp_resource.h"
 #include "ppapi/shared_impl/ppapi_shared_export.h"
 
@@ -19,15 +20,17 @@ class TrackedCallback;
 
 // Pepper callbacks have the following semantics (unless otherwise specified;
 // in particular, the below apply to all completion callbacks):
-//  - Callbacks are always run on the main thread.
-//  - Callbacks are always called from the main message loop. In particular,
-//    calling into Pepper will not result in the plugin being re-entered via a
-//    synchronously-run callback.
+//  - Callbacks are always run on the thread where the plugin called a Pepper
+//    function providing that callback.
+//  - Callbacks are always called from the message loop of the thread. In
+//    particular, calling into Pepper will not result in the plugin being
+//    re-entered via a synchronously-run callback.
 //  - Each callback will be executed (a.k.a. completed) exactly once.
 //  - Each callback may be *aborted*, which means that it will be executed with
-//    result |PP_ERROR_ABORTED| (in the case of completion callbacks).
+//    result |PP_ERROR_ABORTED| (in the case of completion callbacks). The
+//    ABORT counts as the callback's one completion.
 //  - Before |PPP_ShutdownModule()| is called, every pending callback (for that
-//    module) will be aborted.
+//    instance) will be aborted.

[bbudge, 2015/02/25 23:51:31]

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?

[dmichael (off chromium), 2015/02/27 22:04:40]

Good point. I replaced module with instance...  is that roughly what you wanted?

 //  - Callbacks are usually associated to a resource, whose "deletion" provides
 //    a "cancellation" (or abort) mechanism -- see below.
 //  - When a plugin releases its last reference to resource, all callbacks
@@ -42,18 +45,20 @@ class TrackedCallback;
 //    progress, or may be completed (successfully or not). In fact, the
 //    operation may still proceed after the callback has been aborted.
 //  - Any output data buffers provided to Pepper are associated with a resource.
-//    Once that resource is released, no subsequent writes to those buffers. (If
-//    background threads are set up to write into such buffers, the final
-//    release operation should not return into the plugin until it can
+//    Once that resource is released, no subsequent writes to those buffers
+//    will occur. (If background threads are set up to write into such buffers,
+//    the final release operation should not return into the plugin until it can
 //    guaranteed that those threads will no longer write into the buffers.)
 //
 // Thread-safety notes:
-// Currently, everything should happen on the main thread. The objects are
-// thread-safe ref-counted, so objects which live on different threads may keep
-// references. Releasing a reference to |TrackedCallback| on a different thread
-// (possibly causing destruction) is also okay. Otherwise, all methods should be
-// called only from the main thread.
-
+// |CallbackTracker| uses a lock to protect its dictionary of callbacks. This
+// is primarily to allow the safe removal of callbacks from any thread without
+// requiring that the |ProxyLock| is held. Methods that may invoke a callback
+// need to have the |ProxyLock| (and those methods assert that it's acquired).
+// The callbacks are thread-safe ref-counted, so objects which live on different
+// threads may keep references. Releasing a reference to |TrackedCallback| on a
+// different thread (possibly causing destruction) is also okay.
+//
 // |CallbackTracker| tracks pending Pepper callbacks for a single module. It
 // also tracks, for each resource ID, which callbacks are pending. When a
 // callback is (just about to be) completed, it is removed from the tracker. We
@@ -67,10 +72,13 @@ class PPAPI_SHARED_EXPORT CallbackTracker
   CallbackTracker();
 
   // Abort all callbacks (synchronously).
+  // ppapi::ProxyLock must already be held by the caller.
   void AbortAll();
 
   // Abort all callbacks associated to the given resource ID (which must be
   // valid, i.e., nonzero) by posting a task (or tasks).
+  // FIXME: Should this also have the ProxyLock requirement? Probably not
+  //        necessary.

[bbudge, 2015/02/25 23:51:31]

Looking at TrackedCallback::PostAbort / PostRun, it seems the callback could be
run (blocking callback signalled) rather than posted as a task.

[dmichael (off chromium), 2015/02/27 22:04:40]

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.

[bbudge, 2015/03/05 01:29:34]

OK, that is better.

   void PostAbortForResource(PP_Resource resource_id);
 
  private:
@@ -92,6 +100,8 @@ class PPAPI_SHARED_EXPORT CallbackTracker
   typedef std::map<PP_Resource, CallbackSet> CallbackSetMap;
   CallbackSetMap pending_callbacks_;
 
+  base::Lock lock_;
+
   DISALLOW_COPY_AND_ASSIGN(CallbackTracker);
 };