Bug 1339951: Refactor mscom weak reference support and establish lock hierarchy within; r=jimm
☠☠ backed out by 45c6f5ea6b03 ☠ ☠
authorAaron Klotz <aklotz@mozilla.com>
Wed, 15 Feb 2017 15:33:32 -0700
changeset 372685 06cd86f16c70f2c1eae162f95deaa6460efc1cf4
parent 372684 71135678ea64bdedfb38b3ae4ddb2b9e2f4be12f
child 372686 f62e9d267ac5384a827d28d82d7f2f0cc4eeb51e
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimm
bugs1339951
milestone54.0a1
Bug 1339951: Refactor mscom weak reference support and establish lock hierarchy within; r=jimm MozReview-Commit-ID: BJJpSj44alY
ipc/mscom/WeakRef.cpp
ipc/mscom/WeakRef.h
--- a/ipc/mscom/WeakRef.cpp
+++ b/ipc/mscom/WeakRef.cpp
@@ -2,54 +2,117 @@
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #define INITGUID
 #include "mozilla/mscom/WeakRef.h"
 
-#include "mozilla/Assertions.h"
 #include "mozilla/DebugOnly.h"
-#include "mozilla/RefPtr.h"
+#include "mozilla/Mutex.h"
 #include "nsThreadUtils.h"
 #include "nsWindowsHelpers.h"
 
