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.
 //  - 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.
   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);
 };