Bug 1450321, part 1 - Remove locks and threadsafe refcounting from reflect/xptinfo. draft
authorAndrew McCreight <continuation@gmail.com>
Thu, 29 Mar 2018 16:20:17 -0700
changeset 776985 96c505f27f40c218b793dad0a9975c88c336bc66
parent 776984 53169ab0c75f8cd7d030955d790dd67fe160ccf8
child 776986 764f688b823c307e70e8aa0792b9c9296a88aa70
push id105047
push userbmo:continuation@gmail.com
push dateTue, 03 Apr 2018 23:36:00 +0000
bugs1450321
milestone61.0a1
Bug 1450321, part 1 - Remove locks and threadsafe refcounting from reflect/xptinfo. These classes can only be used from the main thread now that XPConnect is single threaded. MozReview-Commit-ID: 1A5j51AGbr
xpcom/reflect/xptinfo/XPTInterfaceInfoManager.h
xpcom/reflect/xptinfo/xptiInterfaceInfo.cpp
xpcom/reflect/xptinfo/xptiInterfaceInfoManager.cpp
xpcom/reflect/xptinfo/xptiTypelibGuts.cpp
xpcom/reflect/xptinfo/xptiWorkingSet.cpp
xpcom/reflect/xptinfo/xptiprivate.h
--- a/xpcom/reflect/xptinfo/XPTInterfaceInfoManager.h
+++ b/xpcom/reflect/xptinfo/XPTInterfaceInfoManager.h
@@ -6,47 +6,43 @@
 
 #ifndef mozilla_XPTInterfaceInfoManager_h_
 #define mozilla_XPTInterfaceInfoManager_h_
 
 #include "nsIInterfaceInfoManager.h"
 #include "nsIMemoryReporter.h"
 
 #include "mozilla/MemoryReporting.h"
-#include "mozilla/Mutex.h"
-#include "mozilla/ReentrantMonitor.h"
 #include "nsDataHashtable.h"
 
 template<typename T>
 class nsCOMArray;
 class nsIMemoryReporter;
 struct XPTInterfaceDescriptor;
 class xptiInterfaceEntry;
 class xptiInterfaceInfo;
 class xptiTypelibGuts;
 
 namespace mozilla {
 
 class XPTInterfaceInfoManager final
   : public nsIInterfaceInfoManager
   , public nsIMemoryReporter
 {
-  NS_DECL_THREADSAFE_ISUPPORTS
+  NS_DECL_ISUPPORTS
   NS_DECL_NSIINTERFACEINFOMANAGER
   NS_DECL_NSIMEMORYREPORTER
 
 public:
   // GetSingleton() is infallible
   static XPTInterfaceInfoManager* GetSingleton();
   static void FreeInterfaceInfoManager();
 
   void GetScriptableInterfaces(nsCOMArray<nsIInterfaceInfo>& aInterfaces);
 
-  static Mutex& GetResolveLock() { return GetSingleton()->mResolveLock; }
-
   xptiInterfaceEntry* GetInterfaceEntryForIID(const nsIID* iid);
 
   size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf);
 
 private:
   XPTInterfaceInfoManager();
   ~XPTInterfaceInfoManager();
 
@@ -62,29 +58,23 @@ private:
   {
   public:
     xptiWorkingSet();
     ~xptiWorkingSet();
 
     void InvalidateInterfaceInfos();
 
     // XXX make these private with accessors
-    // mTableMonitor must be held across:
-    //  * any read from or write to mIIDTable or mNameTable
-    //  * any writing to the links between an xptiInterfaceEntry
-    //    and its xptiInterfaceInfo (mEntry/mInfo)
-    mozilla::ReentrantMonitor mTableReentrantMonitor;
     nsDataHashtable<nsIDHashKey, xptiInterfaceEntry*> mIIDTable;
     nsDataHashtable<nsDepCharHashKey, xptiInterfaceEntry*> mNameTable;
   };
 
-  // XXX xptiInterfaceInfo want's to poke at the working set itself
+  // XXX xptiInterfaceInfo wants to poke at the working set itself
   friend class ::xptiInterfaceInfo;
   friend class ::xptiInterfaceEntry;
   friend class ::xptiTypelibGuts;
 
   xptiWorkingSet mWorkingSet;
-  Mutex mResolveLock;
 };
 
 } // namespace mozilla
 
 #endif
