627490 - Bookmark sync: don't cache places IDs

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

kSpecialIDs caches places IDs for a few special items. They're unlikely to change for the places root and unsorted folder etc., but they can change for the mobile folder when you restore from backup.

Because cache invalidation is hard we should just not do it.

Comment 1

[Richard Newman [:rnewman]]

Note that restoring a bookmarks backup apparently also rewrites GUIDs.

Including special IDs.

Which means wiping fails:

  wipe: function BStore_wipe() {
    // Save a backup before clearing out all bookmarks
    archiveBookmarks();

    for (let [guid, id] in Iterator(kSpecialIds))
      if (guid != "places")
        this._bms.removeFolderChildren(id);
  }

For example:

Wipe failed: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.removeFolderChildren]
Stack trace: BStore_wipe()@resource://services-sync/engines/bookmarks.js:1223
           < test_restorePromptsReupload()@/Users/rnewman/moz/hg/services/fx-sync/services/sync/tests/unit/test_bookmark_engine.js:196
           < run_test()@/Users/rnewman/moz/hg/services/fx-sync/services/sync/tests/unit/test_bookmark_engine.js:214
           < _execute_test()@/Users/rnewman/moz/hg/mozilla-central/testing/xpcshell/head.js:322
           < -e:1

Fixing kSpecialIDs would solve this.

Comment 2

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Attached patch rewrites kSpecialIds and parts of the bookmarks engine that use it. There was a nasty mutual recursion case in child fetching that made the simpler solution impossible. Details:

* Make kSpecialIds a regular object with getters. Refactor the mobile accessor to allow fetching it without creation in some cases.

* Extend idForGUID to allow non-creating lookup of special IDs.

* Amend itemExists and _getChildren to use the amended idForGUID.

* Amend GUIDForId to "ask not tell".

* Add a test that alters the mobile record locally, ensuring that fetching the ID again yields a different value. Verified that this fails without changes. 

Both test suites and CrossWeave pass.

Comment 3

[Richard Newman [:rnewman]]

Note that this patch makes the creation of the Mobile Bookmarks folder much lazier -- simply enumerating special IDs no longer causes it to be created.

I need to verify whether this change b0rks sync with Fennec. I would hope that Fennec isn't expecting the Sync code to create its bookmarks root, but who knows...

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 505908 [details] [diff] [review]
Proposed patch. v1

I'm sure mconnor won't if I reviewed this...


>+  get ids()
>+    [this.menu, this.places, this.tags, this.toolbar, this.unfiled,
>+     this.mobile],

What is this needed for? Looks like it can be removed.

>+
>+  mobileAnno: "mobile/bookmarksRoot",

Please make this a module-level const like the other annotation names.

>+  findMobileRoot: function findMobileRoot(create) {
>+    // Use the (one) mobile root if it already exists.
>+    let root = Svc.Annos.getItemsWithAnnotation(this.mobileAnno, {});
>+    if (root.length != 0)
>+      return root[0];

(Btw, we'll have to handle the pathological case of two or more mobile roots here eventually (bug 627519))

>+  get menu()    Svc.Bookmark["bookmarksMenuFolder"],
>+  get places()  Svc.Bookmark["placesRoot"],
>+  get tags()    Svc.Bookmark["tagsFolder"],
>+  get toolbar() Svc.Bookmark["toolbarFolder"],
>+  get unfiled() Svc.Bookmark["unfiledBookmarksFolder"],

Nit: Use dot notation here (Svc.Bookmark.bookmarksMenuFolder)

>   _getChildren: function BStore_getChildren(guid, items) {

(Btw, we should really make this method use (async) SQL instead of the synchronous query API. Filed bug 628315.)

>+function test_ID_caching() {
>+  
>+  _("Ensure that Places IDs are not cached.");
>+  let engine = new BookmarksEngine();
>+  let store = engine._store;
>+  _("All IDs: " + JSON.stringify(store.getAllIDs()));
>+
>+  let mobileID = store.idForGUID("mobile");
>+  _("Change the GUID for that item, and drop the mobile anno.");
>+  store._setGUID(mobileID, "abcdefghijkl");
>+  Svc.Annos.removeItemAnnotation(mobileID, "mobile/bookmarksRoot");
>+  let err;
>+  let newMobileID;
>+  try {
>+    newMobileID = store.idForGUID("mobile");
>+  } catch (ex) {
>+    err = ex;
>+  }
>+  
>+  // Either lookup worked, and it's different, or it failed with an exception.

Why would it fail with an exception? This and the stanza below don't look right to me for a unit test. Either both code paths can happen, then we should test them individually. Or one of them can't and then we should fail hard if it does occur (e.g. the exception).

>+  _("New mobile ID: " + newMobileID);
>+  if (newMobileID) {
>+    do_check_neq(newMobileID, mobileID);
>+  } else {
>+    if (err)
>+      _("Error: " + Utils.exceptionStr(err));
>+    do_check_true(!!err);
>+  }

r=me with changes addresses. Should land in 1.6.x as well.

Comment 5

[Philipp von Weitershausen [:philikon]]

This should definitely block.

Comment 6

[Richard Newman [:rnewman]]

Pushed:

1.6.x:   http://hg.mozilla.org/services/fx-sync/rev/572b5c64a3bd
default: http://hg.mozilla.org/services/fx-sync/rev/e0c5c8fb8be2