626796 - Bookmark sync: restore from backup should trigger reupload of all bookmarks

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

Right now you have to manually choose Reset Sync to update other clients after a bookmark restore from backup. Also, that resets other engines which isn't necessary.

Comment 1

[Philipp von Weitershausen [:philikon]]

We want to call resetClient() here when the "bookmarks-restore-success" observer notification is triggered.

Comment 2

[Philipp von Weitershausen [:philikon]]

Hmm, we already do this:

    Svc.Obs.add("bookmarks-restore-success", function() {
      this._log.debug("Tracking all items on successful import");
      this._tracker.ignoreAll = false;

      // Mark all the items as changed so they get uploaded
      for (let id in this._store.getAllIDs())
        this._tracker.addChangedID(id);
    }, this);

I wonder why we don't simply call resetClient() here.

Comment 3

[Philipp von Weitershausen [:philikon]]

If already implemented or not, we should write tests for this to make sure we do the Right Thing(tm) on restore-from-backup.

Comment 4

[Richard Newman [:rnewman]]

Marking this as depending on Bug 627490. getAllIDs() currently can return stale values after a restore, which makes the tracking useless.

There's probably more to it, too -- AFAICS there's no logic to wipe bookmarks that were deleted entirely... still testing.

Comment 5

[Richard Newman [:rnewman]]

QA steps, before I attach a patch:

* Set up Profile A. Get bookmarks in known state S1. Make a backup by exporting bookmarks to JSON.

* Make significant changes to bookmarks: add and delete, including folders. Call this known state S2.

* Set up Profile B, syncing with A. Verify that both A and B are in S2.

* Make some changes in B. Call this S3. Verify that both A and B are in S3 after syncing.

* Restore the backup on A. Verify that A is in state S1: no changes from S2 or S3 should be present.

* Sync A, then B. Verify that B is now also in state S1.

* Add a third profile, C. Verify that it ends up in state S1 after first sync.

