Bug 1435446 - Use immediate transactions by default in Places. r=mak
authorKit Cambridge <kit@yakshaving.ninja>
Wed, 28 Feb 2018 23:32:57 -0800
changeset 407709 381fa7a9955cb647efe0043fe106adb85d11d4e7
parent 407708 400d6348abcaff1993820a95a946c99a7abc94c5
child 407710 af3eded235e596c936db723f7c71274680983126
push id33619
push usernbeleuzu@mozilla.com
push dateTue, 13 Mar 2018 09:58:16 +0000
treeherdermozilla-central@8f1b2f872f0e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1435446
milestone60.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 1435446 - Use immediate transactions by default in Places. r=mak MozReview-Commit-ID: L2rCMwqZZkQ
toolkit/components/places/Database.cpp
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/nsNavHistory.cpp
--- a/toolkit/components/places/Database.cpp
+++ b/toolkit/components/places/Database.cpp
@@ -981,37 +981,44 @@ Database::TryToCloneTablesFromCorruptDat
   return NS_OK;
 }
 
 nsresult
 Database::SetupDatabaseConnection(nsCOMPtr<mozIStorageService>& aStorage)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
+  // Using immediate transactions allows the main connection to retry writes
+  // that fail with `SQLITE_BUSY` because a cloned connection has locked the
+  // database for writing.
+  nsresult rv = mMainConn->SetDefaultTransactionType(
+    mozIStorageConnection::TRANSACTION_IMMEDIATE);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   // WARNING: any statement executed before setting the journal mode must be
   // finalized, since SQLite doesn't allow changing the journal mode if there
   // is any outstanding statement.
 
   {
     // Get the page size.  This may be different than the default if the
     // database file already existed with a different page size.
     nsCOMPtr<mozIStorageStatement> statement;
-    nsresult rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
+    rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
       MOZ_STORAGE_UNIQUIFY_QUERY_STR "PRAGMA page_size"
     ), getter_AddRefs(statement));
     NS_ENSURE_SUCCESS(rv, rv);
     bool hasResult = false;
     rv = statement->ExecuteStep(&hasResult);
     NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && hasResult, NS_ERROR_FILE_CORRUPTED);
     rv = statement->GetInt32(0, &mDBPageSize);
     NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && mDBPageSize > 0, NS_ERROR_FILE_CORRUPTED);
   }
 
   // Ensure that temp tables are held in memory, not on disk.
-  nsresult rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
+  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
     MOZ_STORAGE_UNIQUIFY_QUERY_STR "PRAGMA temp_store = MEMORY")
   );
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = SetupDurability(mMainConn, mDBPageSize);
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsAutoCString busyTimeoutPragma("PRAGMA busy_timeout = ");
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -2146,22 +2146,22 @@ PlacesUtils.keywords = {
                 cache.delete(oldKeyword);
               }
             }
 
             await db.executeCached(
               `INSERT INTO moz_keywords (keyword, place_id, post_data)
                VALUES (:keyword, (SELECT id FROM moz_places WHERE url_hash = hash(:url) AND url = :url), :post_data)
               `, { url: url.href, keyword, post_data: postData });
+
+            await PlacesSyncUtils.bookmarks.addSyncChangesForBookmarksWithURL(
+              db, url, PlacesSyncUtils.bookmarks.determineSyncChangeDelta(source));
           });
         }
 
-        await PlacesSyncUtils.bookmarks.addSyncChangesForBookmarksWithURL(
-          db, url, PlacesSyncUtils.bookmarks.determineSyncChangeDelta(source));
-
         cache.set(keyword, { keyword, url, postData: postData || null });
 
         // In any case, notify about the new keyword.
         await notifyKeywordChange(url.href, keyword, source);
       }
     );
   },
 
