Bug 1309774 - Part 1. Sync bookmark validator should only expect to see children of roots on server. r?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Thu, 20 Oct 2016 15:17:33 -0400
changeset 432302 a83c0a54edfcfdcbd2ab82ca7e739866bc4f20a2
parent 432301 4cde141bbedfd7355bdf3e9ac41740fb8fa2f06d
child 432303 ff0f3ee01bc41178dc1f91e3b1da5060f18b2ca7
push id34245
push userbmo:tchiovoloni@mozilla.com
push dateTue, 01 Nov 2016 15:46:57 +0000
reviewersmarkh
bugs1309774
milestone52.0a1
Bug 1309774 - Part 1. Sync bookmark validator should only expect to see children of roots on server. r?markh MozReview-Commit-ID: EEoDmsnBraA
services/sync/modules/bookmark_validator.js
services/sync/tests/unit/test_bookmark_validator.js
toolkit/components/places/PlacesSyncUtils.jsm
--- a/services/sync/modules/bookmark_validator.js
+++ b/services/sync/modules/bookmark_validator.js
@@ -4,16 +4,18 @@
 
 "use strict";
 
 const Cu = Components.utils;
 
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://gre/modules/PlacesSyncUtils.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+
 
 this.EXPORTED_SYMBOLS = ["BookmarkValidator", "BookmarkProblemData"];
 
 const LEFT_PANE_ROOT_ANNO = "PlacesOrganizer/OrganizerFolder";
 const LEFT_PANE_QUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
 
 // Indicates if a local bookmark tree node should be excluded from syncing.
 function isNodeIgnored(treeNode) {
@@ -147,16 +149,24 @@ class BookmarkProblemData {
     if (full) {
       let structural = this._summarizeDifferences("sdiff", this.structuralDifferences);
       result.push.apply(result, structural);
     }
     return result;
   }
 }
 
