Bug 1435446 - Use immediate transactions by default in Places. r?mak draft
authorKit Cambridge <kit@yakshaving.ninja>
Wed, 28 Feb 2018 23:32:57 -0800
changeset 766325 73ab3a654d510dcc1e4827e8060283590aca8b1d
parent 766324 93f2c5b7aaf1c182fb9115e7dc5a6f9caba1ef63
push id102282
push userbmo:kit@mozilla.com
push dateMon, 12 Mar 2018 17:42:48 +0000
reviewersmak
bugs1435446
milestone61.0a1
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);