@@ -2190,21 +2190,23 @@ PlacesUtils.keywords = {
     keyword = keywordOrEntry.keyword.trim().toLowerCase();
     return PlacesUtils.withConnectionWrapper("PlacesUtils.keywords.remove", async db => {
       let cache = await promiseKeywordsCache();
       if (!cache.has(keyword))
         return;
       let { url } = cache.get(keyword);
       cache.delete(keyword);
 
-      await db.execute(`DELETE FROM moz_keywords WHERE keyword = :keyword`,
-                       { keyword });
+      await db.executeTransaction(async function() {
+        await db.execute(`DELETE FROM moz_keywords WHERE keyword = :keyword`,
+                         { keyword });
 
-      await PlacesSyncUtils.bookmarks.addSyncChangesForBookmarksWithURL(
-        db, url, PlacesSyncUtils.bookmarks.determineSyncChangeDelta(source));
+        await PlacesSyncUtils.bookmarks.addSyncChangesForBookmarksWithURL(
+          db, url, PlacesSyncUtils.bookmarks.determineSyncChangeDelta(source));
+      });
 
       // Notify bookmarks about the removal.
       await notifyKeywordChange(url.href, "", source);
     });
   },
 
   /**
    * Moves all (keyword, POST data) pairs from one URL to another, and fires
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -448,17 +448,17 @@ class SyncedBookmarksMirror {
       await this.db.execute(`DELETE FROM itemsChanged`);
       await this.db.execute(`DELETE FROM itemsRemoved`);
       await this.db.execute(`DELETE FROM itemsMoved`);
       await this.db.execute(`DELETE FROM annosChanged`);
       await this.db.execute(`DELETE FROM itemsToWeaklyReupload`);
       await this.db.execute(`DELETE FROM itemsToUpload`);
 
       return changeRecords;
-    }, this.db.TRANSACTION_IMMEDIATE);
+    });
 
     MirrorLog.debug("Replaying recorded observer notifications");
     try {
       await observersToNotify.notifyAll();
     } catch (ex) {
       MirrorLog.error("Error notifying Places observers", ex);
     }
 
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -2395,17 +2395,17 @@ nsNavHistory::GetObservers(uint32_t* _co
 }
 
 // See RunInBatchMode
 nsresult
 nsNavHistory::BeginUpdateBatch()
 {
   if (mBatchLevel++ == 0) {
     mBatchDBTransaction = new mozStorageTransaction(mDB->MainConn(), false,
-                                                    mozIStorageConnection::TRANSACTION_DEFERRED,
+                                                    mozIStorageConnection::TRANSACTION_DEFAULT,
                                                     true);
 
     NOTIFY_OBSERVERS(mCanNotify, mObservers, nsINavHistoryObserver, OnBeginUpdateBatch());
   }
   return NS_OK;
 }
 
 // nsNavHistory::EndUpdateBatch
@@ -2460,25 +2460,25 @@ nsNavHistory::GetHistoryDisabled(bool *_
 
 nsresult
 nsNavHistory::RemovePagesInternal(const nsCString& aPlaceIdsQueryString)
 {
   // Return early if there is nothing to delete.
   if (aPlaceIdsQueryString.IsEmpty())
     return NS_OK;
 
-  mozStorageTransaction transaction(mDB->MainConn(), false,
-                                    mozIStorageConnection::TRANSACTION_DEFERRED,
-                                    true);
-
-  // Delete all visits for the specified place ids.
   nsCOMPtr<mozIStorageConnection> conn = mDB->MainConn();
   if (!conn) {
     return NS_ERROR_UNEXPECTED;
   }
+  mozStorageTransaction transaction(conn, false,
+                                    mozIStorageConnection::TRANSACTION_DEFAULT,
+                                    true);
+
+  // Delete all visits for the specified place ids.
   nsresult rv = conn->ExecuteSimpleSQL(
     NS_LITERAL_CSTRING(
       "DELETE FROM moz_historyvisits WHERE place_id IN (") +
         aPlaceIdsQueryString +
         NS_LITERAL_CSTRING(")")
   );
   NS_ENSURE_SUCCESS(rv, rv);