Bug 1338668 - Ignore tag query folder IDs when comparing client and server bookmark records. r=tcsc draft
authorKit Cambridge <kit@yakshaving.ninja>
Fri, 10 Feb 2017 15:03:34 -0800
changeset 482093 e2030c91a20356d61677a212fa57f88d680bfb90
parent 481376 eea9995ed14c07675c972400e8ce36b3608c01b1
child 482095 b764c5f9c24d1486600f75763b5bd7544a30ed1a
push id45006
push userbmo:kit@mozilla.com
push dateFri, 10 Feb 2017 23:25:49 +0000
reviewerstcsc
bugs1338668
milestone54.0a1
Bug 1338668 - Ignore tag query folder IDs when comparing client and server bookmark records. r=tcsc MozReview-Commit-ID: 2Mz9fTsTou5
services/common/utils.js
services/sync/modules/bookmark_validator.js
services/sync/tests/unit/test_bookmark_validator.js
--- a/services/common/utils.js
+++ b/services/common/utils.js
@@ -65,16 +65,33 @@ this.CommonUtils = {
       if (!b.has(x)) {
         return false;
       }
     }
     return true;
   },
 
   /**
+   * Checks elements in two arrays for equality, as determined by the `===`
+   * operator. This function does not perform a deep comparison; see Sync's
+   * `Util.deepEquals` for that.
+   */
+  arrayEqual(a, b) {
+    if (a.length !== b.length) {
+      return false;
+    }
+    for (let i = 0; i < a.length; i++) {
+      if (a[i] !== b[i]) {
+        return false;
+      }
+    }
+    return true;
+  },
+
+  /**
    * Encode byte string as base64URL (RFC 4648).
    *
    * @param bytes
    *        (string) Raw byte string to encode.
    * @param pad
    *        (bool) Whether to include padding characters (=). Defaults
    *        to true for historical reasons.
    */
--- a/services/sync/modules/bookmark_validator.js
+++ b/services/sync/modules/bookmark_validator.js
@@ -1,33 +1,89 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
-const Cu = Components.utils;
+const { interfaces: Ci, utils: Cu } = Components;
 
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://gre/modules/PlacesSyncUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/Timer.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+Cu.import("resource://services-common/utils.js");
+
+Cu.importGlobalProperties(["URLSearchParams"]);
 
 this.EXPORTED_SYMBOLS = ["BookmarkValidator", "BookmarkProblemData"];
 
 const LEFT_PANE_ROOT_ANNO = "PlacesOrganizer/OrganizerFolder";
 const LEFT_PANE_QUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
+const QUERY_PROTOCOL = "place:";
 
 // Indicates if a local bookmark tree node should be excluded from syncing.
 function isNodeIgnored(treeNode) {
   return treeNode.annos && treeNode.annos.some(anno => anno.name == LEFT_PANE_ROOT_ANNO ||
                                                        anno.name == LEFT_PANE_QUERY_ANNO);
 }
