434817 - Sync open tabs and windows

Status: RESOLVED FIXED

(Cloud Services :: General, defect, P1)

Description

[Dan Mills [:thunder]]

It should be possible to sync to weave currently open tabs and windows (not the full session, just what urls you have open).

Unlike other currently-supported data types, this one may require some UI, or perhaps be set up to only run at startup/shutdown.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Taking a look at this...

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress

Here's a work in progress.  It doesn't try to restore windows (or restore tabs into the appropriate window), nor does it have UI for picking which tabs to restore, and there's at least one known bug, but it does sync tabs to and from the server.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: new and improved not workingness

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: minimal implementation, not yet polished

Here's the minimal implementation that kinda does what we want.  It needs a bunch of polish.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: basic implementation

Here's a basic implementation of this feature.  It includes a store and sync core that stores and syncs a list of tabs identified by the URL currently loaded into them and including their history (as stored by session store) and an arbitrary number identifying the window in which they appear.

The patch also includes a statusbar button that appears when new tabs are available on the server and opens a panel that presents a list of tabs for the user to open in their browser.  Tabs are queued by the store until the user opens that panel and chooses which tabs to open.

Along window IDs are stored and synced, they aren't currently used.  All tabs are presented in a single list and open in the same window where the user pressed the statusbar button.

And full sync isn't supported (i.e. you can't say "make my current session look just like the one stored on the server), since the patch targets the common case of wanting to load some tabs you were looking at on the last computer/device you were at.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: less broken, tastes great

This patch contains two big fixes over the previous one:

1. It includes the set of virtual tabs (formerly known as queued tabs) in the output of wrap so we don't generate delete commands for them and get into a delete ping-pong across the server with another simultaneously-running client.

2. It saves the set of virtual tabs to disk and restores them on startup so the output of wrap doesn't get out of sync with the snapshot state (except for real changes to the state).

The current patch saves virtual tabs to <profile>/weave/store/virtual-tabs.json, but I'm open to suggestions for better locations (.../store/tabs/virtual.json?).

Note: this patch doesn't remove unselected virtual tabs from the list when the user opens some virtual tabs.  To do that without removing those tabs from the cache of virtual tabs, we have to mark those tabs in the cache somehow, but I haven't wrapped my mind around that yet, i.e. how do we mark them, and is it ok for that mark to propogate to the server?

I would request review, but I should resolve that issue first and do some less somnambulant testing.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 322755 [details] [diff] [review]
patch v2: less broken, tastes great

Further testing done, and I've done some preliminary hacking on hiding virtual tabs that have been passed over, but that problem is proving to be a bit thickety, with potentially complicated coding that is highly dependent on how users will want this to behave, which isn't clear at all.

So I think it's better to get review and feedback now and then iterate on that in the next patch.

Comment 8

[Dan Mills [:thunder]]

Reviewing now.

You don't want the 'hidden' flag to propagate to the server, since a virtual tab on one machine is a real tab somewhere else.

That also means that upon syncing on a 3rd machine you'll be prompted again for virtual tabs you decided not to open on another machine.  I think that's okay.

Comment 9

[Dan Mills [:thunder]]

Comment on attachment 322755 [details] [diff] [review]
patch v2: less broken, tastes great


>+  // A cache of "virtual" tabs from another device that synced to the server
>+  // that the user hasn't yet decided whether or not to open locally.
>+  // Unlike other stores, we don't immediately apply create commands, which
>+  // would be jarring to users.  Instead, we store them in this cache
>+  // and prompt the user to pick which ones she wants to open.
>+  //
>+  // We also store this cache on disk and include it in the list of tabs
>+  // on the client to reduce ping-pong updates between simultaneously-running
>+  // clients and to maintain a consistent state across restarts.
>+  virtualTabs: null,


>+    this.__proto__.__proto__.applyCommands.async(this, self.cb, commandList);
>+    yield;

Wouldn't this.__proto__.applyCommands work as well?

>+    try {
>+      if (!this._file.exists())
>+        this._file.create(Ci.nsIFile.NORMAL_FILE_TYPE, PERMS_FILE);
>+
>+      let out = this._json.encode(this.virtualTabs);
>+      let [fos] = Utils.open(this._file, ">");
>+      fos.writeString(out);
>+      fos.close();
>+    } catch (e) {
>+      this._log.warn("could not serialize virtual tabs to disk: " + e);
>+      this._log.debug("Exception: " + e);
>+    }

