Bug 1359887 - Potential deadlock when forcing wal checkpoints on Places startup. r=past
Don't enforce wal checkpoints during Places startup, since that can possibly cause a deadlock that would
make some early synchronous API calls bailout. That would indeed cause unexpected intermittent failures
in tests.
Regardless, forcing checkpoints like this in modern filesystems is unlikely to add much value, since
the probabilities to lose the whole contents of the journal are very low.
Additionally we have better startup handling of invalid databases, so we should be able to recover in any case.
MozReview-Commit-ID: G7nISZkd8s2
--- a/toolkit/components/places/Database.cpp
+++ b/toolkit/components/places/Database.cpp
@@ -333,17 +333,16 @@ SetupDurability(nsCOMPtr<mozIStorageConn
rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
"PRAGMA synchronous = OFF"));
NS_ENSURE_SUCCESS(rv, rv);
} else {
// Be sure to set journal mode after page_size. WAL would prevent the change
// otherwise.
if (JOURNAL_WAL == SetJournalMode(aDBConn, JOURNAL_WAL)) {
// Set the WAL journal size limit.
- // For added safety we will also force checkpointing at strategic moments.
int32_t checkpointPages =
static_cast<int32_t>(DATABASE_MAX_WAL_BYTES / aDBPageSize);
nsAutoCString checkpointPragma("PRAGMA wal_autocheckpoint = ");
checkpointPragma.AppendInt(checkpointPages);
rv = aDBConn->ExecuteSimpleSQL(checkpointPragma);
NS_ENSURE_SUCCESS(rv, rv);
}
else {
@@ -1170,18 +1169,16 @@ Database::InitSchema(bool* aDatabaseMigr
// Set the schema version to the current one.
rv = mMainConn->SetSchemaVersion(DATABASE_SCHEMA_VERSION);
NS_ENSURE_SUCCESS(rv, rv);
rv = transaction.Commit();
NS_ENSURE_SUCCESS(rv, rv);
- ForceWALCheckpoint();
-
// ANY FAILURE IN THIS METHOD WILL CAUSE US TO MARK THE DATABASE AS CORRUPT
// AND TRY TO REPLACE IT.
// DO NOT PUT HERE ANYTHING THAT IS NOT RELATED TO INITIALIZATION OR MODIFYING
// THE DISK DATABASE.
return NS_OK;
}
--- a/toolkit/components/places/FaviconHelpers.cpp
+++ b/toolkit/components/places/FaviconHelpers.cpp
@@ -1208,25 +1208,21 @@ FetchAndConvertUnsupportedPayloads::Run(
NS_ENSURE_SUCCESS(rv, rv);
if (count == 200) {
// There are more results to handle. Re-dispatch to the same thread for the
// next chunk.
return NS_DispatchToCurrentThread(this);
}
- // We're done. Remove any leftovers and force a checkpoint for safety.
+ // We're done. Remove any leftovers.
rv = mDB->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
"DELETE FROM moz_icons WHERE typeof(width) = 'text'"
));
NS_ENSURE_SUCCESS(rv, rv);
- rv = mDB->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
- "PRAGMA wal_checkpoint"
- ));
- NS_ENSURE_SUCCESS(rv, rv);
// Run a one-time VACUUM of places.sqlite, since we removed a lot from it.
// It may cause jank, but not doing it could cause dataloss due to expiration.
rv = mDB->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
"VACUUM"
));
NS_ENSURE_SUCCESS(rv, rv);
// Re-dispatch to the main-thread to flip the conversion pref.
--- a/toolkit/components/places/Helpers.cpp
+++ b/toolkit/components/places/Helpers.cpp
@@ -303,31 +303,16 @@ RoundToMilliseconds(PRTime aTime) {
return aTime - (aTime % PR_USEC_PER_MSEC);
}
PRTime
RoundedPRNow() {
return RoundToMilliseconds(PR_Now());
}
-void
-ForceWALCheckpoint()
-{
- RefPtr<Database> DB = Database::GetDatabase();
- if (DB) {
- nsCOMPtr<mozIStorageAsyncStatement> stmt = DB->GetAsyncStatement(
- "pragma wal_checkpoint "
- );
- if (stmt) {
- nsCOMPtr<mozIStoragePendingStatement> handle;
- (void)stmt->ExecuteAsync(nullptr, getter_AddRefs(handle));
- }
- }
-}
-
bool
GetHiddenState(bool aIsRedirect,
uint32_t aTransitionType)
{
return aTransitionType == nsINavHistoryService::TRANSITION_FRAMED_LINK ||
aTransitionType == nsINavHistoryService::TRANSITION_EMBED ||
aIsRedirect;
}
--- a/toolkit/components/places/Helpers.h
+++ b/toolkit/components/places/Helpers.h
@@ -211,24 +211,16 @@ public:
protected:
mozilla::storage::StatementCache<StatementType>& mStatementCache;
nsCOMPtr<nsISupports> mOwner;
nsCOMPtr<nsIThread> mCallingThread;
};
/**
- * Forces a WAL checkpoint. This will cause all transactions stored in the
- * journal file to be committed to the main database.
- *
- * @note The checkpoint will force a fsync/flush.
- */
-void ForceWALCheckpoint();
-
-/**
* Determines if a visit should be marked as hidden given its transition type
* and whether or not it was a redirect.
*
* @param aIsRedirect
* True if this visit was a redirect, false otherwise.
* @param aTransitionType
* The transition type of the visit.
* @return true if this visit should be hidden.