Bug 1450708 - Ref-count the plugin FunctionBroker mutex. r=bobowen
authorDavid Parks <dparks@mozilla.com>
Mon, 23 Apr 2018 13:48:06 -0700
changeset 471575 1e02fd7fa20cca193353c033b8e4cfb2027c5e0e
parent 471574 ee7a5b7274d8787ccfb3e073b4c10e5c9c0917c3
child 471576 85a77aec89fbf269c6b39fcb037233359665656e
push id1728
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:12:27 +0000
treeherdermozilla-release@c296fde26f5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbobowen
bugs1450708
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1450708 - Ref-count the plugin FunctionBroker mutex. r=bobowen Instead of contending with the idiosyncracies of the platform implementations of condition variables, which have been leading to strange crashes, we hold this mutex as a ref-counted object and avoid complex object lifetime reasoning.
dom/plugins/ipc/FunctionBroker.h
--- a/dom/plugins/ipc/FunctionBroker.h
+++ b/dom/plugins/ipc/FunctionBroker.h
@@ -1191,16 +1191,32 @@ struct ResponseHandler<functionId, Resul
   typedef Marshaler<CLIENT, SelfType> RspUnmarshaler;
 
   // Fixed parameters are not used in the response phase.
   template <int tupleIndex, typename VarParam>
   static void CopyFixedParam(VarParam& aParam) {}
 };
 
 /**
+ * Reference-counted monitor, used to synchronize communication between a
+ * thread using a brokered API and the FunctionDispatch thread.
+ */
+class FDMonitor : public Monitor
+{
+public:
+  FDMonitor() : Monitor("FunctionDispatchThread lock")
+  {}
+
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(FDMonitor)
+
+private:
+  ~FDMonitor() {}
+};
+
+/**
  * Data for hooking a function that we automatically broker in a remote
  * process.
  */
 template <FunctionHookId functionId, typename FunctionType>
 class FunctionBroker;
 
 template <FunctionHookId functionId, typename ResultType, typename ... ParamTypes>
 class FunctionBroker<functionId, ResultType HOOK_CALL (ParamTypes...)> :
@@ -1266,33 +1282,32 @@ protected:
     return BrokerCallServer(aClientId, aInTuple, aOutTuple,
                              std::index_sequence_for<ParamTypes...>{});
   }
 
   bool BrokerCallClient(uint32_t& aWinError, ResultType& aResult, ParamTypes&... aParameters) const;
   bool PostToDispatchThread(uint32_t& aWinError, ResultType& aRet, ParamTypes&... aParameters) const;
 
   static void
-  PostToDispatchHelper(const SelfType* bmhi, Monitor* monitor, bool* notified,
+  PostToDispatchHelper(const SelfType* bmhi, RefPtr<FDMonitor> monitor, bool* notified,
                        bool* ok, uint32_t* winErr, ResultType* r, ParamTypes*... p)
   {
     // Note: p is also non-null... its just hard to assert that.
     MOZ_ASSERT(bmhi && monitor && notified && ok && winErr && r);
     MOZ_ASSERT(*notified == false);
     *ok = bmhi->BrokerCallClient(*winErr, *r, *p...);
 
-    // We need to grab the lock to make sure that Wait() has been
-    // called in PostToDispatchThread.  We need that since we wake it with
-    // Notify().
-    // We also need to keep the lock until _after_ Notify() has been called
-    // since, after we set notified to true, a spurious wakeup could lead
-    // the other thread to wake and proceed -- and one of its first acts would
-    // be to destroy the Monitor.
-    MonitorAutoLock lock(*monitor);
-    *notified = true;
+    {
+      // We need to grab the lock to make sure that Wait() has been
+      // called in PostToDispatchThread.  We need that since we wake it with
+      // Notify().
+      MonitorAutoLock lock(*monitor);
+      *notified = true;
+    }
+
     monitor->Notify();
   };
 
   template<typename ... VarParams>
   ResultType
   RunFunction(FunctionType* aFunction, base::ProcessId aClientId,
                 VarParams&... aParams) const
   {
@@ -1451,29 +1466,29 @@ PostToDispatchThread(uint32_t& aWinError
                      ParamTypes&... aParameters) const
 {
   MOZ_ASSERT(!FunctionBrokerChild::GetInstance()->IsDispatchThread());
   HOOK_LOG(LogLevel::Debug,
            ("Posting broker task '%s' to dispatch thread", FunctionHookInfoType::mFunctionName.Data()));
 
   // Run PostToDispatchHelper on the dispatch thread.  It will notify our
   // waiting monitor when it is done.
-  Monitor monitor("FunctionDispatchThread Lock");
-  MonitorAutoLock lock(monitor);
+  RefPtr<FDMonitor> monitor(new FDMonitor());
+  MonitorAutoLock lock(*monitor);
   bool success = false;
   bool notified = false;
   FunctionBrokerChild::GetInstance()->PostToDispatchThread(
     NewRunnableFunction("FunctionDispatchThreadRunnable", &PostToDispatchHelper,
-                        this, &monitor, &notified, &success, &aWinError, &aRet,
+                        this, monitor, &notified, &success, &aWinError, &aRet,
                         &aParameters...));
 
   // We wait to be notified, testing that notified was actually set to make
   // sure this isn't a spurious wakeup.
   while (!notified) {
-    monitor.Wait();
+    monitor->Wait();
   }
   return success;
 }
 
 void AddBrokeredFunctionHooks(FunctionHookArray& aHooks);
 
 } // namespace plugins
 } // namespace mozilla