PPAPI: Make TrackedCallback more threadsafe

ppapi/shared_impl/tracked_callback.h

 // Copyright (c) 2012 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.
 
 #ifndef PPAPI_SHARED_IMPL_TRACKED_CALLBACK_H_
 #define PPAPI_SHARED_IMPL_TRACKED_CALLBACK_H_
 
 #include <map>
 #include <set>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/synchronization/condition_variable.h"
+#include "base/synchronization/lock.h"
 #include "ppapi/c/pp_completion_callback.h"
 #include "ppapi/c/pp_instance.h"
 #include "ppapi/c/pp_resource.h"
 #include "ppapi/shared_impl/ppapi_shared_export.h"
 #include "ppapi/shared_impl/ppb_message_loop_shared.h"
 
 namespace ppapi {
 
 class CallbackTracker;
 class MessageLoopShared;
@@ skipping 19 matching lines @@
 // The ability to post aborts is needed in many situations to ensure that the
 // plugin is not re-entered into. (Note that posting a task to just run
 // |Abort()| is different and not correct; calling |PostAbort()| additionally
 // guarantees that all subsequent completions will be abortive.)
 //
 // This class is reference counted so that different things can hang on to it,
 // and not worry too much about ensuring Pepper callback semantics. Note that
 // the "owning" |CallbackTracker| will keep a reference until the callback is
 // completed.
 //
-// Subclasses must do several things:
-//  - They must ensure that the callback is executed at most once (by looking at
-//    |completed()| before running the callback).
-//  - They must ensure that the callback is run abortively if it is marked as to
-//    be aborted (by looking at |aborted()| before running the callback).
-//  - They must call |MarkAsCompleted()| immediately before actually running the
-//    callback; see the comment for |MarkAsCompleted()| for a caveat.
+// A note on threading:
+// TrackedCallback is usable on any thread. It is *mostly* only used when
+// ppapi::ProxyLock is held. However, it's necessary that Run() can be called
+// without the ProxyLock. This is used to allow signalling the callback from

[bbudge, 2015/02/25 23:51:31]

It might not be clear to readers what "signalling the callback" means.

+// the IO thread. In particular, blocking callbacks may not have a message loop
+// to which we could post.
 class PPAPI_SHARED_EXPORT TrackedCallback
     : public base::RefCountedThreadSafe<TrackedCallback> {
  public:
   // Create a tracked completion callback and register it with the tracker. The
   // resource pointer is not stored. If |resource| is NULL, this callback will
   // not be added to the callback tracker.
   TrackedCallback(Resource* resource, const PP_CompletionCallback& callback);
 
   // These run the callback in an abortive manner, or post a task to do so (but
   // immediately marking the callback as to be aborted).
   void Abort();
   void PostAbort();
 
   // Run the callback with the given result. If the callback had previously been
   // marked as to be aborted (by |PostAbort()|), |result| will be ignored and
   // the callback will be run with result |PP_ERROR_ABORTED|.
   //
   // Run() will invoke the call immediately, if invoked from the target thread
   // (as determined by target_loop_). If invoked on a different thread, the
   // callback will be scheduled to run later on target_loop_.
   void Run(int32_t result);
+  void AcquireProxyLockAndRun(int32_t result);
   // PostRun is like Run(), except it guarantees that the callback will be run
   // later. In particular, if you invoke PostRun on the same thread on which the
   // callback is targeted to run, it will *not* be run immediately.
   void PostRun(int32_t result);
 
   // A task to perform cleanup or write output parameters before the callback
   // returns a result to the plugin. The |result| parameter has the result so
   // far, e.g. whether the callback has been aborted. If the callback hasn't
   // been aborted the return value of the task will become the callback result.
   // The task is always called on the same thread as the callback to the plugin.
   typedef base::Callback<int32_t(int32_t /* result */)> CompletionTask;
 
   // Sets a task that is run just before calling back into the plugin. This
-  // should only be called once.
+  // should only be called once. Note that the CompletionTask always runs while
+  // holding the ppapi::ProxyLock.
   void set_completion_task(const CompletionTask& completion_task);
 
   // Returns the ID of the resource which "owns" the callback, or 0 if the
   // callback is not associated with any resource.
   PP_Resource resource_id() const { return resource_id_; }
 
   // Returns true if the callback was completed (possibly aborted).
-  bool completed() const { return completed_; }
+  bool completed() const {
+    base::AutoLock acquire(lock_);
+    return completed_;
+  }
 
   // Returns true if the callback was or should be aborted; this will be the
   // case whenever |Abort()| or |PostAbort()| is called before a non-abortive
   // completion.
-  bool aborted() const { return aborted_; }
+  bool aborted() const {
+    base::AutoLock acquire(lock_);
+    return aborted_;
+  }
 
   // Returns true if this is a blocking callback.
-  bool is_blocking() { return !callback_.func; }
+  bool is_blocking() const {
+    // This is set on construction and never changes after that, so there is
+    // no need to lock.
+    return !callback_.func;
+  }
 
-  MessageLoopShared* target_loop() const { return target_loop_.get(); }
+  MessageLoopShared* target_loop() const {
+    // This is set on construction and never changes after that, so there is
+    // no need to lock.
+    return target_loop_.get();
+  }
 
   // Determines if the given callback is pending. A callback is pending if it
   // has not completed and has not been aborted. When receiving a plugin call,
   // use this to detect if |callback| represents an operation in progress. When
   // finishing a plugin call, use this to determine whether to write 'out'
   // params and Run |callback|.
   // NOTE: an aborted callback has not necessarily completed, so a false result
   // doesn't imply that the callback has completed.
   // As a convenience, if |callback| is null, this returns false.
   static bool IsPending(const scoped_refptr<TrackedCallback>& callback);
 
   // Helper to determine if the given callback is scheduled to run on another
   // message loop.
   static bool IsScheduledToRun(const scoped_refptr<TrackedCallback>& callback);
 
- protected:
+ private:
   bool is_required() {
     return (callback_.func &&
             !(callback_.flags & PP_COMPLETIONCALLBACK_FLAG_OPTIONAL));
   }
   bool is_optional() {

[bbudge, 2015/02/25 23:51:31]

This doesn't seem to be used anywhere. Can we eliminate it?

[dmichael (off chromium), 2015/02/27 22:04:40]

Done.

     return (callback_.func &&
             (callback_.flags & PP_COMPLETIONCALLBACK_FLAG_OPTIONAL));
   }
   bool has_null_target_loop() const { return target_loop_.get() == NULL; }
 
- private:
+  // Same as PostRun(), but lock_ must already be held.
+  void PostRunImpl(int32_t result);
+
+  void SignalBlockingCallback(int32_t result);
+
   // TrackedCallback and EnterBase manage dealing with how to invoke callbacks
   // appropriately. Pepper interface implementations and proxies should not have
   // to check the type of callback, block, or mark them complete explicitly.

[bbudge, 2015/02/25 23:51:31]

Some of our resources do check whether callbacks are blocking. Is that what this
comment means by "type"?

[dmichael (off chromium), 2015/02/27 22:04:40]

Right. I tried improving & updating the callback. I think I wrote that comment
before the FileIO optimization you did.

   friend class ppapi::thunk::subtle::EnterBase;
 
   // Block until the associated operation has completed. Returns the result.
   // This must only be called on a non-main thread on a blocking callback.
   int32_t BlockUntilComplete();
 
   // Mark this object as complete and remove it from the tracker. This must only
   // be called once. Note that running this may result in this object being
   // deleted (so keep a reference if it'll still be needed).
   void MarkAsCompleted();
+  void MarkAsCompletedWithLock();
 
   // This class is ref counted.
   friend class base::RefCountedThreadSafe<TrackedCallback>;
-  virtual ~TrackedCallback();
+  ~TrackedCallback();
+
+  mutable base::Lock lock_;
 
   // Flag used by |PostAbort()| and |PostRun()| to check that we don't schedule
   // the callback more than once.
   bool is_scheduled_;
 
   scoped_refptr<CallbackTracker> tracker_;
   PP_Resource resource_id_;
   bool completed_;
   bool aborted_;
   PP_CompletionCallback callback_;
@@ skipping 10 matching lines @@
   // callback. Note that in-process, there is no lock, blocking callbacks are
   // not allowed, and therefore this pointer will be NULL.
   scoped_ptr<base::ConditionVariable> operation_completed_condvar_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(TrackedCallback);
 };
 
 }  // namespace ppapi
 
 #endif  // PPAPI_SHARED_IMPL_TRACKED_CALLBACK_H_