author | Andrew Sutherland <asutherland@asutherland.org> |
Sun, 25 Feb 2018 23:50:42 -0500 | |
changeset 461046 | fb544e206108dedd39a8dfac98a06116f039fc2a |
parent 461045 | 8e31ba435fa80983e2fb84958fd1f6fbbb1b51ea |
child 461047 | 5e05f1b828468b6e951253929044c655643c4208 |
push id | 1683 |
push user | sfraser@mozilla.com |
push date | Thu, 26 Apr 2018 16:43:40 +0000 |
treeherder | mozilla-release@5af6cb21869d [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | mak |
bugs | 1422327 |
milestone | 60.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
|
--- a/storage/mozStorageConnection.cpp +++ b/storage/mozStorageConnection.cpp @@ -530,16 +530,17 @@ Connection::Connection(Service *aService bool aIgnoreLockingMode) : sharedAsyncExecutionMutex("Connection::sharedAsyncExecutionMutex") , sharedDBMutex("Connection::sharedDBMutex") , threadOpenedOn(do_GetCurrentThread()) , mDBConn(nullptr) , mAsyncExecutionThreadShuttingDown(false) , mConnectionClosed(false) , mTransactionInProgress(false) +, mDestroying(false) , mProgressHandler(nullptr) , mFlags(aFlags) , mIgnoreLockingMode(aIgnoreLockingMode) , mStorageService(aService) , mAsyncOnly(aAsyncOnly) { MOZ_ASSERT(!mIgnoreLockingMode || mFlags & SQLITE_OPEN_READONLY, "Can't ignore locking for a non-readonly connection!"); @@ -567,66 +568,61 @@ NS_INTERFACE_MAP_END // This is identical to what NS_IMPL_RELEASE provides, but with the // extra |1 == count| case. 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 - // 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). + // If the refcount went to 1, the single reference must be from + // gService->mConnections (in class |Service|). And the code calling + // Release is either: + // - The "user" code that had created the connection, releasing on any + // thread. + // - One of Service's getConnections() callers had acquired a strong + // reference to the Connection that out-lived the last "user" reference, + // and now that just got dropped. Note that this reference could be + // getting dropped on the main thread or Connection->threadOpenedOn + // (because of the NewRunnableMethod used by minimizeMemory). // - // 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 + // Either way, we should now perform our failsafe Close() and unregister. + // However, we only want to do this once, and the reality is that our + // refcount could go back up above 1 and down again at any time if we are + // off the main thread and getConnections() gets called on the main thread, + // so we use an atomic here to do this exactly once. + if (mDestroying.compareExchange(false, true)) { + // Close the connection, dispatching to the opening thread if we're not + // on that thread already and that thread is still accepting runnables. + // We do this because it's possible we're on the main thread because of + // getConnections(), and we REALLY don't want to transfer I/O to the main + // thread if we can avoid it. + if (threadOpenedOn->IsOnCurrentThread()) { + // This could cause SpinningSynchronousClose() to be invoked and AddRef + // triggered for AsyncCloseConnection's strong ref if the conn was ever + // use for async purposes. (Main-thread only, though.) + Unused << Close(); + } else { + nsCOMPtr<nsIRunnable> event = + NewRunnableMethod("storage::Connection::Close", + this, &Connection::Close); + if (NS_FAILED(threadOpenedOn->Dispatch(event.forget(), + NS_DISPATCH_NORMAL))) { + // The target thread was dead and so we've just leaked our runnable. + // This should not happen because our non-main-thread consumers should + // be explicitly closing their connections, not relying on us to close + // them for them. (It's okay to let a statement go out of scope for + // automatic cleanup, but not a Connection.) + MOZ_ASSERT(false, "Leaked Connection::Close(), ownership fail."); + Unused << Close(); + } + } + + // This will drop its strong reference right here, right now. 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;
--- a/storage/mozStorageConnection.h +++ b/storage/mozStorageConnection.h @@ -4,16 +4,17 @@ * 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/. */ #ifndef mozilla_storage_Connection_h #define mozilla_storage_Connection_h #include "nsAutoPtr.h" #include "nsCOMPtr.h" +#include "mozilla/Atomics.h" #include "mozilla/Mutex.h" #include "nsProxyRelease.h" #include "nsThreadUtils.h" #include "nsIInterfaceRequestor.h" #include "nsDataHashtable.h" #include "mozIStorageProgressHandler.h" #include "SQLiteMutex.h" @@ -376,16 +377,26 @@ private: /** * Tracks if we have a transaction in progress or not. Access protected by * sharedDBMutex. */ bool mTransactionInProgress; /** + * Used to trigger cleanup logic only the first time our refcount hits 1. We + * may trigger a failsafe Close() that invokes SpinningSynchronousClose() + * which invokes AsyncClose() which may bump our refcount back up to 2 (and + * which will then fall back down to 1 again). It's also possible that the + * Service may bump our refcount back above 1 if getConnections() runs before + * we invoke unregisterConnection(). + */ + mozilla::Atomic<bool> mDestroying; + + /** * Stores the mapping of a given function by name to its instance. Access is * protected by sharedDBMutex. */ nsDataHashtable<nsCStringHashKey, FunctionInfo> mFunctions; /** * Stores the registered progress handler for the database connection. Access * is protected by sharedDBMutex.
--- a/storage/mozStorageService.cpp +++ b/storage/mozStorageService.cpp @@ -292,27 +292,22 @@ Service::unregisterConnection(Connection break; } } } 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); + // Do not proxy the release anywhere, just let this reference drop here. (We + // previously did proxy the release, but that was because we invoked Close() + // in the destructor and Close() likes to complain if it's not invoked on the + // opener thread, so it was essential that the last reference be dropped on + // the opener thread. We now enqueue Close() inside our caller, Release(), so + // it doesn't actually matter what thread our reference drops on.) } void Service::getConnections(/* inout */ nsTArray<RefPtr<Connection> >& aConnections) { mRegistrationMutex.AssertNotCurrentThreadOwns(); MutexAutoLock mutex(mRegistrationMutex); aConnections.Clear();