* Verify that changes made in B and C propagate to A. [i.e., client records aren't screwed up].

Extra points if you verify the behavior when you quit A *immediately* after the restore (i.e., without syncing).

Comment 6

[Richard Newman [:rnewman]]

Attachment: Proposed patch, v1

Here's a stab at this.

We can't just call resetClient: we have to make sure the server gets wiped, and in any case BookmarksEngine doesn't implement resetClient.

This patch, which sits on top of Bug 627490:

* Adds a test which:
  * Adds a bookmark in a folder
  * Backs up
  * Changes local bookmarks
  * Syncs
  * Verifies server contents
  * Restores the backup
  * Verifies local contents
  * Verifies that the engine wants to wipe the server
  * Syncs
  * Verifies that the server now matches up to the local contents.

* Extends ServerCollection to support adding arbitrary WBOs over HTTP. This is necessary for when the GUIDs keep changing, and is useful functionality to have anyway! Also a little bit more logging.

* Fixes trailing whitespace from the patch in Bug 627490, currently pending review. Oops.

* Adds to the bookmarks-restore-success handler, setting the bookmarks.wasRestoredFromBackup pref. BookmarksEngine._syncStartup inspects this pref and wipes the bookmarks folder on the server, as well as skipping the customary first-sync backup if we just restored.

* Both _syncStartup and the newly added _resetClient clear the restore flag.

Tests and CrossWeave pass.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 506014 [details] [diff] [review]
Proposed patch, v1

Goddamn input focus flipped out and hit Submit.

Comment 8

[Richard Newman [:rnewman]]

QA steps verified (apart from third profile) on current Minefield with patch installed as add-on.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

(Happy for either of you to review this, philiKON/mconnor.)

This version uses observers to wipe the server, avoiding a dependency on main.js.

I also factored out the pref handling.

Comment 10

[Philipp von Weitershausen [:philikon]]

Comment on attachment 506515 [details] [diff] [review]
Proposed patch. v2

>+  get restoredPref() this.name + ".wasRestoredFromBackup",
>+  
>+  get restoredFlag() {
>+    return Svc.Prefs.get(this.restoredPref, false);
>+  },
>+  
>+  set restoredFlag(value) {
>+    if (value == void 0) {   // For neatness, remove pref rather than false.

void 0? WTF? My eyes! If you want to use a nil value, use null. But in this case you can just ask for falsiness.

>+      this._log.debug("Clearing " + this.name + " restore flag.");
>+      Svc.Prefs.resetBranch(this.restoredPref);

You want to use

  Svc.Prefs.reset(this.restoredPref);

here. resetBranch() resets a bunch of preferences starting with the given name.

>+      return;
>+    }
>+    this._log.debug("Setting " + this.name + " restore flag.");
>+    Svc.Prefs.set(this.restoredPref, value);
>+  },

All that said, I don't think we need this pref at all. See below.

>   _handleImport: function _handleImport() {
>     Svc.Obs.add("bookmarks-restore-begin", function() {
>       this._log.debug("Ignoring changes from importing bookmarks");
>       this._tracker.ignoreAll = true;
>     }, this);
> 
>     Svc.Obs.add("bookmarks-restore-success", function() {
>       this._log.debug("Tracking all items on successful import");
>       this._tracker.ignoreAll = false;
> 
>-      // Mark all the items as changed so they get uploaded
>+      // Mark all the items as changed so they get uploaded.
>       for (let id in this._store.getAllIDs())
>         this._tracker.addChangedID(id);

I think we can get rid of this loop if we just did

  this.resetClient();

That will reset the lastSync timestamp and then _syncStartup() will do this for us! (Contrary to your comment 6, *all* engines have resetClient(). Inheritance FTW!)

>+    
>+      this._log.debug("Restore succeeded: setting flag to wipe server on next sync.");
>+      this.restoredFlag = true;

We don't need this flag. Just queue a wipeEngine command for the next sync:

  Weave.Service.prepCommand("wipeEngine", [this.name]);

Yes, this requires us to lazily circular-import service.js here. It's not ideal, I understand why mconnor doesn't like it. But using an observer when you really mean to call a single specific method isn't a better solution IMHO. If anything it makes our code harder to read. Really, prepCommand just needs to live in clients.js so it can be used without importing service.js. But that's for another day.

>+  _resetClient: function _resetClient() {
>+    this.restoredFlag = void 0;
>+  },

Erm, you're overwriting SyncEngine._resetSync here. I also don't understand the rationale for clearing the flag in _resetClient... Anyway, since we don't need the flag we don't need this change, yay! (But I won't forgive you for hurting my eyes TWICE with that void 0 ;))

>   _syncStartup: function _syncStart() {
>     SyncEngine.prototype._syncStartup.call(this);
> 
>     // For first-syncs, make a backup for the user to restore
>-    if (this.lastSync == 0)
>-      archiveBookmarks();
>+    if (this.restoredFlag) {
>+      this._log.debug("Wiping server after restore.");
>+      // Need to wipe the server if we just restored from a backup.
>+      // Yes, if you restore from backup on two separate profiles before
>+      // syncing either, they'll race. Don't do that.
>+      Svc.Obs.notify("weave:engine:should-wipe", "", this.name);

Wiping here is too late IMHO. Clients sync already happened way earlier (it's the first engine that's synced), now we're syncing the clients engine *again* within the bookmarks engine? We should just queue the remote wipe command earlier, when we receive the "bookmarks-restore-success" notification (see above).

>+      this.restoredFlag = void 0;
>+    }
>+    else {
>+      // For first-syncs, make a backup for the user to restore. No point doing
>+      // this if we just restored from a backup.

I see now why you had _resetClient reset restoredFlag. But since your "bookmarks-restore-success" observer didn't call resetClient, lastSync wasn't set to 0. So the if-branch below wouldn't be traversed. Aaaaanyway, no flag, no need for this. Let's always make a backup, the few cases where people just restored from backup we can easily neglect.


>diff --git a/services/sync/modules/service.js b/services/sync/modules/service.js
>--- a/services/sync/modules/service.js
>+++ b/services/sync/modules/service.js

Based on what I proposed above, we shouldn't need any changes to service.js.


>diff --git a/services/sync/tests/unit/head_http_server.js b/services/sync/tests/unit/head_http_server.js
>--- a/services/sync/tests/unit/head_http_server.js
>+++ b/services/sync/tests/unit/head_http_server.js
...
>     // This will count records where we have an existing ServerWBO
>     // registered with us as successful and all other records as failed.
>     for each (let record in input) {
>       let wbo = this.wbos[record.id];
>+      if (!wbo && this.acceptNew) {
>+        _("Creating WBO " + JSON.stringify(record.id) + " on the fly.");
>+        wbo = new ServerWBO(record.id);
>+        this.wbos[record.id] = wbo;
>+      }

Nice! Please update the comment above to mention the new acceptNew option.


>+function test_restorePromptsReupload() {
>+  _("Ensure that restoring from a backup will reupload all records.");
>+  Svc.Prefs.set("username", "foo");
>+  Service.serverURL = "http://localhost:8080/";
>+  Service.clusterURL = "http://localhost:8080/";
>+
>+  let collection = new ServerCollection({}, true);
>+  let engine = new BookmarksEngine();
>+  let store = engine._store;
>+  let global = new ServerWBO('global',
>+                             {engines: {bookmarks: {version: engine.version,
>+                                                    syncID: engine.syncID}}});
>+  let server = httpd_setup({
>+    "/1.0/foo/storage/meta/global": global.handler(),
>+    "/1.0/foo/storage/bookmarks": collection.handler()
>+  });
>+
>+
>+  try {
>+
>+    let folder1_id = Svc.Bookmark.createFolder(
>+      Svc.Bookmark.toolbarFolder, "Folder 1", 0);
>+    let folder1_guid = store.GUIDForId(folder1_id);
>+    _("Folder 1: " + folder1_id + ", " + folder1_guid);
>+
>+    let fxuri = Utils.makeURI("http://getfirefox.com/");
>+    let tburi = Utils.makeURI("http://getthunderbird.com/");
>+
>+    _("Create a single record.");
>+    let bmk1_id = Svc.Bookmark.insertBookmark(
>+      folder1_id, fxuri, Svc.Bookmark.DEFAULT_INDEX, "Get Firefox!");
>+    let bmk1_guid = store.GUIDForId(bmk1_id);
>+    _("Get Firefox!: " + bmk1_id + ", " + bmk1_guid);
>+
>+
>+    let dirSvc = Cc["@mozilla.org/file/directory_service;1"]
>+      .getService(Ci.nsIProperties);
>+
>+    let backupFile = dirSvc.get("TmpD", Ci.nsILocalFile);
>+
>+    _("Make a backup.");
>+    try {
>+      backupFile.append("t_b_e.json");
>+
>+      _("Backing up to file " + backupFile.path);
>+      backupFile.create(Ci.nsILocalFile.NORMAL_FILE_TYPE, 0600);
>+      PlacesUtils.backupBookmarksToFile(backupFile);

Please make sure you give these tests a try run. Windows and nsILocalFile can be hairy :)

>+      //PlacesUtils.archiveBookmarksFile(null, true);

Left-over comment?

>+    } catch (ex) {
>+      _("Can't test backups.");
>+      _(Utils.exceptionStr(ex));
>+      return;
>+    }

Why would this fail? This test basically disables itself if it can't create a backup. We'd have no way of knowing whether we actually have test coverage or not.

>+  } finally {
>+    try {
>+      store.wipe();
>+    } catch (ex) {
>+      _("Wipe failed: " + Utils.exceptionStr(ex));
>+    }

Why would store.wipe() fail?


r- because the change can be much less invasive IMHO. Summarizing what I outlined above:

* Prep the wipeRemote command for the bookmarks engine as soon as we get the observer notification for backup restore
* In addition to that, use resetClient() rather than manually repeating what _syncStartup() would do for us anyway.
* No easy way out for the tests.

Comment 11

[Richard Newman [:rnewman]]

(In reply to comment #10)

> void 0? WTF? My eyes! If you want to use a nil value, use null. But in this
> case you can just ask for falsiness.

Pref being false and pref not existing are two different values. Checking for undefined avoids having to have a separate clear method.

> You want to use
> 
>   Svc.Prefs.reset(this.restoredPref);

Ah, didn't know about that.


> We don't need this flag. Just queue a wipeEngine command for the next sync:
> 
>   Weave.Service.prepCommand("wipeEngine", [this.name]);

That I didn't know about. That does make things simpler!
 

> really mean to call a single specific method isn't a better solution IMHO. If
> anything it makes our code harder to read. 

That's why I just did the import first... it was you guys who disliked it :D

> Erm, you're overwriting SyncEngine._resetSync here.

Oops.

> Nice! Please update the comment above to mention the new acceptNew option.

Will do.
 
> Please make sure you give these tests a try run. Windows and nsILocalFile can
> be hairy :)

Was planning to :D
 
> >+      //PlacesUtils.archiveBookmarksFile(null, true);
> 
> Left-over comment?

Indeed.

> >+    } catch (ex) {
> >+      _("Can't test backups.");
> >+      _(Utils.exceptionStr(ex));
> >+      return;
> >+    }
> 
> Why would this fail? This test basically disables itself if it can't create a
> backup. We'd have no way of knowing whether we actually have test coverage or
> not.

I read (in some Places docs?) that some 3.x builds were missing necessary functionality for this part, though I can't find the reference. I'd rather the test not run on those builds than fail unexpectedly.

> >+    } catch (ex) {
> >+      _("Wipe failed: " + Utils.exceptionStr(ex));
> >+    }
> 
> Why would store.wipe() fail?

If Places IDs are cached. Places would throw an exception (in getChildren, IIRC) if you'd restored from a backup then tried to wipe, because we'd be still trying to use the old pre-restore IDs.

In other words, if you run this test before the caching fix, this line would be hit.

 
> r- because the change can be much less invasive IMHO. Summarizing what I
> outlined above:
> 
> * Prep the wipeRemote command for the bookmarks engine as soon as we get the
> observer notification for backup restore
> * In addition to that, use resetClient() rather than manually repeating what
> _syncStartup() would do for us anyway.
> * No easy way out for the tests.

Sounds fair!

Comment 12

[Philipp von Weitershausen [:philikon]]

(In reply to comment #11)
> > really mean to call a single specific method isn't a better solution IMHO. If
> > anything it makes our code harder to read. 
> 
> That's why I just did the import first... it was you guys who disliked it :D

Not me! It's a hack, sure. Sometimes you need those... :)

> > >+    } catch (ex) {
> > >+      _("Can't test backups.");
> > >+      _(Utils.exceptionStr(ex));
> > >+      return;
> > >+    }
> > 
> > Why would this fail? This test basically disables itself if it can't create a
> > backup. We'd have no way of knowing whether we actually have test coverage or
> > not.
> 
> I read (in some Places docs?) that some 3.x builds were missing necessary
> functionality for this part, though I can't find the reference. I'd rather the
> test not run on those builds than fail unexpectedly.

There's a comment in bookmarks.js. Apparently some 3.7 nightly builds didn't have PlacesUtils.archiveBookmarksFile(). That was, like, a year ago :). I'm willing to take the risk of not having our tests pass on those builds ;)

> > >+    } catch (ex) {
> > >+      _("Wipe failed: " + Utils.exceptionStr(ex));
> > >+    }
> > 
> > Why would store.wipe() fail?
> 
> If Places IDs are cached. Places would throw an exception (in getChildren,
> IIRC) if you'd restored from a backup then tried to wipe, because we'd be still
> trying to use the old pre-restore IDs.
> 
> In other words, if you run this test before the caching fix, this line would be
> hit.

Right, but since bug 627490 is a dependency of this and has landed already, we can make it fail hard there now, right? I think we should, it's another good exercise of kSpecialIDs.

Comment 13

[Philipp von Weitershausen [:philikon]]

(In reply to comment #10)
> >+    
> >+      this._log.debug("Restore succeeded: setting flag to wipe server on next sync.");
> >+      this.restoredFlag = true;
> 
> We don't need this flag. Just queue a wipeEngine command for the next sync:
> 
>   Weave.Service.prepCommand("wipeEngine", [this.name]);

Forgot to mention, we still need to do

  Weave.Service.wipeServer([this.name]);

of course.

Comment 14

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v3

Here we go.

Note that I added a little logic in the BookmarksEngine observer setup -- I noticed duplicate log lines, caused by attaching observers every time an engine was created. The bookmark tests create at least three separate engines.

That makes a difference now that we manipulate singleton Service state, so I added a safety check to avoid hitting it every time. This shouldn't make a difference in practice (I don't think we create multiple engine objects), and it actually doesn't make the test fail, but I didn't like leaving it.

I also added the date to the temporary file, because under 3.6 the temp directory is fixed, and collisions cause the test to fail.

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 506590 [details] [diff] [review]
Proposed patch. v3

>   _handleImport: function _handleImport() {
>+    // Hack to avoid multiple observers when real state matters. Otherwise we
>+    // can end up calling wipeServer/prepCommand once for every BookmarksEngine
>+    // created...
>+    // Simply check for a test observer, and if it's there, only manipulate
>+    // local state.
>+    let result = {};
>+    Svc.Obs.notify("bookmarks-observers-attached", result);
>+    let alreadyAttached = result.called;
>+    
>+    if (!alreadyAttached)
>+      Svc.Obs.add("bookmarks-observers-attached",
>+                  function(a) a.called = true, this);
>+
>     Svc.Obs.add("bookmarks-restore-begin", function() {
>       this._log.debug("Ignoring changes from importing bookmarks");
>       this._tracker.ignoreAll = true;
>     }, this);
> 
>     Svc.Obs.add("bookmarks-restore-success", function() {
>       this._log.debug("Tracking all items on successful import");
>       this._tracker.ignoreAll = false;
> 
>-      // Mark all the items as changed so they get uploaded
>-      for (let id in this._store.getAllIDs())
>-        this._tracker.addChangedID(id);
>+      this.resetClient();
>+      if (!alreadyAttached) {
>+        this._log.debug("Restore succeeded: wiping server and other clients.");
>+        Weave.Service.prepCommand("wipeEngine", [this.name]);
>+        Weave.Service.wipeServer([this.name]);
>+      }
>     }, this);

Here's an alternate solution to this alreadyAttached business:

How about we move these bookmarks-restore-* observers to the tracker? They only depend on this._tracker and Weave.Service after all. We can register them on weave:engine:start-tracking and unregister them on weave:engine:stop-tracking. The tests could easily issue these notifications (some of them already do) to ensure a clean slate. Having tests rely on lingering observers seems kinda icky...

This way we also don't bother doing anything when the user doesn't have Sync set up or stops using Sync. In fact, come to think of it, that's another excellent reason for doing it this way. :)

r=me with those changes. Sorry I didn't come up with this idea earlier.

Comment 16

[Richard Newman [:rnewman]]

Attachment: Final patch, v1

This is the version I just pushed to try, along with its dependencies:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=c04ea57f54f9

Let's see how it is in the morning!

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to comment #16)
> This is the version I just pushed to try, along with its dependencies:
> 
> http://tbpl.mozilla.org/?tree=MozillaTry&rev=c04ea57f54f9

Looks green for the important bits. Please land in 1.6.x and default.

Comment 18

[Philipp von Weitershausen [:philikon]]

This should block btw.

Comment 19

[Richard Newman [:rnewman]]

I fixed an old stupidity in ServerCollection: it was returning deleted WBOs, which made tests fail on 1.6.x.

Test pass on default and 1.6.x, as does CrossWeave.

Pushed:

  default: http://hg.mozilla.org/services/fx-sync/rev/1da5780882e4
  1.6.x:   http://hg.mozilla.org/services/fx-sync/rev/51718181a015

Comment 20

[Tracy Walker [:tracy]]

the log shows:
2011-02-03 14:34:22	Service.Main         DEBUG	Sending clients: wipeEngine,bookmarks,Delete all client data for engine


but it didn't actually do an upload 'til I did a Sync Now. based on comment #5, I believe this is all expected.

Comment 21

[Richard Newman [:rnewman]]

(In reply to comment #20)

> but it didn't actually do an upload 'til I did a Sync Now. based on comment #5,
> I believe this is all expected.

Correct. An earlier patch also delayed the server wipe, but now we do that and then upload on the next sync.