--- a/xpcom/reflect/xptinfo/xptiInterfaceInfo.cpp
+++ b/xpcom/reflect/xptinfo/xptiInterfaceInfo.cpp
@@ -43,17 +43,16 @@ xptiInterfaceEntry::xptiInterfaceEntry(c
   SetScriptableFlag(aIface->IsScriptable());
   SetBuiltinClassFlag(aIface->IsBuiltinClass());
   SetMainProcessScriptableOnlyFlag(aIface->IsMainProcessScriptableOnly());
 }
 
 bool
 xptiInterfaceEntry::Resolve()
 {
-  MutexAutoLock lock(XPTInterfaceInfoManager::GetResolveLock());
   return ResolveLocked();
 }
 
 bool
 xptiInterfaceEntry::ResolveLocked()
 {
   int resolvedState = GetResolveState();
 
@@ -558,21 +557,16 @@ xptiInterfaceEntry::HasAncestor(const ns
   return NS_OK;
 }
 
 /***************************************************/
 
 already_AddRefed<xptiInterfaceInfo>
 xptiInterfaceEntry::InterfaceInfo()
 {
-#ifdef DEBUG
-  XPTInterfaceInfoManager::GetSingleton()
-    ->mWorkingSet.mTableReentrantMonitor.AssertCurrentThreadIn();
-#endif
-
   if (!mInfo) {
     mInfo = new xptiInterfaceInfo(this);
   }
 
   RefPtr<xptiInterfaceInfo> info = mInfo;
   return info.forget();
 }
 
@@ -583,19 +577,16 @@ xptiInterfaceEntry::LockedInvalidateInte
     mInfo->Invalidate();
     mInfo = nullptr;
   }
 }
 
 bool
 xptiInterfaceInfo::BuildParent()
 {
-  mozilla::ReentrantMonitorAutoEnter monitor(
-    XPTInterfaceInfoManager::GetSingleton()
-      ->mWorkingSet.mTableReentrantMonitor);
   NS_ASSERTION(mEntry && mEntry->IsFullyResolved() && !mParent &&
                  mEntry->Parent(),
                "bad BuildParent call");
   mParent = mEntry->Parent()->InterfaceInfo();
   return true;
 }
 
 /***************************************************************************/
