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..629859f0ad94825c335476c17d48b7972edc3a72 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/03/05 01:29:35]

how about s/that instance/every instance of the module/ ?

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

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

 //  - 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,25 +45,27 @@ 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.)

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

s/guaranteed/be guaranteed

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

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

 //
 // 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| tracks pending Pepper callbacks for a single module. It
+// |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 instance. 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

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

nit: wording
s/When a callback is (just about to be) completed,/Just before a callback is
completed/

 // use |CallbackTracker| for two things: (1) to ensure that all callbacks are
-// properly aborted before module shutdown, and (2) to ensure that all callbacks
-// associated to a given resource are aborted when a plugin (module) releases
-// its last reference to that resource.
+// properly aborted before instance shutdown, and (2) to ensure that all
+// callbacks associated to a given resource are aborted when a plugin instance

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

nit: s/to/with

+// releases its last reference to that resource.
 class PPAPI_SHARED_EXPORT CallbackTracker
     : public base::RefCountedThreadSafe<CallbackTracker> {
  public:
@@ -92,6 +97,8 @@ class PPAPI_SHARED_EXPORT CallbackTracker
   typedef std::map<PP_Resource, CallbackSet> CallbackSetMap;
   CallbackSetMap pending_callbacks_;
 
+  base::Lock lock_;
+
   DISALLOW_COPY_AND_ASSIGN(CallbackTracker);
 };