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..bafd818fa6ba06dd5a9132b00c5727a2903547fe 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).
-//  - Before |PPP_ShutdownModule()| is called, every pending callback (for that
-//    module) will be aborted.
+//    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 every
+//    instance of that module) 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,25 +45,29 @@ 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
-//    guaranteed that those threads will no longer write into the buffers.)
+//    Once that resource is released, no subsequent writes to those buffers
+//    will occur. When operations occur on background threads, writing to the
+//    plugin's data buffers should be delayed to happen on the callback's thread
+//    to ensure that we don't write to the buffers if the callback has been
+//    aborted (see TrackedCallback::set_completion_task()).
 //
 // 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
-// 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
-// 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.
+// |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).
+// |TrackedCallback| is 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. Just before
+// a callback is completed, it is removed from the tracker. We use
+// |CallbackTracker| for two things: (1) to ensure that all callbacks are
+// properly aborted before instance shutdown, and (2) to ensure that all
+// callbacks associated with a given resource are aborted when a plugin instance
+// releases its last reference to that resource.
 class PPAPI_SHARED_EXPORT CallbackTracker
     : public base::RefCountedThreadSafe<CallbackTracker> {
  public:
@@ -92,6 +99,11 @@ class PPAPI_SHARED_EXPORT CallbackTracker
   typedef std::map<PP_Resource, CallbackSet> CallbackSetMap;
   CallbackSetMap pending_callbacks_;
 
+  // Used to ensure we don't add any callbacks after AbortAll.
+  bool abort_all_called_;
+
+  base::Lock lock_;
+
   DISALLOW_COPY_AND_ASSIGN(CallbackTracker);
 };