+
+function areURLsEqual(a, b) {
+  if (a === b) {
+    return true;
+  }
+  if (a.startsWith(QUERY_PROTOCOL) != b.startsWith(QUERY_PROTOCOL)) {
+    return false;
+  }
+  // Tag queries are special because we rewrite them to point to the
+  // local tag folder ID. It's expected that the folders won't match,
+  // but all other params should.
+  let aParams = new URLSearchParams(a.slice(QUERY_PROTOCOL.length));
+  let aType = +aParams.get("type");
+  if (aType != Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_CONTENTS) {
+    return false;
+  }
+  let bParams = new URLSearchParams(b.slice(QUERY_PROTOCOL.length));
+  let bType = +bParams.get("type");
+  if (bType != Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_CONTENTS) {
+    return false;
+  }
+  let aKeys = new Set(aParams.keys());
+  let bKeys = new Set(bParams.keys());
+  if (aKeys.size != bKeys.size) {
+    return false;
+  }
+  // Tag queries shouldn't reference multiple folders, or named folders like
+  // "TOOLBAR" or "BOOKMARKS_MENU". Just in case, we make sure all folder IDs
+  // are numeric. If they are, we ignore them when comparing the query params.
+  if (aKeys.has("folder") && aParams.getAll("folder").every(isFinite)) {
+    aKeys.delete("folder");
+  }
+  if (bKeys.has("folder") && bParams.getAll("folder").every(isFinite)) {
+    bKeys.delete("folder");
+  }
+  for (let key of aKeys) {
+    if (!bKeys.has(key)) {
+      return false;
+    }
+    if (!CommonUtils.arrayEqual(aParams.getAll(key).sort(),
+                                bParams.getAll(key).sort())) {
+      return false;
+    }
+  }
+  for (let key of bKeys) {
+    if (!aKeys.has(key)) {
+      return false;
+    }
+  }
+  return true;
+}
+
 const BOOKMARK_VALIDATOR_VERSION = 1;
 
 /**
  * Result of bookmark validation. Contains the following fields which describe
  * server-side problems unless otherwise specified.
  *
  * - missingIDs (number): # of objects with missing ids
  * - duplicates (array of ids): ids seen more than once
@@ -163,17 +219,17 @@ XPCOMUtils.defineLazyGetter(this, "SYNCE
   PlacesUtils.bookmarks.unfiledGuid,
   PlacesUtils.bookmarks.mobileGuid,
 ]);
 
 class BookmarkValidator {
 
   _followQueries(recordMap) {
     for (let [guid, entry] of recordMap) {
-      if (entry.type !== "query" && (!entry.bmkUri || !entry.bmkUri.startsWith("place:"))) {
+      if (entry.type !== "query" && (!entry.bmkUri || !entry.bmkUri.startsWith(QUERY_PROTOCOL))) {
         continue;
       }
       // Might be worth trying to parse the place: query instead so that this
       // works "automatically" with things like aboutsync.
       let queryNodeParent = PlacesUtils.getFolderContents(entry, false, true);
       if (!queryNodeParent || !queryNodeParent.root.hasChildren) {
         continue;
       }
@@ -233,17 +289,17 @@ class BookmarkValidator {
       }
       let guid = PlacesSyncUtils.bookmarks.guidToSyncId(treeNode.guid);
       let itemType = "item";
       treeNode.ignored = !synced;
       treeNode.id = guid;
       switch (treeNode.type) {
         case PlacesUtils.TYPE_X_MOZ_PLACE:
           let query = null;
-          if (treeNode.annos && treeNode.uri.startsWith("place:")) {
+          if (treeNode.annos && treeNode.uri.startsWith(QUERY_PROTOCOL)) {
             query = treeNode.annos.find(({name}) =>
               name === PlacesSyncUtils.bookmarks.SMART_BOOKMARKS_ANNO);
           }
           if (query && query.value) {
             itemType = "query";
           } else {
             itemType = "bookmark";
           }
@@ -680,38 +736,38 @@ class BookmarkValidator {
 
       if (client.parentid || server.parentid) {
         if (client.parentid !== server.parentid) {
           structuralDifferences.push("parentid");
         }
       }
 
       if (client.tags || server.tags) {
-        let cl = client.tags || [];
-        let sl = server.tags || [];
-        if (cl.length !== sl.length || !cl.every((tag, i) => sl.indexOf(tag) >= 0)) {
+        let cl = client.tags ? [...client.tags].sort() : [];
+        let sl = server.tags ? [...server.tags].sort() : [];
+        if (!CommonUtils.arrayEqual(cl, sl)) {
           differences.push("tags");
         }
       }
 
       let sameType = client.type === server.type;
       if (!sameType) {
-        if (server.type === "query" && client.type === "bookmark" && client.bmkUri.startsWith("place:")) {
+        if (server.type === "query" && client.type === "bookmark" && client.bmkUri.startsWith(QUERY_PROTOCOL)) {
           sameType = true;
         }
       }
 
 
       if (!sameType) {
         differences.push("type");
       } else {
         switch (server.type) {
           case "bookmark":
           case "query":
-            if (server.bmkUri !== client.bmkUri) {
+            if (!areURLsEqual(server.bmkUri, client.bmkUri)) {
               differences.push("bmkUri");
             }
             break;
           case "livemark":
             if (server.feedUri != client.feedUri) {
               differences.push("feedUri");
             }
             if (server.siteUri != client.siteUri) {
@@ -722,17 +778,17 @@ class BookmarkValidator {
             if (server.id === "places" && !problemData.rootOnServer) {
               // It's the fabricated places root. It won't have the GUIDs, but
               // it doesn't matter.
               break;
             }
             if (client.childGUIDs || server.childGUIDs) {
               let cl = client.childGUIDs || [];
               let sl = server.childGUIDs || [];
-              if (cl.length !== sl.length || !cl.every((id, i) => sl[i] === id)) {
+              if (!CommonUtils.arrayEqual(cl, sl)) {
                 structuralDifferences.push("childGUIDs");
               }
             }
             break;
         }
       }
 
       if (differences.length) {
--- a/services/sync/tests/unit/test_bookmark_validator.js
+++ b/services/sync/tests/unit/test_bookmark_validator.js
@@ -226,16 +226,54 @@ add_task(async function test_cswc_differ
     server[2].type = "bookmark";
     let c = (await compareServerWithClient(server, client)).problemData;
     equal(c.clientMissing.length, 0);
     equal(c.serverMissing.length, 0);
     deepEqual(c.differences, [{id: "cccccccccccc", differences: ["type"]}]);
   }
 });
 
+add_task(async function test_cswc_differentURLs() {
+  let {server, client} = getDummyServerAndClient();
+  client.children[0].children.push({
+    guid: "dddddddddddd",
+    title: "Tag query",
+    "type": "text/x-moz-place",
+    "uri": "place:type=7&folder=80",
+  }, {
+    guid: "eeeeeeeeeeee",
+    title: "Firefox",
+    "type": "text/x-moz-place",
+    "uri": "http://getfirefox.com",
+  });
+  server.push({
+    id: "dddddddddddd",
+    parentid: "menu",
+    parentName: "foo",
+    title: "Tag query",
+    type: "query",
+    folderName: "taggy",
+    bmkUri: "place:type=7&folder=90",
+  }, {
+    id: "eeeeeeeeeeee",
+    parentid: "menu",
+    parentName: "foo",
+    title: "Firefox",
+    type: "bookmark",
+    bmkUri: "https://mozilla.org/firefox",
+  });
+
+  let c = (await compareServerWithClient(server, client)).problemData;
+  equal(c.differences.length, 1);
+  deepEqual(c.differences, [{
+    id: "eeeeeeeeeeee",
+    differences: ["bmkUri"],
+  }]);
+});
+
 add_task(async function test_cswc_serverUnexpected() {
   let {server, client} = getDummyServerAndClient();
   client.children.push({
     "guid": "dddddddddddd",
     "title": "",
     "id": 2000,
     "annos": [{
       "name": "places/excludeFromBackup",