PPAPI: Make TrackedCallback more threadsafe

ppapi/shared_impl/callback_tracker.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ppapi/shared_impl/callback_tracker.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "ppapi/c/pp_completion_callback.h"
+#include "ppapi/shared_impl/proxy_lock.h"
 #include "ppapi/shared_impl/tracked_callback.h"
 
 namespace ppapi {
 
 // CallbackTracker -------------------------------------------------------------
 
 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_;
+  // We must have the ProxyLock whenever this method is called; this ensures
+  // that no callbacks are added while we're aborting them all (see Add).
+  ProxyLock::AssertAcquired();
+
+  // 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_;
+  }
   for (CallbackSetMap::iterator it1 = pending_callbacks_copy.begin();
        it1 != pending_callbacks_copy.end();
        ++it1) {
     for (CallbackSet::iterator it2 = it1->second.begin();
          it2 != it1->second.end();
          ++it2) {
       (*it2)->Abort();
     }
   }
 }
 
 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;

[bbudge, 2015/03/19 00:21:10]

It seems like we need the proxy lock here for the same reason as AbortAll.

[dmichael (off chromium), 2015/03/19 17:50:58]

Hmm. The more I think about it, the more I think the lock doesn't do anything
for us. We're basically talking about "AbortAll" getting called around the same
time as Add on another thread. Forgetting for the moment that that can't happen
due to both holding the ProxyLock, what's the outcome?

Also let's ignore that no Adds should happen after AbortAll just based on the
fact that AbortAll happens at instance destruction.

Ignoring those, imagine that we don't assume the lock is held, and a call to
AbortAll beats the Add. We end up aborting all the callbacks that existed at the
time AbortAll was called. Then, another callback is added that won't be aborted.

Imagine instead we guarantee that the ProxyLock is held. Can't we still get in
the same situation where Add gets the ProxyLock just after AbortAll is done? It
seems like we haven't really gained anything.

If we want to be bullet-proof, maybe we're better off having a "aborted_all_"
kind of flag that we check in Add? That, or just don't worry about it.

+  {
+    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();
   }
 }
 
 CallbackTracker::~CallbackTracker() {
   // All callbacks must be aborted before destruction.
   CHECK_EQ(0u, pending_callbacks_.size());
 }
 
 void CallbackTracker::Add(
     const scoped_refptr<TrackedCallback>& tracked_callback) {
+  // We must have the ProxyLock whenever this method is called; this ensures
+  // that no callbacks are added while we're aborting them all (see AbortAll).

[bbudge, 2015/03/19 00:21:10]

This comment doesn't make sense here. Do we really need to hold the proxy lock?

[dmichael (off chromium), 2015/03/19 17:50:58]

If we don't have it, having the proxy lock in AbortAll wouldn't be meaningful,
would it?

Anyway, I think I convinced myself that the lock doesn't accomplish anything
anyway. WDYT?

[bbudge, 2015/03/27 23:46:20]

OK, that makes sense.

+  ProxyLock::AssertAcquired();
+
+  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());
   pending_callbacks_[resource_id].insert(tracked_callback);
 }
 
 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());
   CallbackSet::iterator it = map_it->second.find(tracked_callback);
   DCHECK(it != map_it->second.end());
   map_it->second.erase(it);
 
   // If there are no pending callbacks left for this ID, get rid of the entry.
   if (map_it->second.empty())
     pending_callbacks_.erase(map_it);
 }
 
 }  // namespace ppapi