@@ -629,20 +620,16 @@ xptiInterfaceInfo::AddRef(void)
 
 MozExternalRefCountType
 xptiInterfaceInfo::Release(void)
 {
   xptiInterfaceEntry* entry = mEntry;
   nsrefcnt cnt = --mRefCnt;
   NS_LOG_RELEASE(this, cnt, "xptiInterfaceInfo");
   if (!cnt) {
-    mozilla::ReentrantMonitorAutoEnter monitor(
-      XPTInterfaceInfoManager::GetSingleton()
-        ->mWorkingSet.mTableReentrantMonitor);
-
     // If InterfaceInfo added and *released* a reference before we
     // acquired the monitor then 'this' might already be dead. In that
     // case we would not want to try to access any instance data. We
     // would want to bail immediately. If 'this' is already dead then the
     // entry will no longer have a pointer to 'this'. So, we can protect
     // ourselves from danger without more aggressive locking.
     if (entry && !entry->InterfaceInfoEquals(this)) {
       return 0;
--- a/xpcom/reflect/xptinfo/xptiInterfaceInfoManager.cpp
+++ b/xpcom/reflect/xptinfo/xptiInterfaceInfoManager.cpp
@@ -27,17 +27,16 @@ NS_IMPL_ISUPPORTS(XPTInterfaceInfoManage
 
 static StaticRefPtr<XPTInterfaceInfoManager> gInterfaceInfoManager;
 
 size_t
 XPTInterfaceInfoManager::SizeOfIncludingThis(
   mozilla::MallocSizeOf aMallocSizeOf)
 {
   size_t n = aMallocSizeOf(this);
-  ReentrantMonitorAutoEnter monitor(mWorkingSet.mTableReentrantMonitor);
   // The entries themselves are allocated out of an arena accounted
   // for elsewhere, so don't measure them
   n += mWorkingSet.mIIDTable.ShallowSizeOfExcludingThis(aMallocSizeOf);
   n += mWorkingSet.mNameTable.ShallowSizeOfExcludingThis(aMallocSizeOf);
   return n;
 }
 
 MOZ_DEFINE_MALLOC_SIZE_OF(XPTIMallocSizeOf)
@@ -77,21 +76,19 @@ XPTInterfaceInfoManager::GetSingleton()
 void
 XPTInterfaceInfoManager::FreeInterfaceInfoManager()
 {
   gInterfaceInfoManager = nullptr;
 }
 
 XPTInterfaceInfoManager::XPTInterfaceInfoManager()
   : mWorkingSet()
-  , mResolveLock("XPTInterfaceInfoManager.mResolveLock")
 {
   xptiTypelibGuts* typelib = xptiTypelibGuts::Create();
 
-  ReentrantMonitorAutoEnter monitor(mWorkingSet.mTableReentrantMonitor);
   for (uint16_t k = 0; k < XPTHeader::kNumInterfaces; k++) {
     VerifyAndAddEntryIfNew(XPTHeader::kInterfaces + k, k, typelib);
   }
 }
 
 XPTInterfaceInfoManager::~XPTInterfaceInfoManager()
 {
   // We only do this on shutdown of the service.
@@ -124,17 +121,16 @@ XPTInterfaceInfoManager::VerifyAndAddEnt
   // in xpcom/reflect/xptcall/genstubs.pl; do not change this value
   // without changing that one or you WILL see problems.
   if (iface->mNumMethods > 250 && !iface->IsBuiltinClass()) {
     NS_ASSERTION(0, "Too many methods to handle for the stub, cannot load");
     fprintf(stderr, "ignoring too large interface: %s\n", iface->Name());
     return;
   }
 
-  mWorkingSet.mTableReentrantMonitor.AssertCurrentThreadIn();
   xptiInterfaceEntry* entry = mWorkingSet.mIIDTable.Get(iface->mIID);
   if (entry) {
     // XXX validate this info to find possible inconsistencies
     return;
   }
 
   // Build a new xptiInterfaceEntry object and hook it up.
 
@@ -160,54 +156,50 @@ EntryToInfo(xptiInterfaceEntry* entry, n
   RefPtr<xptiInterfaceInfo> info = entry->InterfaceInfo();
   info.forget(_retval);
   return NS_OK;
 }
 
 xptiInterfaceEntry*
 XPTInterfaceInfoManager::GetInterfaceEntryForIID(const nsIID* iid)
 {
-  ReentrantMonitorAutoEnter monitor(mWorkingSet.mTableReentrantMonitor);
   return mWorkingSet.mIIDTable.Get(*iid);
 }
 
 NS_IMETHODIMP
 XPTInterfaceInfoManager::GetInfoForIID(const nsIID* iid,
                                        nsIInterfaceInfo** _retval)
 {
   NS_ASSERTION(iid, "bad param");
   NS_ASSERTION(_retval, "bad param");
 
-  ReentrantMonitorAutoEnter monitor(mWorkingSet.mTableReentrantMonitor);
   xptiInterfaceEntry* entry = mWorkingSet.mIIDTable.Get(*iid);
   return EntryToInfo(entry, _retval);
 }
 
 NS_IMETHODIMP
 XPTInterfaceInfoManager::GetInfoForName(const char* name,
                                         nsIInterfaceInfo** _retval)
 {
   NS_ASSERTION(name, "bad param");
   NS_ASSERTION(_retval, "bad param");
 
-  ReentrantMonitorAutoEnter monitor(mWorkingSet.mTableReentrantMonitor);
   xptiInterfaceEntry* entry = mWorkingSet.mNameTable.Get(name);
   return EntryToInfo(entry, _retval);
 }
 
 void
 XPTInterfaceInfoManager::GetScriptableInterfaces(
   nsCOMArray<nsIInterfaceInfo>& aInterfaces)
 {
   // I didn't want to incur the size overhead of using nsHashtable just to
   // make building an enumerator easier. So, this code makes a snapshot of
   // the table using an nsCOMArray and builds an enumerator for that.
   // We can afford this transient cost.
 
-  ReentrantMonitorAutoEnter monitor(mWorkingSet.mTableReentrantMonitor);
   aInterfaces.SetCapacity(mWorkingSet.mNameTable.Count());
   for (auto iter = mWorkingSet.mNameTable.Iter(); !iter.Done(); iter.Next()) {
     xptiInterfaceEntry* entry = iter.UserData();
     if (entry->GetScriptableFlag()) {
       nsCOMPtr<nsIInterfaceInfo> ii = entry->InterfaceInfo();
       aInterfaces.AppendElement(ii);
     }
   }
--- a/xpcom/reflect/xptinfo/xptiTypelibGuts.cpp
+++ b/xpcom/reflect/xptinfo/xptiTypelibGuts.cpp
@@ -46,23 +46,20 @@ xptiTypelibGuts::GetEntryAt(uint16_t i)
     return r;
   }
 
   const XPTInterfaceDescriptor* iface = XPTHeader::kInterfaces + i;
 
   XPTInterfaceInfoManager::xptiWorkingSet& set =
     XPTInterfaceInfoManager::GetSingleton()->mWorkingSet;
 
-  {
-    ReentrantMonitorAutoEnter monitor(set.mTableReentrantMonitor);
-    if (iface->mIID.Equals(zeroIID)) {
-      r = set.mNameTable.Get(iface->Name());
-    } else {
-      r = set.mIIDTable.Get(iface->mIID);
-    }
+  if (iface->mIID.Equals(zeroIID)) {
+    r = set.mNameTable.Get(iface->Name());
+  } else {
+    r = set.mIIDTable.Get(iface->mIID);
   }
 
   if (r) {
     SetEntryAt(i, r);
   }
 
   return r;
 }
--- a/xpcom/reflect/xptinfo/xptiWorkingSet.cpp
+++ b/xpcom/reflect/xptinfo/xptiWorkingSet.cpp
@@ -14,30 +14,28 @@
 using namespace mozilla;
 
 static const size_t XPTI_ARENA8_BLOCK_SIZE = 16 * 1024;
 static const size_t XPTI_ARENA1_BLOCK_SIZE = 8 * 1024;
 
 static const uint32_t XPTI_HASHTABLE_LENGTH = 1024;
 
 XPTInterfaceInfoManager::xptiWorkingSet::xptiWorkingSet()
-  : mTableReentrantMonitor("xptiWorkingSet::mTableReentrantMonitor")
-  , mIIDTable(XPTI_HASHTABLE_LENGTH)
+  : mIIDTable(XPTI_HASHTABLE_LENGTH)
   , mNameTable(XPTI_HASHTABLE_LENGTH)
 {
   MOZ_COUNT_CTOR(xptiWorkingSet);
 
   gXPTIStructArena =
     XPT_NewArena(XPTI_ARENA8_BLOCK_SIZE, XPTI_ARENA1_BLOCK_SIZE);
 }
 
 void
 XPTInterfaceInfoManager::xptiWorkingSet::InvalidateInterfaceInfos()
 {
-  ReentrantMonitorAutoEnter monitor(mTableReentrantMonitor);
   for (auto iter = mNameTable.Iter(); !iter.Done(); iter.Next()) {
     xptiInterfaceEntry* entry = iter.UserData();
     entry->LockedInvalidateInterfaceInfo();
   }
 }
 
 XPTInterfaceInfoManager::xptiWorkingSet::~xptiWorkingSet()
 {
--- a/xpcom/reflect/xptinfo/xptiprivate.h
+++ b/xpcom/reflect/xptinfo/xptiprivate.h
@@ -335,17 +335,17 @@ private:
   xptiInfoFlags mFlags;
 
   const char* mName;
 };
 
 class xptiInterfaceInfo final : public nsIInterfaceInfo
 {
 public:
-  NS_DECL_THREADSAFE_ISUPPORTS
+  NS_DECL_ISUPPORTS
 
   // Use delegation to implement (most!) of nsIInterfaceInfo.
   NS_IMETHOD GetName(char** aName) override
   {
     return !mEntry ? NS_ERROR_UNEXPECTED : mEntry->GetName(aName);
   }
   NS_IMETHOD IsScriptable(bool* _retval) override
   {