If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Bookmark sync: don't cache places IDs

RESOLVED FIXED

Status

Cloud Services
Firefox Sync: Backend
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: philikon, Assigned: rnewman)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+, fennec2.0+)

Details

(Whiteboard: [has patch][has review] [qa-])

Attachments

(1 attachment)

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.
(Assignee)

Comment 1

7 years ago
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.
(Assignee)

Comment 2

7 years ago
Created attachment 505908 [details] [diff] [review]
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.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #505908 - Flags: review?(mconnor)
(Assignee)

Updated

7 years ago
Blocks: 626796
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][needs review mconnor]
(Assignee)

Comment 3

7 years ago
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 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.
Attachment #505908 - Flags: review?(mconnor) → review+
This should definitely block.
blocking2.0: --- → ?
tracking-fennec: --- → ?
Whiteboard: [has patch][needs review mconnor] → [has patch][has review]
(Assignee)

Comment 6

7 years ago
Pushed:

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

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Blocks: 627511

Updated

7 years ago
blocking2.0: ? → final+
tracking-fennec: ? → 2.0+

Updated

7 years ago
Whiteboard: [has patch][has review] → [has patch][has review] [qa-]
You need to log in before you can comment on or make changes to this bug.