Bug 1442523: mscom::Interceptor: Don't dispatch QI calls to the main thread while creating a marshaler. r?aklotz draft
authorJames Teh <jteh@mozilla.com>
Tue, 06 Mar 2018 17:08:15 +1000
changeset 763658 487a6ce40b06ff5bfb93a8f641fcf00713487f6a
parent 763311 0ef34a9ec4fbfccd03ee0cfb26b182c03e28133a
push id101511
push userbmo:jteh@mozilla.com
push dateTue, 06 Mar 2018 11:57:30 +0000
reviewersaklotz
bugs1442523
milestone60.0a1
Bug 1442523: mscom::Interceptor: Don't dispatch QI calls to the main thread while creating a marshaler. r?aklotz COM queries for special interfaces such as IFastRundown when creating a marshaler. We don't want these being dispatched to the main thread, since this would cause a deadlock on mStdMarshalMutex if the main thread is also querying for IMarshal. MozReview-Commit-ID: EQcN8Zhewjh
ipc/mscom/Interceptor.cpp
ipc/mscom/Interceptor.h
--- a/ipc/mscom/Interceptor.cpp
+++ b/ipc/mscom/Interceptor.cpp
@@ -226,16 +226,18 @@ private:
 
 static detail::LiveSet&
 GetLiveSet()
 {
   static detail::LiveSet sLiveSet;
   return sLiveSet;
 }
 
+MOZ_THREAD_LOCAL(bool) Interceptor::tlsCreatingStdMarshal;
+
 /* static */ HRESULT
 Interceptor::Create(STAUniquePtr<IUnknown> aTarget, IInterceptorSink* aSink,
                     REFIID aInitialIid, void** aOutInterface)
 {
   MOZ_ASSERT(aOutInterface && aTarget && aSink);
   if (!aOutInterface) {
     return E_INVALIDARG;
   }
@@ -266,16 +268,19 @@ Interceptor::Create(STAUniquePtr<IUnknow
 
 Interceptor::Interceptor(IInterceptorSink* aSink)
   : WeakReferenceSupport(WeakReferenceSupport::Flags::eDestroyOnMainThread)
   , mEventSink(aSink)
   , mInterceptorMapMutex("mozilla::mscom::Interceptor::mInterceptorMapMutex")
   , mStdMarshalMutex("mozilla::mscom::Interceptor::mStdMarshalMutex")
   , mStdMarshal(nullptr)
 {
+  static const bool kHasTls = tlsCreatingStdMarshal.init();
+  MOZ_ASSERT(kHasTls);
+
   MOZ_ASSERT(aSink);
   RefPtr<IWeakReference> weakRef;
   if (SUCCEEDED(GetWeakReference(getter_AddRefs(weakRef)))) {
     aSink->SetInterceptor(weakRef);
   }
 }
 
 Interceptor::~Interceptor()
@@ -714,16 +719,27 @@ Interceptor::GetInterceptorForIID(REFIID
 
 HRESULT
 Interceptor::QueryInterfaceTarget(REFIID aIid, void** aOutput,
                                   TimeDuration* aOutDuration)
 {
   // NB: This QI needs to run on the main thread because the target object
   // is probably Gecko code that is not thread-safe. Note that this main
   // thread invocation is *synchronous*.
+  if (!NS_IsMainThread() && tlsCreatingStdMarshal.get()) {
+    mStdMarshalMutex.AssertCurrentThreadOwns();
+    // COM queries for special interfaces such as IFastRundown when creating a
+    // marshaler. We don't want these being dispatched to the main thread,
+    // since this would cause a deadlock on mStdMarshalMutex if the main
+    // thread is also querying for IMarshal. If we do need to respond to these
+    // special interfaces, this should be done before this point; e.g. in
+    // Interceptor::QueryInterface like we do for INoMarshal.
+    return E_NOINTERFACE;
+  }
+
   MainThreadInvoker invoker;
   HRESULT hr;
   auto runOnMainThread = [&]() -> void {
     MOZ_ASSERT(NS_IsMainThread());
     hr = mTarget->QueryInterface(aIid, aOutput);
   };
   if (!invoker.Invoke(NS_NewRunnableFunction("Interceptor::QueryInterface", runOnMainThread))) {
     return E_FAIL;
@@ -770,23 +786,26 @@ Interceptor::WeakRefQueryInterface(REFII
   }
 
   if (aIid == IID_IMarshal) {
     MutexAutoLock lock(mStdMarshalMutex);
 
     HRESULT hr;
 
     if (!mStdMarshalUnk) {
+      MOZ_ASSERT(!tlsCreatingStdMarshal.get());
+      tlsCreatingStdMarshal.set(true);
       if (XRE_IsContentProcess()) {
         hr = FastMarshaler::Create(static_cast<IWeakReferenceSource*>(this),
                                    getter_AddRefs(mStdMarshalUnk));
       } else {
         hr = ::CoGetStdMarshalEx(static_cast<IWeakReferenceSource*>(this),
                                  SMEXF_SERVER, getter_AddRefs(mStdMarshalUnk));
       }
+      tlsCreatingStdMarshal.set(false);
 
       ENSURE_HR_SUCCEEDED(hr);
     }
 
     if (!mStdMarshal) {
       hr = mStdMarshalUnk->QueryInterface(IID_IMarshal, (void**)&mStdMarshal);
       ENSURE_HR_SUCCEEDED(hr);
 
--- a/ipc/mscom/Interceptor.h
+++ b/ipc/mscom/Interceptor.h
@@ -140,16 +140,17 @@ private:
   InterceptorTargetPtr<IUnknown>  mTarget;
   RefPtr<IInterceptorSink>  mEventSink;
   mozilla::Mutex            mInterceptorMapMutex; // Guards mInterceptorMap
   // Using a nsTArray since the # of interfaces is not going to be very high
   nsTArray<MapEntry>        mInterceptorMap;
   mozilla::Mutex            mStdMarshalMutex; // Guards mStdMarshalUnk and mStdMarshal
   RefPtr<IUnknown>          mStdMarshalUnk;
   IMarshal*                 mStdMarshal; // WEAK
+  static MOZ_THREAD_LOCAL(bool) tlsCreatingStdMarshal;
 };
 
 template <typename InterfaceT>
 inline HRESULT
 CreateInterceptor(STAUniquePtr<InterfaceT> aTargetInterface,
                   IInterceptorSink* aEventSink,
                   InterfaceT** aOutInterface)
 {