Bug 1413501 - Fix for SpinningSynchronousClose unregisterConnection edge-case. r=mak
authorAndrew Sutherland <asutherland@asutherland.org>
Wed, 08 Nov 2017 10:11:27 -0800
changeset 435995 47f845a860d85465af6df2827cf40f77432deafb
parent 435994 f196f626243a3643315ec146e848181ea6dd04db
child 435996 4b34fc8bfe40da9748d014f3562e9468eab18a89
push id117
push userfmarier@mozilla.com
push dateTue, 28 Nov 2017 20:17:16 +0000
reviewersmak
bugs1413501
milestone59.0a1
Bug 1413501 - Fix for SpinningSynchronousClose unregisterConnection edge-case. r=mak
storage/mozStorageConnection.cpp
--- a/storage/mozStorageConnection.cpp
+++ b/storage/mozStorageConnection.cpp
@@ -543,17 +543,19 @@ Connection::Connection(Service *aService
 {
   MOZ_ASSERT(!mIgnoreLockingMode || mFlags & SQLITE_OPEN_READONLY,
              "Can't ignore locking for a non-readonly connection!");
   mStorageService->registerConnection(this);
 }
 
 Connection::~Connection()
 {
-  Unused << Close();
+  // Failsafe Close() occurs in our custom Release method because of
+  // complications related to Close() potentially invoking AsyncClose() which
+  // will increment our refcount.
   MOZ_ASSERT(!mAsyncExecutionThread,
              "The async thread has not been shutdown properly!");
 }
 
 NS_IMPL_ADDREF(Connection)
 
 NS_INTERFACE_MAP_BEGIN(Connection)
   NS_INTERFACE_MAP_ENTRY(mozIStorageAsyncConnection)
@@ -567,18 +569,65 @@ NS_INTERFACE_MAP_END
 NS_IMETHODIMP_(MozExternalRefCountType) Connection::Release(void)
 {
   NS_PRECONDITION(0 != mRefCnt, "dup release");
   nsrefcnt count = --mRefCnt;
   NS_LOG_RELEASE(this, count, "Connection");
   if (1 == count) {
     // If the refcount is 1, the single reference must be from
     // gService->mConnections (in class |Service|).  Which means we can
-    // unregister it safely.
-    mStorageService->unregisterConnection(this);
+    // perform our failsafe Close() and unregister...
+    //
+    // HOWEVER, there is an edge-case where our failsafe Close() may trigger
+    // a call to AsyncClose() which obtains a strong reference.  This reference
+    // will be released via NS_ReleaseOnMainThreadSystemGroup() before Close()
+    // returns, which can potentially result in reentrancy into this method and
+    // this branch a second time.  (It may also be deferred if we're not in
+    // that event target ourselves.)  To avoid reentrancy madness, we explicitly
+    // bump our refcount up to 2 without going through AddRef().
+    ++mRefCnt;
+    // Okay, now our refcount is 2, we trigger Close().
+    Unused << Close();
+    // Now our refcount should either be at 2 (because nothing happened, or the
+    // addref and release pair happened due to SpinningSynchronousClose) or
+    // 3 (because SpinningSynchronousClose happened but didn't release yet).
+    //
+    // We *really* want to avoid re-entrancy, and we have potentially two strong
+    // references remaining that will invoke Release() and potentially trigger
+    // a transition to 1 again.  Since the second reference would be just a
+    // proxy release of an already-closed connection, it's not a big deal for us
+    // to unregister the connection now.  We do need to take care to avoid a
+    // strong refcount transition to 1 from 2 because that would induce
+    // reentrancy.  Note that we do not have any concerns about other threads
+    // being involved here; we MUST be the main thread if AsyncClose() is
+    // involved.
+    //
+    // Note: While Close() potentially spins the nested event loop, it is
+    // conceivable that Service::CollectReports or Service::minimizeMemory might
+    // be invoked.  These call Service::getConnections() and will perform
+    // matching AddRef and Release calls but will definitely not retain any
+    // references.  (Because connectionReady() will return false so both loops
+    // will immediately "continue" to bypass the connection in question.)
+    // Because our refcount is at least 2 at the lowest point, these do not pose
+    // a problem.
+    if (mRefCnt == 3) {
+      // pending proxy release, strong release to 2
+      mStorageService->unregisterConnection(this);
+      // now weak release to 1, the outstanding refcount will strong release to
+      // 0 and result in destruction later.
+      --mRefCnt;
+    } else if (mRefCnt == 2) {
+      // weak release to 1
+      --mRefCnt;
+      // strong release to 0, destruction will happen, we must NOT touch
+      // `this` after this point.
+      mStorageService->unregisterConnection(this);
+    } else {
+      MOZ_ASSERT(false, "Connection refcount invariant violated.");
+    }
   } else if (0 == count) {
     mRefCnt = 1; /* stabilize */
 #if 0 /* enable this to find non-threadsafe destructors: */
     NS_ASSERT_OWNINGTHREAD(Connection);
 #endif
     delete (this);
     return 0;
   }