Bug 1359887 - Potential deadlock when forcing wal checkpoints on Places startup. r=past draft
authorMarco Bonardo <mbonardo@mozilla.com>
Sat, 29 Apr 2017 16:49:35 +0200
changeset 571331 9ae86e7e7389483bdfd2603ca7c8f84668bd9a95
parent 569138 0b77ed3f26c5335503bc16e85b8c067382e7bb1e
child 626727 75450eb0a0c33f73d38ac86799fb78a5036bf1d8
push id56749
push usermak77@bonardo.net
push dateTue, 02 May 2017 11:22:16 +0000
reviewerspast
bugs1359887
milestone55.0a1
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
toolkit/components/places/Database.cpp
toolkit/components/places/FaviconHelpers.cpp
toolkit/components/places/Helpers.cpp
toolkit/components/places/Helpers.h
--- 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.