Bug 1472137 - Prevent mutex reentry in mscom::Interceptor::Create if GetInitialInterceptorForIID fails. r=aklotz, a=RyanVM
authorJames Teh <jteh@mozilla.com>
Mon, 02 Jul 2018 15:17:12 +1000
changeset 477810 505d8765ce63f12bbeca8e7be96e52033a137687
parent 477809 6ce7f5a3eaf3e8935f8d6b9c60b3a3fd8558828b
child 477811 1e5070713a74afa001c8316f4d2a86a847f0945d
push id9432
push userryanvm@gmail.com
push dateWed, 04 Jul 2018 00:21:37 +0000
treeherdermozilla-beta@505d8765ce63 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaklotz, RyanVM
bugs1472137, 1364624
milestone62.0
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, std::move(aTarget));
+    hr = PublishTarget(aLiveSetLock, nullptr, aTargetIid, std::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