PPAPI: Make TrackedCallback more threadsafe

ppapi/shared_impl/callback_tracker.cc

Index: ppapi/shared_impl/callback_tracker.cc
diff --git a/ppapi/shared_impl/callback_tracker.cc b/ppapi/shared_impl/callback_tracker.cc
index feefb0e117d70aa862553838656da09763927824..de519a13f71be385d41a8f0cfd940fe3084402f2 100644
--- a/ppapi/shared_impl/callback_tracker.cc
+++ b/ppapi/shared_impl/callback_tracker.cc
@@ -20,9 +20,14 @@ namespace ppapi {
 CallbackTracker::CallbackTracker() {}
 
 void CallbackTracker::AbortAll() {
-  // Iterate over a copy since |Abort()| calls |Remove()| (indirectly).
-  // TODO(viettrungluu): This obviously isn't so efficient.
-  CallbackSetMap pending_callbacks_copy = pending_callbacks_;
+  // Iterate over a copy:
+  // 1) because |Abort()| calls |Remove()| (indirectly).
+  // 2) So we can drop the lock before calling in to TrackedCallback.
+  CallbackSetMap pending_callbacks_copy;
+  {
+    base::AutoLock acquire(lock_);
+    pending_callbacks_copy = pending_callbacks_;
+  }

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

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.

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

Done, thanks.

   for (CallbackSetMap::iterator it1 = pending_callbacks_copy.begin();
        it1 != pending_callbacks_copy.end();
        ++it1) {
@@ -35,15 +40,20 @@ void CallbackTracker::AbortAll() {
 }
 
 void CallbackTracker::PostAbortForResource(PP_Resource resource_id) {
-  CHECK(resource_id != 0);
-  CallbackSetMap::iterator it1 = pending_callbacks_.find(resource_id);
-  if (it1 == pending_callbacks_.end())
-    return;
-  for (CallbackSet::iterator it2 = it1->second.begin();
-       it2 != it1->second.end();
-       ++it2) {
-    // Post the abort.
-    (*it2)->PostAbort();
+  CHECK_NE(resource_id, 0);
+  CallbackSet callbacks_for_resource;
+  {
+    base::AutoLock acquire(lock_);
+    CallbackSetMap::iterator iter = pending_callbacks_.find(resource_id);
+    // The resource may have no callbacks, so it won't be found, and we're done.
+    if (iter == pending_callbacks_.end())
+      return;
+    // Copy the set so we can drop the lock before calling in to
+    // TrackedCallback.
+    callbacks_for_resource = iter->second;
+  }
+  for (const auto& iter : callbacks_for_resource) {
+    iter->PostAbort();
   }
 }
 
@@ -54,6 +64,7 @@ CallbackTracker::~CallbackTracker() {
 
 void CallbackTracker::Add(
     const scoped_refptr<TrackedCallback>& tracked_callback) {
+  base::AutoLock acquire(lock_);
   PP_Resource resource_id = tracked_callback->resource_id();
   DCHECK(pending_callbacks_[resource_id].find(tracked_callback) ==
          pending_callbacks_[resource_id].end());
@@ -62,6 +73,7 @@ void CallbackTracker::Add(
 
 void CallbackTracker::Remove(
     const scoped_refptr<TrackedCallback>& tracked_callback) {
+  base::AutoLock acquire(lock_);
   CallbackSetMap::iterator map_it =
       pending_callbacks_.find(tracked_callback->resource_id());
   DCHECK(map_it != pending_callbacks_.end());