Bug 1472137 - Prevent mutex reentry in mscom::Interceptor::Create if GetInitialInterceptorForIID fails. r=aklotz, a=RyanVM FIREFOX_61_0_1_BUILD1 FIREFOX_61_0_1_RELEASE
authorJames Teh <jteh@mozilla.com>
Mon, 02 Jul 2018 15:17:12 +1000
changeset 473801 7d280b7e277b
parent 473800 f7ffdfcae3e7
child 473802 eaaba1aa3702
push id1740
push userryanvm@gmail.com
push dateWed, 04 Jul 2018 00:31:37 +0000
treeherdermozilla-release@7d280b7e277b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaklotz, RyanVM
bugs1472137, 1364624
milestone61.0.1
Bug 1472137 - Prevent mutex reentry in mscom::Interceptor::Create if GetInitialInterceptorForIID fails. r=aklotz, a=RyanVM If GetInitialInterceptorForIID fails, the live set lock is not released in most cases, but the newly created Interceptor will be destroyed. The Interceptor's destructor tries to acquire the live set lock again, but that causes a deadlock, since reentry is no longer allowed for a mutex after bug 1364624. GetInitialInterceptorForIID now ensures the live set lock is always released on failure, thus preventing the deadlock. MozReview-Commit-ID: z0Q7JLnJXQ
accessible/ipc/win/HandlerProvider.cpp
ipc/mscom/Interceptor.cpp
ipc/mscom/Utils.h
--- a/accessible/ipc/win/HandlerProvider.cpp
+++ b/accessible/ipc/win/HandlerProvider.cpp
@@ -159,43 +159,16 @@ HandlerProvider::GetHandlerPayloadSize(N
     *aOutPayloadSize = mscom::StructToStream::GetEmptySize();
     return S_OK;
   }
 
   *aOutPayloadSize = mSerializer->GetSize();
   return S_OK;
 }
 
