Bug 1359887 - Potential deadlock when forcing wal checkpoints on Places startup. r=past
authorMarco Bonardo <mbonardo@mozilla.com>
Sat, 29 Apr 2017 16:49:35 +0200
changeset 356014 f0b87912987658836237d7b3e3139fa9bae2dffa
parent 356013 9530e0ce7d55f0b22a7cd10aa332dbc8a2fc4b49
child 356015 ae079b9935e8c8fd58af02f7dd4085dcde9c6e6a
push id31754
push userkwierso@gmail.com
push dateWed, 03 May 2017 00:28:51 +0000
treeherdermozilla-central@5eaf2d70eded [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspast
bugs1359887
milestone55.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
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
@@ -1213,25 +1213,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.