Bug 1310525 - don't exceed sqlite's max variable limit when many changed bookmarks exist. r=kitcambridge a=gchang
authorMark Hammond <mhammond@skippinet.com.au>
Mon, 17 Oct 2016 12:54:05 +1100
changeset 356182 49dfb63c28b9c093f13391b999de40b01f125613
parent 356181 ef8ca045351940ed9ef04a4eff500b4df05efd0f
child 356183 c91317b095bd760c0e50ec1b14249e3306974315
push id6570
push userraliiev@mozilla.com
push dateMon, 14 Nov 2016 12:26:13 +0000
treeherdermozilla-beta@f455459b2ae5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskitcambridge, gchang
bugs1310525
milestone51.0a2
Bug 1310525 - don't exceed sqlite's max variable limit when many changed bookmarks exist. r=kitcambridge a=gchang MozReview-Commit-ID: H2l2L9fZm9t
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_tracker.js
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -449,18 +449,18 @@ BookmarksEngine.prototype = {
 
     // Filter out tags, organizer queries, and other descendants that we're
     // not tracking. We chunk `modifiedGUIDs` because SQLite limits the number
     // of bound parameters per query.
     for (let startIndex = 0;
          startIndex < modifiedGUIDs.length;
          startIndex += SQLITE_MAX_VARIABLE_NUMBER) {
 
-      let chunkLength = Math.min(startIndex + SQLITE_MAX_VARIABLE_NUMBER,
-                                 modifiedGUIDs.length);
+      let chunkLength = Math.min(SQLITE_MAX_VARIABLE_NUMBER,
+                                 modifiedGUIDs.length - startIndex);
 
       let query = `
         WITH RECURSIVE
         modifiedGuids(guid) AS (
           VALUES ${new Array(chunkLength).fill("(?)").join(", ")}
         ),
         syncedItems(id) AS (
           VALUES ${getChangeRootIds().map(id => `(${id})`).join(", ")}
--- a/services/sync/tests/unit/test_bookmark_tracker.js
+++ b/services/sync/tests/unit/test_bookmark_tracker.js
@@ -168,16 +168,43 @@ add_task(function* test_nested_batch_tra
   }, null);
 
   // Out of both batches - tracker should be the same, but score should be up.
   yield verifyTrackedCount(2);
   do_check_eq(tracker.score, SCORE_INCREMENT_XLARGE);
   yield cleanup();
 });
 
+add_task(function* test_tracker_sql_batching() {
+  _("Test tracker does the correct thing when it is forced to batch SQL queries");
+
+  const SQLITE_MAX_VARIABLE_NUMBER = 999;
+  let numItems = SQLITE_MAX_VARIABLE_NUMBER * 2 + 10;
+  let createdIDs = [];
+
+  yield startTracking();
+
+  PlacesUtils.bookmarks.runInBatchMode({
+    runBatched: function() {
+      for (let i = 0; i < numItems; i++) {
+        let syncBmkID = PlacesUtils.bookmarks.insertBookmark(
+                          PlacesUtils.bookmarks.unfiledBookmarksFolder,
+                          Utils.makeURI("https://example.org/" + i),
+                          PlacesUtils.bookmarks.DEFAULT_INDEX,
+                          "Sync Bookmark " + i);
+        createdIDs.push(syncBmkID);
+      }
+    }
+  }, null);
+
+  do_check_eq(createdIDs.length, numItems);
+  yield verifyTrackedCount(numItems + 1); // the folder is also tracked.
+  yield cleanup();
+});
+
 add_task(function* test_onItemAdded() {
   _("Items inserted via the synchronous bookmarks API should be tracked");
 
   try {
     yield startTracking();
 
     _("Insert a folder using the sync API");
     let syncFolderID = PlacesUtils.bookmarks.createFolder(