commit ecf9614c118fdfa1008b0f445655f322f7be3c6b
parent 0a27214edf22bfa75d85241be829c233364f1fd5
Author: Marco Bonardo <mbonardo@mozilla.com>
Date: Fri, 3 Oct 2025 01:09:28 +0000
Bug 1986816 r=places-reviewers,Standard8
Differential Revision: https://phabricator.services.mozilla.com/D264337
Diffstat:
4 files changed, 90 insertions(+), 32 deletions(-)
diff --git a/toolkit/components/places/ConcurrentConnection.cpp b/toolkit/components/places/ConcurrentConnection.cpp
@@ -8,6 +8,7 @@
#include "Helpers.h"
#include "SQLFunctions.h"
+#include "MainThreadUtils.h"
#include "mozilla/AppShutdown.h"
#include "mozilla/Assertions.h"
#include "mozilla/ClearOnShutdown.h"
@@ -96,7 +97,17 @@ NS_IMPL_ISUPPORTS(ConcurrentConnection, nsIObserver, nsISupportsWeakReference,
ConcurrentConnection::ConcurrentConnection() {
MOZ_DIAGNOSTIC_ASSERT(XRE_IsParentProcess(),
"Can only instantiate in the parent process");
- MOZ_DIAGNOSTIC_ASSERT(NS_IsMainThread(), "Must be on the main-thread");
+ if (NS_IsMainThread()) {
+ InitializeOnMainThread();
+ } else {
+ NS_DispatchToMainThread(NewRunnableMethod(
+ "places::ConcurrentConnection::InitializeOnMainThread", this,
+ &ConcurrentConnection::InitializeOnMainThread));
+ }
+}
+
+void ConcurrentConnection::InitializeOnMainThread() {
+ AssertIsOnMainThread();
// Check shutdown and try to add this as a blocker.
nsCOMPtr<nsIAsyncShutdownService> asyncShutdownSvc =
@@ -135,7 +146,9 @@ ConcurrentConnection::ConcurrentConnection() {
Maybe<ConcurrentConnection*> ConcurrentConnection::GetInstance() {
static StaticRefPtr<ConcurrentConnection> sInstance;
- if (!sInstance &&
+ // TODO (Bug 1989632): make this properly thread-safe so it can be initialized
+ // on an helper thread.
+ if (!sInstance && NS_IsMainThread() &&
!AppShutdown::IsInOrBeyond(ShutdownPhase::AppShutdownTeardown)) {
sInstance = new ConcurrentConnection();
ClearOnShutdown(&sInstance, ShutdownPhase::AppShutdownTeardown);
@@ -171,7 +184,7 @@ ConcurrentConnection::GetState(nsIPropertyBag** _state) {
// nsIAsyncShutdownBlocker
NS_IMETHODIMP
ConcurrentConnection::BlockShutdown(nsIAsyncShutdownClient* aBarrierClient) {
- MOZ_ASSERT(NS_IsMainThread(), "Must be on the main-thread");
+ AssertIsOnMainThread();
mShutdownBarrierClient = aBarrierClient;
mState = AWAITING_DATABASE_CLOSED;
mIsShuttingDown = true;
@@ -190,11 +203,17 @@ ConcurrentConnection::BlockShutdown(nsIAsyncShutdownClient* aBarrierClient) {
// mozIStorageCompletionCallback
NS_IMETHODIMP
ConcurrentConnection::Complete(nsresult aRv, nsISupports* aData) {
- MOZ_ASSERT(NS_IsMainThread(), "Must be on main-thread");
+ AssertIsOnMainThread();
- // This is invoked only for connection opening.
+ // This is invoked as a consequence of connection opening, but the internal
+ // connection handle is not yet available, nor ready for consumption.
MOZ_ASSERT(!mConn);
- MOZ_ASSERT(!mIsConnectionReady);
+#ifdef DEBUG
+ mConnectionReadyMutex.NoteOnMainThread();
+ if (mIsConnectionReady) {
+ MOZ_CRASH("The connection should not be markes as ready yet");
+ }
+#endif
// It's possible we got shutdown while the connection was being opened. We
// don't even assign the connection, just try to close it.
@@ -297,8 +316,14 @@ NS_IMETHODIMP ConcurrentConnection::HandleCompletion(uint16_t aReason) {
}
void ConcurrentConnection::CloseConnection() {
- mIsConnectionReady = false;
- nsCOMPtr<mozIStorageAsyncConnection> conn = mConn.forget();
+ AssertIsOnMainThread();
+ nsCOMPtr<mozIStorageAsyncConnection> conn;
+ {
+ MutexAutoLock lock(mConnectionReadyMutex.Lock());
+ mConnectionReadyMutex.NoteExclusiveAccess();
+ mIsConnectionReady = false;
+ conn = mConn.forget();
+ }
if (mAsyncStatements) {
mAsyncStatements->FinalizeStatements();
@@ -322,6 +347,7 @@ void ConcurrentConnection::CloseConnection() {
}
void ConcurrentConnection::CloseConnectionComplete(nsresult rv) {
+ AssertIsOnMainThread();
if (mIsShuttingDown || NS_FAILED(rv)) {
Shutdown();
}
@@ -329,6 +355,7 @@ void ConcurrentConnection::CloseConnectionComplete(nsresult rv) {
void ConcurrentConnection::SetupConnection() {
MOZ_ASSERT(mConn, "Connection must be defined at this point");
+ AssertIsOnMainThread();
// Create common functions.
nsresult rv = Database::InitFunctions(mConn);
@@ -346,11 +373,15 @@ void ConcurrentConnection::SetupConnection() {
return;
}
- // Create the statements caches.
- mAsyncStatements = MakeUnique<AsyncStatementCache>(mConn);
- mHelperThreadStatements = MakeUnique<StatementCache>(mConn);
+ {
+ MutexAutoLock lock(mConnectionReadyMutex.Lock());
+ mConnectionReadyMutex.NoteExclusiveAccess();
+ // Create the statements caches.
+ mAsyncStatements = MakeUnique<AsyncStatementCache>(mConn);
+ mHelperThreadStatements = MakeUnique<StatementCache>(mConn);
+ mIsConnectionReady = true;
+ }
- mIsConnectionReady = true;
TryToConsumeQueues();
}
@@ -385,7 +416,7 @@ nsresult ConcurrentConnection::AttachDatabase(const nsString& aFileName,
NS_IMETHODIMP
ConcurrentConnection::Observe(nsISupports* aSubject, const char* aTopic,
const char16_t* aData) {
- MOZ_ASSERT(NS_IsMainThread());
+ AssertIsOnMainThread();
if (strcmp(aTopic, TOPIC_PLACES_INIT_COMPLETE) == 0) {
mPlacesIsInitialized = true;
TryToOpenConnection();
@@ -395,7 +426,7 @@ ConcurrentConnection::Observe(nsISupports* aSubject, const char* aTopic,
void ConcurrentConnection::Queue(const nsCString& aSQL,
PendingStatementCallback* aCallback) {
- MOZ_DIAGNOSTIC_ASSERT(NS_IsMainThread(), "Must be on the main-thread");
+ AssertIsOnMainThread();
if (mIsShuttingDown) {
return;
}
@@ -404,7 +435,7 @@ void ConcurrentConnection::Queue(const nsCString& aSQL,
}
void ConcurrentConnection::Queue(Runnable* aRunnable) {
- MOZ_DIAGNOSTIC_ASSERT(NS_IsMainThread(), "Must be on the main-thread");
+ AssertIsOnMainThread();
if (mIsShuttingDown) {
return;
}
@@ -418,6 +449,11 @@ ConcurrentConnection::GetStatementOnHelperThread(const nsCString& aQuery) {
MOZ_DIAGNOSTIC_CRASH("Use `GetStatement()` on the main-thread");
return nullptr;
}
+ MutexAutoLock lock(mConnectionReadyMutex.Lock());
+ mConnectionReadyMutex.NoteLockHeld();
+ if (!mIsConnectionReady) {
+ return nullptr;
+ }
nsCOMPtr<mozIStorageStatement> stmt =
mHelperThreadStatements->GetCachedStatement(aQuery);
if (stmt) {
@@ -428,6 +464,7 @@ ConcurrentConnection::GetStatementOnHelperThread(const nsCString& aQuery) {
already_AddRefed<mozIStorageAsyncStatement> ConcurrentConnection::GetStatement(
const nsCString& aQuery) {
+ AssertIsOnMainThread();
if (!NS_IsMainThread()) {
MOZ_DIAGNOSTIC_CRASH(
"Use `GetStatementOnHelperThread()` on the helper thread");
@@ -442,6 +479,8 @@ already_AddRefed<mozIStorageAsyncStatement> ConcurrentConnection::GetStatement(
}
void ConcurrentConnection::TryToConsumeQueues() {
+ AssertIsOnMainThread();
+ mConnectionReadyMutex.NoteOnMainThread();
if (!mConn || !mIsConnectionReady) {
return;
}
@@ -472,10 +511,14 @@ void ConcurrentConnection::TryToConsumeQueues() {
}
void ConcurrentConnection::TryToOpenConnection() {
- // This is invoked at different times, thus it may try to re-enter.
- if (mIsShuttingDown || mIsOpening || mIsConnectionReady) {
+ AssertIsOnMainThread();
+ mConnectionReadyMutex.NoteOnMainThread();
+ // Avoid re-entering as this may be invoked multiple times. mIsOpening is
+ // used until mConn is assigned.
+ if (mIsShuttingDown || mIsOpening || mConn) {
return;
}
+
mIsOpening = true;
// Any error here means we'll be unable to do anything, thus we just shutdown.
@@ -508,6 +551,14 @@ void ConcurrentConnection::TryToOpenConnection() {
void ConcurrentConnection::Shutdown() {
MOZ_ASSERT(!mConn, "Connection should have been closed");
+#ifdef DEBUG
+ AssertIsOnMainThread();
+ mConnectionReadyMutex.NoteOnMainThread();
+ if (mIsConnectionReady) {
+ MOZ_CRASH("Connection should be closed");
+ }
+#endif
+
mConn = nullptr;
mIsOpening = false;
mIsShuttingDown = true;
diff --git a/toolkit/components/places/ConcurrentConnection.h b/toolkit/components/places/ConcurrentConnection.h
@@ -5,6 +5,8 @@
#ifndef mozilla_places_ConcurrentConnection_h_
#define mozilla_places_ConcurrentConnection_h_
+#include "mozilla/EventTargetAndLockCapability.h"
+#include "mozilla/Mutex.h"
#include "mozilla/storage/StatementCache.h"
#include "mozIStorageCompletionCallback.h"
#include "mozIStorageStatementCallback.h"
@@ -92,6 +94,8 @@ class ConcurrentConnection final : public nsIObserver,
const nsCString& aQuery);
private:
+ void InitializeOnMainThread();
+
/**
* Gets a cached asynchronous statement on the main thread.
* This is private, as you normally should use Queue.
@@ -162,7 +166,13 @@ class ConcurrentConnection final : public nsIObserver,
bool mPlacesIsInitialized = false;
bool mRetryOpening = true;
bool mIsShuttingDown = false;
- bool mIsConnectionReady = false;
+
+ // mIsConnectionReady is only changed on the main-thread, though it may be
+ // read on the helper thread for which it needs a mutex.
+ MainThreadAndLockCapability<Mutex> mConnectionReadyMutex{
+ "ConcurrentConnection::mConnectionReadyMutex"};
+ bool mIsConnectionReady MOZ_GUARDED_BY(mConnectionReadyMutex) = false;
+
int32_t mSchemaVersion = -1;
// Ideally this should be a mozIStorageAsyncConnection, as that would give us
diff --git a/toolkit/components/places/FaviconHelpers.cpp b/toolkit/components/places/FaviconHelpers.cpp
@@ -136,14 +136,14 @@ nsresult FetchPageInfo(const RefPtr<Database>& aDB, PageData& _page) {
// The page is not bookmarked. Since updating the icon with a disabled
// history would be a privacy leak, bail out as if the page did not exist.
return NS_ERROR_NOT_AVAILABLE;
- } else {
- // The page, or a redirect to it, is bookmarked. If the bookmarked spec
- // is different from the requested one, use it.
- if (!_page.bookmarkedSpec.Equals(_page.spec)) {
- _page.spec = _page.bookmarkedSpec;
- rv = FetchPageInfo(aDB, _page);
- NS_ENSURE_SUCCESS(rv, rv);
- }
+ }
+
+ // The page, or a redirect to it, is bookmarked. If the bookmarked spec
+ // is different from the requested one, use it.
+ if (!_page.bookmarkedSpec.Equals(_page.spec)) {
+ _page.spec = _page.bookmarkedSpec;
+ rv = FetchPageInfo(aDB, _page);
+ NS_ENSURE_SUCCESS(rv, rv);
}
}
diff --git a/toolkit/components/places/FaviconHelpers.h b/toolkit/components/places/FaviconHelpers.h
@@ -270,17 +270,14 @@ class ConnectionAdapter {
: mDatabase(nullptr), mConcurrentConnection(aConn) {}
already_AddRefed<mozIStorageStatement> GetStatement(
-
const nsCString& aQuery) const {
MOZ_ASSERT(!NS_IsMainThread(), "Must be on helper thread");
if (mDatabase) {
return mDatabase->GetStatement(aQuery);
- } else if (mConcurrentConnection) {
- auto conn = mConcurrentConnection.get();
- if (conn) {
- return conn->GetStatementOnHelperThread(aQuery);
- }
+ }
+ if (mConcurrentConnection) {
+ return mConcurrentConnection->GetStatementOnHelperThread(aQuery);
}
return nullptr;
}
