Bug 1388998 - Drop final Connection reference after finishing guarded connection list mutation. r=mak
authorAndrew Sutherland <asutherland@asutherland.org>
Tue, 03 Oct 2017 01:14:22 -0400
changeset 440458 2f78917dbcfdfab5ef0470557c8ca98003cb5f09
parent 440457 380afb457c9a45c977c1cace6263c9284d6bf649
child 440459 3b3a3a585d0d97233737efb1573670f34689bc63
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1388998
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1388998 - Drop final Connection reference after finishing guarded connection list mutation. r=mak
storage/mozStorageService.cpp
--- a/storage/mozStorageService.cpp
+++ b/storage/mozStorageService.cpp
@@ -297,37 +297,48 @@ Service::registerConnection(Connection *
 
 void
 Service::unregisterConnection(Connection *aConnection)
 {
   // If this is the last Connection it might be the only thing keeping Service
   // alive.  So ensure that Service is destroyed only after the Connection is
   // cleanly unregistered and destroyed.
   RefPtr<Service> kungFuDeathGrip(this);
+  RefPtr<Connection> forgettingRef;
   {
     mRegistrationMutex.AssertNotCurrentThreadOwns();
     MutexAutoLock mutex(mRegistrationMutex);
 
     for (uint32_t i = 0 ; i < mConnections.Length(); ++i) {
       if (mConnections[i] == aConnection) {
-        nsCOMPtr<nsIThread> thread = mConnections[i]->threadOpenedOn;
-
-        // Ensure the connection is released on its opening thread.  Note, we
-        // must use .forget().take() so that we can manually cast to an
-        // unambiguous nsISupports type.
-        NS_ProxyRelease(
-          "storage::Service::mConnections", thread, mConnections[i].forget());
-
+        // Because dropping the final reference can potentially result in
+        // spinning a nested event loop if the connection was not properly
+        // shutdown, we want to do that outside this loop so that we can finish
+        // mutating the array and drop our mutex.
+        forgettingRef = mConnections[i].forget();
         mConnections.RemoveElementAt(i);
-        return;
+        break;
       }
     }
+  }
 
-    MOZ_ASSERT_UNREACHABLE("Attempt to unregister unknown storage connection!");
-  }
+  MOZ_ASSERT(forgettingRef,
+             "Attempt to unregister unknown storage connection!");
+
+  // Ensure the connection is released on its opening thread.  We explicitly use
+  // aAlwaysDispatch=false because at the time of writing this, LocalStorage's
+  // StorageDBThread uses a hand-rolled PRThread implementation that cannot
+  // handle us dispatching events at it during shutdown.  However, it is
+  // arguably also desirable for callers to not be aware of our connection
+  // tracking mechanism.  And by synchronously dropping the reference (when
+  // on the correct thread), this avoids surprises for the caller and weird
+  // shutdown edge cases.
+  nsCOMPtr<nsIThread> thread = forgettingRef->threadOpenedOn;
+  NS_ProxyRelease(
+    "storage::Service::mConnections", thread, forgettingRef.forget(), false);
 }
 
 void
 Service::getConnections(/* inout */ nsTArray<RefPtr<Connection> >& aConnections)
 {
   mRegistrationMutex.AssertNotCurrentThreadOwns();
   MutexAutoLock mutex(mRegistrationMutex);
   aConnections.Clear();