Bug 1450708 - Ref-count the plugin FunctionBroker mutex. r=bobowen a=jcristau FIREFOX_60_0_BUILD2 FIREFOX_60_0_RELEASE
authorDavid Parks <dparks@mozilla.com>
Mon, 23 Apr 2018 13:48:06 -0700
changeset 463608 ea4f3168c604994f051644b467aad92723448d12
parent 463607 e26d6d916b943830d2f123e142533e99811ebfd6
child 463609 4855a5d6748066cc7821e243afd3e7b07bcc5966
push id1704
push userjcristau@mozilla.com
push dateThu, 03 May 2018 14:31:29 +0000
treeherdermozilla-release@ea4f3168c604 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbobowen, jcristau
bugs1450708
milestone60.0
Bug 1450708 - Ref-count the plugin FunctionBroker mutex. r=bobowen a=jcristau 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
@@ -1190,16 +1190,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...)> :
@@ -1265,33 +1281,32 @@ protected:
     return BrokerCallServer(aClientId, aInTuple, aOutTuple,
                              typename IndexSequenceFor<ParamTypes...>::Type());
   }
 
   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
   {
@@ -1450,29 +1465,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