-template <typename CondFnT, typename ExeFnT>
-class MOZ_RAII ExecuteWhen final
-{
-public:
-  ExecuteWhen(CondFnT& aCondFn, ExeFnT& aExeFn)
-    : mCondFn(aCondFn)
-    , mExeFn(aExeFn)
-  {
-  }
-
-  ~ExecuteWhen()
-  {
-    if (mCondFn()) {
-      mExeFn();
-    }
-  }
-
-  ExecuteWhen(const ExecuteWhen&) = delete;
-  ExecuteWhen(ExecuteWhen&&) = delete;
-  ExecuteWhen& operator=(const ExecuteWhen&) = delete;
-  ExecuteWhen& operator=(ExecuteWhen&&) = delete;
-
-private:
-  CondFnT&  mCondFn;
-  ExeFnT&   mExeFn;
-};
-
 void
 HandlerProvider::BuildStaticIA2Data(
   NotNull<mscom::IInterceptor*> aInterceptor,
   StaticIA2Data* aOutData)
 {
   MOZ_ASSERT(aOutData);
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(mTargetUnk);
@@ -273,17 +246,18 @@ HandlerProvider::BuildDynamicIA2Data(Dyn
   auto hasFailed = [&hr]() -> bool {
     return FAILED(hr);
   };
 
   auto cleanup = [this, aOutIA2Data]() -> void {
     CleanupDynamicIA2Data(*aOutIA2Data);
   };
 
-  ExecuteWhen<decltype(hasFailed), decltype(cleanup)> onFail(hasFailed, cleanup);
+  mscom::ExecuteWhen<decltype(hasFailed), decltype(cleanup)>
+    onFail(hasFailed, cleanup);
 
   const VARIANT kChildIdSelf = {VT_I4};
   VARIANT varVal;
 
   hr = target->accLocation(&aOutIA2Data->mLeft, &aOutIA2Data->mTop,
                            &aOutIA2Data->mWidth, &aOutIA2Data->mHeight,
                            kChildIdSelf);
   if (FAILED(hr)) {
--- a/ipc/mscom/Interceptor.cpp
+++ b/ipc/mscom/Interceptor.cpp
@@ -537,34 +537,47 @@ Interceptor::GetInitialInterceptorForIID
                                          REFIID aTargetIid,
                                          STAUniquePtr<IUnknown> aTarget,
                                          void** aOutInterceptor)
 {
   MOZ_ASSERT(aOutInterceptor);
   MOZ_ASSERT(aTargetIid != IID_IMarshal);
   MOZ_ASSERT(!IsProxy(aTarget.get()));
 
+  HRESULT hr = E_UNEXPECTED;
+
+  auto hasFailed = [&hr]() -> bool {
+    return FAILED(hr);
+  };
+
+  auto cleanup = [&aLiveSetLock]() -> void {
+    aLiveSetLock.Unlock();
+  };
+
+  ExecuteWhen<decltype(hasFailed), decltype(cleanup)>
+    onFail(hasFailed, cleanup);
+
   if (aTargetIid == IID_IUnknown) {
     // We must lock mInterceptorMapMutex so that nothing can race with us once
     // we have been published to the live set.
     MutexAutoLock lock(mInterceptorMapMutex);
 
-    HRESULT hr = PublishTarget(aLiveSetLock, nullptr, aTargetIid, Move(aTarget));
+    hr = PublishTarget(aLiveSetLock, nullptr, aTargetIid, Move(aTarget));
     ENSURE_HR_SUCCEEDED(hr);
 
     hr = QueryInterface(aTargetIid, aOutInterceptor);
     ENSURE_HR_SUCCEEDED(hr);
     return hr;
   }
 
   // Raise the refcount for stabilization purposes during aggregation
   WeakReferenceSupport::StabilizeRefCount stabilizer(*this);
 
   RefPtr<IUnknown> unkInterceptor;
-  HRESULT hr = CreateInterceptor(aTargetIid,
+  hr = CreateInterceptor(aTargetIid,
                                  static_cast<WeakReferenceSupport*>(this),
                                  getter_AddRefs(unkInterceptor));
   ENSURE_HR_SUCCEEDED(hr);
 
   RefPtr<ICallInterceptor> interceptor;
   hr = unkInterceptor->QueryInterface(IID_ICallInterceptor,
                                       getter_AddRefs(interceptor));
   ENSURE_HR_SUCCEEDED(hr);
--- a/ipc/mscom/Utils.h
+++ b/ipc/mscom/Utils.h
@@ -6,16 +6,17 @@
 
 #ifndef mozilla_mscom_Utils_h
 #define mozilla_mscom_Utils_h
 
 #if defined(MOZILLA_INTERNAL_API)
 #include "nsString.h"
 #endif // defined(MOZILLA_INTERNAL_API)
 
+#include "mozilla/Attributes.h"
 #include <guiddef.h>
 
 struct IStream;
 struct IUnknown;
 
 namespace mozilla {
 namespace mscom {
 
@@ -58,13 +59,47 @@ bool IsVtableIndexFromParentInterface(RE
 bool IsCallerExternalProcess();
 
 bool IsInterfaceEqualToOrInheritedFrom(REFIID aInterface, REFIID aFrom,
                                        unsigned long aVtableIndexHint);
 #endif // defined(MOZILLA_INTERNAL_API)
 
 #endif // defined(ACCESSIBILITY)
 
+/**
+ * Execute cleanup code when going out of scope if a condition is met.
+ * This is useful when, for example, particular cleanup needs to be performed
+ * whenever a call returns a failure HRESULT.
+ * Both the condition and cleanup code are provided as functions (usually
+ * lambdas).
+ */
+template <typename CondFnT, typename ExeFnT>
+class MOZ_RAII ExecuteWhen final
+{
+public:
+  ExecuteWhen(CondFnT& aCondFn, ExeFnT& aExeFn)
+    : mCondFn(aCondFn)
+    , mExeFn(aExeFn)
+  {
+  }
+
+  ~ExecuteWhen()
+  {
+    if (mCondFn()) {
+      mExeFn();
+    }
+  }
+
+  ExecuteWhen(const ExecuteWhen&) = delete;
+  ExecuteWhen(ExecuteWhen&&) = delete;
+  ExecuteWhen& operator=(const ExecuteWhen&) = delete;
+  ExecuteWhen& operator=(ExecuteWhen&&) = delete;
+
+private:
+  CondFnT&  mCondFn;
+  ExeFnT&   mExeFn;
+};
+
 } // namespace mscom
 } // namespace mozilla
 
 #endif // mozilla_mscom_Utils_h