+// Defined lazily to avoid initializing PlacesUtils.bookmarks too soon.
+XPCOMUtils.defineLazyGetter(this, "SYNCED_ROOTS", () => [
+  PlacesUtils.bookmarks.menuGuid,
+  PlacesUtils.bookmarks.toolbarGuid,
+  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:"))) {
         continue;
       }
       // Might be worth trying to parse the place: query instead so that this
@@ -203,20 +213,26 @@ class BookmarkValidator {
     }
   }
 
   createClientRecordsFromTree(clientTree) {
     // Iterate over the treeNode, converting it to something more similar to what
     // the server stores.
     let records = [];
     let recordsByGuid = new Map();
-    function traverse(treeNode) {
+    let syncedRoots = SYNCED_ROOTS;
+    function traverse(treeNode, synced) {
+      if (!synced) {
+        synced = syncedRoots.includes(treeNode.guid);
+      } else if (isNodeIgnored(treeNode)) {
+        synced = false;
+      }
       let guid = PlacesSyncUtils.bookmarks.guidToSyncId(treeNode.guid);
       let itemType = 'item';
-      treeNode.ignored = isNodeIgnored(treeNode);
+      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:")) {
             query = treeNode.annos.find(({name}) =>
               name === PlacesSyncUtils.bookmarks.SMART_BOOKMARKS_ANNO);
           }
@@ -258,24 +274,24 @@ class BookmarkValidator {
       // We want to use the "real" guid here.
       recordsByGuid.set(treeNode.guid, treeNode);
       if (treeNode.type === 'folder') {
         treeNode.childGUIDs = [];
         if (!treeNode.children) {
           treeNode.children = [];
         }
         for (let child of treeNode.children) {
-          traverse(child);
+          traverse(child, synced);
           child.parent = treeNode;
           child.parentid = guid;
           treeNode.childGUIDs.push(child.guid);
         }
       }
     }
-    traverse(clientTree);
+    traverse(clientTree, false);
     clientTree.id = 'places';
     this._followQueries(recordsByGuid);
     return records;
   }
 
   /**
    * Process the server-side list. Mainly this builds the records into a tree,
    * but it also records information about problems, and produces arrays of the
@@ -363,17 +379,17 @@ class BookmarkValidator {
       }
     }
 
     let root = idToRecord.get('places');
 
     if (!root) {
       // Fabricate a root. We want to remember that it's fake so that we can
       // avoid complaining about stuff like it missing it's childGUIDs later.
-      root = { id: 'places', children: [], type: 'folder', title: '' };
+      root = { id: 'places', children: [], type: 'folder', title: '', fake: true };
       resultRecords.push(root);
       idToRecord.set('places', root);
     } else {
       problemData.rootOnServer = true;
     }
 
     // Build the tree, find orphans, and record most problems having to do with
     // the tree structure.
@@ -569,23 +585,17 @@ class BookmarkValidator {
     }
 
     return cycles;
   }
 
   // Perform client-side sanity checking that doesn't involve server data
   _validateClient(problemData, clientRecords) {
     problemData.clientCycles = this._detectCycles(clientRecords);
-    const rootsToCheck = [
-      PlacesUtils.bookmarks.menuGuid,
-      PlacesUtils.bookmarks.toolbarGuid,
-      PlacesUtils.bookmarks.unfiledGuid,
-      PlacesUtils.bookmarks.mobileGuid,
-    ];
-    for (let rootGUID of rootsToCheck) {
+    for (let rootGUID of SYNCED_ROOTS) {
       let record = clientRecords.find(record =>
         record.guid === rootGUID);
       if (!record || record.parentid !== "places") {
         problemData.badClientRoots.push(rootGUID);
       }
     }
   }
 
@@ -612,16 +622,19 @@ class BookmarkValidator {
     this._validateClient(problemData, clientRecords);
 
     let matches = [];
 
     let allRecords = new Map();
     let serverDeletedLookup = new Set(inspectionInfo.deletedRecords.map(r => r.id));
 
     for (let sr of serverRecords) {
+      if (sr.fake) {
+        continue;
+      }
       allRecords.set(sr.id, {client: null, server: sr});
     }
 
     for (let cr of clientRecords) {
       let unified = allRecords.get(cr.id);
       if (!unified) {
         allRecords.set(cr.id, {client: cr, server: null});
       } else {
--- a/services/sync/tests/unit/test_bookmark_validator.js
+++ b/services/sync/tests/unit/test_bookmark_validator.js
@@ -114,49 +114,49 @@ add_test(function test_isr_duplicateChil
   run_next_test();
 });
 
 // Each compareServerWithClient test mutates these, so we can't just keep them
 // global
 function getDummyServerAndClient() {
   let server = [
     {
-      id: 'aaaaaaaaaaaa',
+      id: 'menu',
       parentid: 'places',
       type: 'folder',
       parentName: '',
       title: 'foo',
       children: ['bbbbbbbbbbbb', 'cccccccccccc']
     },
     {
       id: 'bbbbbbbbbbbb',
       type: 'bookmark',
-      parentid: 'aaaaaaaaaaaa',
+      parentid: 'menu',
       parentName: 'foo',
       title: 'bar',
       bmkUri: 'http://baz.com'
     },
     {
       id: 'cccccccccccc',
-      parentid: 'aaaaaaaaaaaa',
+      parentid: 'menu',
       parentName: 'foo',
       title: '',
       type: 'query',
       bmkUri: 'place:type=6&sort=14&maxResults=10'
     }
   ];
 
   let client = {
     "guid": "root________",
     "title": "",
     "id": 1,
     "type": "text/x-moz-place-container",
     "children": [
       {
-        "guid": "aaaaaaaaaaaa",
+        "guid": "menu________",
         "title": "foo",
         "id": 1000,
         "type": "text/x-moz-place-container",
         "children": [
           {
             "guid": "bbbbbbbbbbbb",
             "title": "bar",
             "id": 1001,
@@ -198,28 +198,28 @@ add_test(function test_cswc_serverMissin
   let {server, client} = getDummyServerAndClient();
   // remove c
   server.pop();
   server[0].children.pop();
 
   let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
   deepEqual(c.serverMissing, ['cccccccccccc']);
   equal(c.clientMissing.length, 0);
-  deepEqual(c.structuralDifferences, [{id: 'aaaaaaaaaaaa', differences: ['childGUIDs']}]);
+  deepEqual(c.structuralDifferences, [{id: 'menu', differences: ['childGUIDs']}]);
   run_next_test();
 });
 
 add_test(function test_cswc_clientMissing() {
   let {server, client} = getDummyServerAndClient();
   client.children[0].children.pop();
 
   let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
   deepEqual(c.clientMissing, ['cccccccccccc']);
   equal(c.serverMissing.length, 0);
-  deepEqual(c.structuralDifferences, [{id: 'aaaaaaaaaaaa', differences: ['childGUIDs']}]);
+  deepEqual(c.structuralDifferences, [{id: 'menu', differences: ['childGUIDs']}]);
   run_next_test();
 });
 
 add_test(function test_cswc_differences() {
   {
     let {server, client} = getDummyServerAndClient();
     client.children[0].children[0].title = 'asdf';
     let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
@@ -326,17 +326,17 @@ add_task(function *test_telemetry_integr
   let bme = ping.engines.find(e => e.name === "bookmarks");
   ok(bme);
   ok(bme.validation);
   ok(bme.validation.problems)
   equal(bme.validation.checked, server.length);
   equal(bme.validation.took, duration);
   bme.validation.problems.sort((a, b) => String.localeCompare(a.name, b.name));
   deepEqual(bme.validation.problems, [
-    { name: "badClientRoots", count: 4 },
+    { name: "badClientRoots", count: 3 },
     { name: "sdiff:childGUIDs", count: 1 },
     { name: "serverMissing", count: 1 },
     { name: "structuralDifferences", count: 1 },
   ]);
 });
 
 function run_test() {
   run_next_test();
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -337,17 +337,16 @@ const BookmarkSyncUtils = PlacesSyncUtil
     return PlacesUtils.bookmarks.fetch(guid)
     .then(item => {
       if (!item) {
         return null;
       }
       return getKindForItem(item)
     });
   },
-
 });
 
 XPCOMUtils.defineLazyGetter(this, "BookmarkSyncLog", () => {
   return Log.repository.getLogger("BookmarkSyncUtils");
 });
 
 function validateSyncBookmarkObject(input, behavior) {
   return PlacesUtils.validateItemProperties(