+static void
+InitializeCS(CRITICAL_SECTION& aCS)
+{
+  DWORD flags = 0;
+#if defined(RELEASE_OR_BETA)
+  flags |= CRITICAL_SECTION_NO_DEBUG_INFO;
+#endif
+  InitializeCriticalSectionEx(&aCS, 4000, flags);
+}
+
 namespace mozilla {
 namespace mscom {
 
+namespace detail {
+
+SharedRef::SharedRef(WeakReferenceSupport* aSupport)
+  : mSupport(aSupport)
+{
+  ::InitializeCS(mCS);
+}
+
+SharedRef::~SharedRef()
+{
+  ::DeleteCriticalSection(&mCS);
+}
+
+void
+SharedRef::Lock()
+{
+  ::EnterCriticalSection(&mCS);
+}
+
+void
+SharedRef::Unlock()
+{
+  ::LeaveCriticalSection(&mCS);
+}
+
+HRESULT
+SharedRef::Resolve(REFIID aIid, void** aOutStrongReference)
+{
+  RefPtr<WeakReferenceSupport> strongRef;
+
+  { // Scope for lock
+    AutoCriticalSection lock(&mCS);
+    if (!mSupport) {
+      return E_POINTER;
+    }
+    strongRef = mSupport;
+  }
+
+  return strongRef->QueryInterface(aIid, aOutStrongReference);
+}
+
+void
+SharedRef::Clear()
+{
+  AutoCriticalSection lock(&mCS);
+  MOZ_ASSERT(mSupport);
+  mSupport = nullptr;
+}
+
+} // namespace detail
+
+typedef BaseAutoLock<detail::SharedRef> SharedRefAutoLock;
+typedef BaseAutoUnlock<detail::SharedRef> SharedRefAutoUnlock;
+
 WeakReferenceSupport::WeakReferenceSupport(Flags aFlags)
-  : mRefCnt(1)
+  : mRefCnt(0)
   , mFlags(aFlags)
 {
-  ::InitializeCriticalSectionAndSpinCount(&mCS, 4000);
+  mSharedRef = new detail::SharedRef(this);
+  ::InitializeCS(mCSForQI);
 }
 
 WeakReferenceSupport::~WeakReferenceSupport()
 {
-  MOZ_ASSERT(mWeakRefs.IsEmpty());
-  ::DeleteCriticalSection(&mCS);
+  ::DeleteCriticalSection(&mCSForQI);
 }
 
 HRESULT
 WeakReferenceSupport::QueryInterface(REFIID riid, void** ppv)
 {
-  AutoCriticalSection lock(&mCS);
   RefPtr<IUnknown> punk;
   if (!ppv) {
     return E_INVALIDARG;
   }
   *ppv = nullptr;
 
   // Raise the refcount for stabilization purposes during aggregation
   RefPtr<IUnknown> kungFuDeathGrip(this);
 
   if (riid == IID_IUnknown || riid == IID_IWeakReferenceSource) {
     punk = static_cast<IUnknown*>(this);
   } else {
+    AutoCriticalSection lock(&mCSForQI);
     HRESULT hr = ThreadSafeQueryInterface(riid, getter_AddRefs(punk));
     if (FAILED(hr)) {
       return hr;
     }
   }
 
   if (!punk) {
     return E_NOINTERFACE;
@@ -57,31 +120,34 @@ WeakReferenceSupport::QueryInterface(REF
 
   punk.forget(ppv);
   return S_OK;
 }
 
 ULONG
 WeakReferenceSupport::AddRef()
 {
-  AutoCriticalSection lock(&mCS);
-  return ++mRefCnt;
+  SharedRefAutoLock lock(*mSharedRef);
+  ULONG result = ++mRefCnt;
+  NS_LOG_ADDREF(this, result, "mscom::WeakReferenceSupport", sizeof(*this));
+  return result;
 }
 
 ULONG
 WeakReferenceSupport::Release()
 {
   ULONG newRefCnt;
   { // Scope for lock
-    AutoCriticalSection lock(&mCS);
+    SharedRefAutoLock lock(*mSharedRef);
     newRefCnt = --mRefCnt;
     if (newRefCnt == 0) {
-      ClearWeakRefs();
+      mSharedRef->Clear();
     }
   }
+  NS_LOG_RELEASE(this, newRefCnt, "mscom::WeakReferenceSupport");
   if (newRefCnt == 0) {
     if (mFlags != Flags::eDestroyOnMainThread || NS_IsMainThread()) {
       delete this;
     } else {
       // It is possible for the last Release() call to happen off-main-thread.
       // If so, we need to dispatch an event to delete ourselves.
       mozilla::DebugOnly<nsresult> rv =
         NS_DispatchToMainThread(NS_NewRunnableFunction([this]() -> void
@@ -89,52 +155,32 @@ WeakReferenceSupport::Release()
           delete this;
         }));
       MOZ_ASSERT(NS_SUCCEEDED(rv));
     }
   }
   return newRefCnt;
 }
 
-void
-WeakReferenceSupport::ClearWeakRefs()
-{
-  for (uint32_t i = 0, len = mWeakRefs.Length(); i < len; ++i) {
-    mWeakRefs[i]->Clear();
-    mWeakRefs[i] = nullptr;
-  }
-  mWeakRefs.Clear();
-}
-
 HRESULT
 WeakReferenceSupport::GetWeakReference(IWeakReference** aOutWeakRef)
 {
   if (!aOutWeakRef) {
     return E_INVALIDARG;
   }
-  *aOutWeakRef = nullptr;
 
-  AutoCriticalSection lock(&mCS);
-  RefPtr<WeakRef> weakRef = MakeAndAddRef<WeakRef>(this);
-
-  HRESULT hr = weakRef->QueryInterface(IID_IWeakReference, (void**)aOutWeakRef);
-  if (FAILED(hr)) {
-    return hr;
-  }
-
-  mWeakRefs.AppendElement(weakRef);
-  return S_OK;
+  RefPtr<WeakRef> weakRef = MakeAndAddRef<WeakRef>(mSharedRef);
+  return weakRef->QueryInterface(IID_IWeakReference, (void**)aOutWeakRef);
 }
 
-WeakRef::WeakRef(WeakReferenceSupport* aSupport)
-  : mRefCnt(1)
-  , mMutex("mozilla::mscom::WeakRef::mMutex")
-  , mSupport(aSupport)
+WeakRef::WeakRef(RefPtr<detail::SharedRef>& aSharedRef)
+  : mRefCnt(0)
+  , mSharedRef(aSharedRef)
 {
-  MOZ_ASSERT(aSupport);
+  MOZ_ASSERT(aSharedRef);
 }
 
 HRESULT
 WeakRef::QueryInterface(REFIID riid, void** ppv)
 {
   IUnknown* punk = nullptr;
   if (!ppv) {
     return E_INVALIDARG;
@@ -151,42 +197,33 @@ WeakRef::QueryInterface(REFIID riid, voi
 
   punk->AddRef();
   return S_OK;
 }
 
 ULONG
 WeakRef::AddRef()
 {
-  return (ULONG) InterlockedIncrement((LONG*)&mRefCnt);
+  ULONG result = ++mRefCnt;
+  NS_LOG_ADDREF(this, result, "mscom::WeakRef", sizeof(*this));
+  return result;
 }
 
 ULONG
 WeakRef::Release()
 {
-  ULONG newRefCnt = (ULONG) InterlockedDecrement((LONG*)&mRefCnt);
+  ULONG newRefCnt = --mRefCnt;
+  NS_LOG_RELEASE(this, newRefCnt, "mscom::WeakRef");
   if (newRefCnt == 0) {
     delete this;
   }
   return newRefCnt;
 }
 
 HRESULT
 WeakRef::Resolve(REFIID aIid, void** aOutStrongReference)
 {
-  MutexAutoLock lock(mMutex);
-  if (!mSupport) {
-    return E_FAIL;
-  }
-  return mSupport->QueryInterface(aIid, aOutStrongReference);
-}
-
-void
-WeakRef::Clear()
-{
-  MutexAutoLock lock(mMutex);
-  MOZ_ASSERT(mSupport);
-  mSupport = nullptr;
+  return mSharedRef->Resolve(aIid, aOutStrongReference);
 }
 
 } // namespace mscom
 } // namespace mozilla
 
--- a/ipc/mscom/WeakRef.h
+++ b/ipc/mscom/WeakRef.h
@@ -5,34 +5,67 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_mscom_WeakRef_h
 #define mozilla_mscom_WeakRef_h
 
 #include <guiddef.h>
 #include <unknwn.h>
 
-#include "mozilla/Mutex.h"
-#include "nsTArray.h"
+#include "mozilla/Assertions.h"
+#include "mozilla/Atomics.h"
+#include "mozilla/RefPtr.h"
+#include "nsISupportsImpl.h"
 
 /**
  * Thread-safe weak references for COM that works pre-Windows 8 and do not
  * require WinRT.
  */
 
 namespace mozilla {
 namespace mscom {
 
+class WeakReferenceSupport;
+
+namespace detail {
+
+class SharedRef final
+{
+public:
+  explicit SharedRef(WeakReferenceSupport* aSupport);
+  void Lock();
+  void Unlock();
+
+  HRESULT Resolve(REFIID aIid, void** aOutStrongReference);
+  void Clear();
+
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(SharedRef)
+
+  SharedRef(const SharedRef&) = delete;
+  SharedRef(SharedRef&&) = delete;
+  SharedRef& operator=(const SharedRef&) = delete;
+  SharedRef& operator=(SharedRef&&) = delete;
+
+private:
+  ~SharedRef();
+
+private:
+  CRITICAL_SECTION mCS;
+  WeakReferenceSupport* mSupport;
+};
+
+} // namespace detail
+
 // {F841AEFA-064C-49A4-B73D-EBD14A90F012}
 DEFINE_GUID(IID_IWeakReference,
 0xf841aefa, 0x64c, 0x49a4, 0xb7, 0x3d, 0xeb, 0xd1, 0x4a, 0x90, 0xf0, 0x12);
 
 struct IWeakReference : public IUnknown
 {
-  virtual STDMETHODIMP Resolve(REFIID aIid, void** aOutStringReference) = 0;
+  virtual STDMETHODIMP Resolve(REFIID aIid, void** aOutStrongReference) = 0;
 };
 
 // {87611F0C-9BBB-4F78-9D43-CAC5AD432CA1}
 DEFINE_GUID(IID_IWeakReferenceSource,
 0x87611f0c, 0x9bbb, 0x4f78, 0x9d, 0x43, 0xca, 0xc5, 0xad, 0x43, 0x2c, 0xa1);
 
 struct IWeakReferenceSource : public IUnknown
 {
@@ -61,44 +94,37 @@ public:
 protected:
   explicit WeakReferenceSupport(Flags aFlags);
   virtual ~WeakReferenceSupport();
 
   virtual HRESULT ThreadSafeQueryInterface(REFIID aIid,
                                            IUnknown** aOutInterface) = 0;
 
 private:
-  void ClearWeakRefs();
-
-private:
-  // Using a raw CRITICAL_SECTION here because it can be reentered
-  CRITICAL_SECTION           mCS;
-  ULONG                      mRefCnt;
-  nsTArray<RefPtr<WeakRef>>  mWeakRefs;
-  Flags                      mFlags;
+  RefPtr<detail::SharedRef> mSharedRef;
+  ULONG                     mRefCnt;
+  Flags                     mFlags;
+  CRITICAL_SECTION          mCSForQI;
 };
 
 class WeakRef : public IWeakReference
 {
 public:
   // IUnknown
   STDMETHODIMP QueryInterface(REFIID riid, void** ppv) override;
   STDMETHODIMP_(ULONG) AddRef() override;
   STDMETHODIMP_(ULONG) Release() override;
 
   // IWeakReference
   STDMETHODIMP Resolve(REFIID aIid, void** aOutStrongReference) override;
 
-  explicit WeakRef(WeakReferenceSupport* aSupport);
-
-  void Clear();
+  explicit WeakRef(RefPtr<detail::SharedRef>& aSharedRef);
 
 private:
-  ULONG                 mRefCnt;
-  mozilla::Mutex        mMutex; // Protects mSupport
-  WeakReferenceSupport* mSupport;
+  Atomic<ULONG>             mRefCnt;
+  RefPtr<detail::SharedRef> mSharedRef;
 };
 
 } // namespace mscom
 } // namespace mozilla
 
 #endif // mozilla_mscom_WeakRef_h