Maybe rethrow the exception instead of logging it and moving on?  I'm not sure if we can expect sync to work in case of failure to write to disk.

>+    // XXX Does writing the GUID (i.e. URI) to the log compromise privacy?
>+    this._log.info("_createCommand: " + command.GUID);

I'd lower it to debug, info will make it a bit chatty.  Ditto for other log messages in your patch, I think.

We log URLs in the history engine/store as well, so I don't think logging it here is any worse than the status quo, though that doesn't mean we shouldn't re-examine our logging.

It can help with debugging, so leave it in for now.

>+    /*
>+    // Create the tab immediately in the most recent browser window.
>+    // XXX nsIWindowMediator::getMostRecentWindow claims to always return
>+    // a window, but other code checks its return value.  Should we do the same?
>+    let window = this._windowMediator.getMostRecentWindow("navigator:browser");
>+
>+    // FIXME: make this work even if there isn't an open window at the time
>+    // (f.e. on Mac, where the user could have selected Weave -> Sync Now
>+    // without there being an open window).  The code will be something like
>+    // the following, except that the window probably isn't ready for a new
>+    // tab the moment it opens, so we probably need to add some async magic.
>+    //if (window.location.href != "chrome://browser/content/browser.xul")
>+    //  window = window.openDialog(Utils.prefs.getCharPref("browser.chromeURL"),
>+    //                             "_blank", "all,dialog=no");
>+
>+    let tab = window.gBrowser.addTab("about:blank");
>+    this._sessionStore.setTabState(tab, this._json.encode(command.data.state));
>+    */

Above comment should be removed, imo.

>+    // Queue the tab and notify the UI to prompt the user to open the tab.
>+    this.virtualTabs[command.GUID] = command.data;

Perhaps we should check here whether the tab is already loaded, or in the set of virtual tabs, and throw an exception?  It should never happen, but other stores will throw in this case.

>+  _editCommand: function TabStore__editCommand(command) {
>+    // XXX Does writing the GUID (i.e. URI) to the log compromise privacy?
>+    this._log.info("_editCommand (not implemented): " + command.GUID);
>+
>+    // Do nothing here.  We only pay attention to the URIs of loaded tabs, not
>+    // the rest of the info that comes with tabs (history, title, etc.), so we
>+    // only create and remove tabs, we don't edit existing ones (although we do
>+    // restore the rest of the info that comes with tabs when we create one).
>+  },

It would be OK to process edits for virtual tabs, imo.

>+  wrap: function TabStore_wrap() {
...

In the unlikely event that a tab is both loaded and in the set of virtual tabs, wrap will clobber the real one with the virtual one.  It would not be a big problem (possibly some metadata would differ, but not a big deal), but I would like to check for this condition and either attempt to repair it or bail out (throw).  Or at the very least, log the condition.

>+  wipe: function TabStore_wipe() {
>+    this._log.info("wipe");
>+    // We're not going to close tabs, since that's undoubtedly not what
>+    // the user wants, but we'll reset the queue.
>+    this.virtualTabs = {};
>+  },

It's sort of supposed to, but not doing a full wipe seems reasonable too.  It's easy enough to close them by hand.

>+  resetGUIDs: function TabStore_resetGUIDs() {
>+    this._log.info("resetGUIDs: not implemented");
>+  }

No need to log anything.  Maybe at Trace level if you really want to.  It's perfectly ok to ignore this method in your case.

>+  _itemExists: function TSC__itemExists(GUID) {

The sync core is going to need to get to the virtual tabs list somehow, because _itemExists needs to return true for items that are a virtual tab.  Not sure what the best way to do that is.

* Make the virtual tabs their own object, globally available singleton via Weave.VirtualTabs or somesuch.
* Have this code get the tabs engine from Weave.Service and get _core from there, like you do from chrome/content.
* Something else?

Good job so far!

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: addresses review comments

(In reply to comment #8)
> You don't want the 'hidden' flag to propagate to the server, since a virtual
> tab on one machine is a real tab somewhere else.

Ok, here's a version that sets a _disposed flag that doesn't propagate to the server.  I implemented it by making wrap strip properties with underscores from the items it returns, in case we want to add additional private properties later.


(In reply to comment #9)

> >+    this.__proto__.__proto__.applyCommands.async(this, self.cb, commandList);
> >+    yield;
> 
> Wouldn't this.__proto__.applyCommands work as well?

this.__proto__.applyCommands is the function doing the calling (TabStore__applyCommands), since |this| is the instance of TabStore, and this.__proto__ is TabStore.prototype.  So calling that would result in infinite recursion.  We need to access the prototype's prototype to get to the superclass.


> >+    try {
> >+      if (!this._file.exists())
> >+        this._file.create(Ci.nsIFile.NORMAL_FILE_TYPE, PERMS_FILE);
> >+
> >+      let out = this._json.encode(this.virtualTabs);
> >+      let [fos] = Utils.open(this._file, ">");
> >+      fos.writeString(out);
> >+      fos.close();
> >+    } catch (e) {
> >+      this._log.warn("could not serialize virtual tabs to disk: " + e);
> >+      this._log.debug("Exception: " + e);
> >+    }
> 
> Maybe rethrow the exception instead of logging it and moving on?  I'm not sure
> if we can expect sync to work in case of failure to write to disk.

I think the worst case here is that we resync those tabs on restart, which seems ok enough not to die here, given that they are merely virtual tabs, so I've left it as is (minus the redundancy), but I can make it rethrow if you still think that makes sense.


> >+    // XXX Does writing the GUID (i.e. URI) to the log compromise privacy?
> >+    this._log.info("_createCommand: " + command.GUID);
> 
> I'd lower it to debug, info will make it a bit chatty.  Ditto for other log
> messages in your patch, I think.

Good point, I've done that.


> We log URLs in the history engine/store as well, so I don't think logging it
> here is any worse than the status quo, though that doesn't mean we shouldn't
> re-examine our logging.
> 
> It can help with debugging, so leave it in for now.

Ok, done, and I've left in the comment to help us remember to think about it.


> >+    /*
> >+    // Create the tab immediately in the most recent browser window.
> >+    // XXX nsIWindowMediator::getMostRecentWindow claims to always return
> >+    // a window, but other code checks its return value.  Should we do the same?
> >+    let window = this._windowMediator.getMostRecentWindow("navigator:browser");
> >+
> >+    // FIXME: make this work even if there isn't an open window at the time
> >+    // (f.e. on Mac, where the user could have selected Weave -> Sync Now
> >+    // without there being an open window).  The code will be something like
> >+    // the following, except that the window probably isn't ready for a new
> >+    // tab the moment it opens, so we probably need to add some async magic.
> >+    //if (window.location.href != "chrome://browser/content/browser.xul")
> >+    //  window = window.openDialog(Utils.prefs.getCharPref("browser.chromeURL"),
> >+    //                             "_blank", "all,dialog=no");
> >+
> >+    let tab = window.gBrowser.addTab("about:blank");
> >+    this._sessionStore.setTabState(tab, this._json.encode(command.data.state));
> >+    */
> 
> Above comment should be removed, imo.

Yup, removed.  Worst case we have a record of the code in earlier versions of this patch.


> >+    // Queue the tab and notify the UI to prompt the user to open the tab.
> >+    this.virtualTabs[command.GUID] = command.data;
> 
> Perhaps we should check here whether the tab is already loaded, or in the set
> of virtual tabs, and throw an exception?  It should never happen, but other
> stores will throw in this case.

Hmm, I don't see any code in any other _createCommand implementations that does this check and throws, but I've added it to the TabStore's implementation.  Hmm, I guess _removeCommand and _editCommand should do that too, then.  I haven't added that yet.


> >+  _editCommand: function TabStore__editCommand(command) {
> >+    // XXX Does writing the GUID (i.e. URI) to the log compromise privacy?
> >+    this._log.info("_editCommand (not implemented): " + command.GUID);
> >+
> >+    // Do nothing here.  We only pay attention to the URIs of loaded tabs, not
> >+    // the rest of the info that comes with tabs (history, title, etc.), so we
> >+    // only create and remove tabs, we don't edit existing ones (although we do
> >+    // restore the rest of the info that comes with tabs when we create one).
> >+  },
> 
> It would be OK to process edits for virtual tabs, imo.

Good point.  This version does that.


> >+  wrap: function TabStore_wrap() {
> ...
> 
> In the unlikely event that a tab is both loaded and in the set of virtual tabs,
> wrap will clobber the real one with the virtual one.  It would not be a big
> problem (possibly some metadata would differ, but not a big deal), but I would
> like to check for this condition and either attempt to repair it or bail out
> (throw).  Or at the very least, log the condition.

This is not so unlikely.  The scenario is this:

1. The user syncs one machine to the server.
2. The user goes to a second machine and starts Firefox, which syncs tabs from the server.
3. The user independently opens one of the synced tabs (not that unlikely, if the user is used to opening similar tabs on both machines manually and not yet used to using Weave to make that process easier).
4. Weave calls wrap as part of the process of syncing the second machine to the server.

Note that wrap actually clobbers the virtual one with the real one, since it pushes the virtual one into the items hash and then replaces it with the real one (if any).  However, it should go further and actually remove the virtual one, since it is no longer valid.  This version of the patch does that (and logs it).

Another option would be to watch for all location changes on all tabs in all windows and update the virtual tab cache every time that happens, but that's complex and expensive.  I do now update the cache when the user opens the panel, so the panel doesn't display tabs they recently opened themselves.

Note: preferring the real tab means ping-pong edits between clients that each have their own version of the tab open.  The clients will ignore the edits, but the server will still accumulate the cruft of them.  I think we could avoid this situation by preferring the virtual tab, but I'm not sure that's the best solution, since it preserves stale state in some cases.


> >+  wipe: function TabStore_wipe() {
> >+    this._log.info("wipe");
> >+    // We're not going to close tabs, since that's undoubtedly not what
> >+    // the user wants, but we'll reset the queue.
> >+    this.virtualTabs = {};
> >+  },
> 
> It's sort of supposed to, but not doing a full wipe seems reasonable too.  It's
> easy enough to close them by hand.

Ok, I have made the comment less strident.


> >+  resetGUIDs: function TabStore_resetGUIDs() {
> >+    this._log.info("resetGUIDs: not implemented");
> >+  }
> 
> No need to log anything.  Maybe at Trace level if you really want to.  It's
> perfectly ok to ignore this method in your case.

Ok, cool, I've changed this to a comment.


> >+  _itemExists: function TSC__itemExists(GUID) {
> 
> The sync core is going to need to get to the virtual tabs list somehow, 
> because _itemExists needs to return true for items that are a virtual tab.
> Not sure what the best way to do that is.
...
> * Make the virtual tabs their own object, globally available singleton via
> Weave.VirtualTabs or somesuch.
> * Have this code get the tabs engine from Weave.Service and get _core from
> there, like you do from chrome/content.
> * Something else?

Before I saw this review comment I had already implemented a public virtualTabs getter on the store for the stuff in chrome/content.  I could use this for the sync core, but then I realized that _itemExists's job would be much easier if I just make TabStore::wrap accessible to it, so that's what I've done by making a public |store| getter on the engine and giving the sync core a reference to the engine when instantiating it.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: a few minor changes

Hrm, my last patch missed a few minor changes.  Here's the version with those changes included.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: updated to work on the tip

Previous patches were using Engines instead of Weave.Engines in sync.js, and a recent checkin breaks that, so this patch uses Weave.Engines.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v6: just the meat

The previous patches contained some typo fixes that I noticed in the process of writing them.  I've checked those in separately, so this patch contains just the implementation of tab sync.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v7: adds access via the History menu

The patch adds access to tabs from other computers via the History menu.  It also includes some updates to the language describing the tabs, and it removes the XXX comments about writing tabs to the logs, which were beginning to be redundant.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch with tracker

And here's a patch with an untested tracker.  Note: feel free to review patch v7 (or even v6, for that matter) in lieu of this one and push the work done here to a separate bug/patch.

Comment 16

[Dan Mills [:thunder]]

Comment on attachment 323486 [details] [diff] [review]
patch with tracker

>+  // XXX Should we put this into SyncCore so it's available to all subclasses?

Actually, TabSyncCore no longer uses the _json property ;)

>+   * In this case we've decided to go with the on-demand approach, and we
>+   * calculate the score as the percent difference between the snapshot set
>+   * and the current tab set, where tabs that only exist in one set are
>+   * completely different, while tabs that exist in both sets but whose data
>+   * doesn't match (f.e. because of variations in history) are considered
>+   * "half different".
>+   *
>+   * So if the sets don't match at all, we return 100;
>+   * if they completely match, we return 0;
>+   * if half the tabs match, and their data is the same, we return 50;
>+   * and if half the tabs match, but their data is all different, we return 75.

The service will sync engines with scores *higher* than some number.  So I think you should modify your return value by multiplying it by -1 and adding 100 (that is, return 100 - (what you return now)).

Also, there is one more problem, though not specific to your tracker: scores need to increase over time.  Otherwise if you make a small change, but no further changes, the small change will never be synced up.

Otherwise your patch looks great.  Feel free to commit it.

Comment 17

[Dan Mills [:thunder]]

Comment on attachment 323465 [details] [diff] [review]
patch v7: adds access via the History menu

Clearing review?, marking obsolete.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #16)
> (From update of attachment 323486 [details] [diff] [review])
> >+  // XXX Should we put this into SyncCore so it's available to all subclasses?
> 
> Actually, TabSyncCore no longer uses the _json property ;)

D'oh!  Removed in a subsequent checkin.


> >+   * In this case we've decided to go with the on-demand approach, and we
> >+   * calculate the score as the percent difference between the snapshot set
> >+   * and the current tab set, where tabs that only exist in one set are
> >+   * completely different, while tabs that exist in both sets but whose data
> >+   * doesn't match (f.e. because of variations in history) are considered
> >+   * "half different".
> >+   *
> >+   * So if the sets don't match at all, we return 100;
> >+   * if they completely match, we return 0;
> >+   * if half the tabs match, and their data is the same, we return 50;
> >+   * and if half the tabs match, but their data is all different, we return 75.
> 
> The service will sync engines with scores *higher* than some number.  So I
> think you should modify your return value by multiplying it by -1 and adding
> 100 (that is, return 100 - (what you return now)).

Hmm, perhaps I don't understand how trackers work, but based on your description, which matches the one in a comment on the trackers object <http://hg.mozilla.org/labs/weave/index.cgi/file/a1f9604ae7f3/modules/trackers.js#l53>, I think the score TabTracker returns is the correct one.

As I understand it, tracker scores are supposed to represent the urgency to sync.  The tab tracker score represents the amount of difference between the snapshot and the current set, on the theory that the more different they are, the more urgent it is to sync (i.e. that there is a direct relationship between difference and urgency).

So if the threshold was, say 50, and the two sets were 75% different, the code would currently return a score of 75 and get synced, which seems correct, while if the score was inverted by multiplying it by -1 and adding 100, then it would be 25 and not get synced, which seems incorrect.


> Also, there is one more problem, though not specific to your tracker: scores
> need to increase over time.  Otherwise if you make a small change, but no
> further changes, the small change will never be synced up.

Another way to do this is for the code that periodically checks the tracker scores to decrement their thresholds over time, so even if a tracker's score doesn't increase over time, it eventually crosses its threshold anyway (as long as there are any changes).


> Otherwise your patch looks great.  Feel free to commit it.

Thanks, committed!

Comment 19

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #18)
> Another way to do this is for the code that periodically checks the tracker
> scores to decrement their thresholds over time, so even if a tracker's score
> doesn't increase over time, it eventually crosses its threshold anyway (as
> long as there are any changes).

Note: a benefit of this approach is that it segregates calculations of difference from calculations of time, which allows us to tweak each independently and may make the each one's algorithms simpler than if we were to conflate the two considerations.

Comment 20

[Dan Mills [:thunder]]

Wow, yesterday I was convinced your tracker was returning the percentage of sameness, instead of the percentage of differences.  Clearly, I should have my head examined.  Your tracker is correct.

As for the need to eventually sync even small changes, I also like the solution of decreasing the required score gradually.  That makes it easier to implement each tracker as there's one less thing to worry about.  Individual trackers could still tweak their score over time if they so choose, of course.

Comment 21

[Dan Mills [:thunder]]

Added a comment to bug 434816 about decreasing the required score.

Comment 22

[Ruben Petersen]

In how far could/should this support Session Manager, https://addons.mozilla.org/en-US/firefox/addon/2324 ?

(I'm going to mention this on https://www.mozdev.org/bugs/show_bug.cgi?id=18115 as well)

Comment 23

[Ayan Shah[:ashah]]

Investigating this bug for being potential crossweave automation test case
candidate.