686704 - Send Tab to Device - Support for session state

Status: NEW

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

The previous implementation of send tab to device (bug 676375) is using client commands to send URIs. After discussion, we want to support sending the tab context blob as part of the API.

This blob is quite sizable and may exceed the size limit on the clients engine. Not to mention it would be subverting the clients engine quite a bit. Discussion is leaning towards a new engine to support this. This bug tracks this new implementation, whatever it may be.

We also need to decide if we want to keep the old implementation around.

Comment 1

[Philipp von Weitershausen [:philikon]]

(In reply to Gregory Szorc [:gps] from comment #0)
> The previous implementation of send tab to device (bug 676375) is using
> client commands to send URIs. After discussion, we want to support sending
> the tab context blob as part of the API.

Yep, if by tab context you mean stuff like the session history and form data. We should probably come up with a data format for that because Firefox and Fennec use different formats for that already in session restore.

> This blob is quite sizable and may exceed the size limit on the clients
> engine.

Maybe, with lots of form data, and/or lots of tabs being sent to a single device. A WBO record can be up to 256k, so we should see how much room we have.

But yes, perhaps a separate engine would be better.

Comment 2

[Richard Newman [:rnewman]]

> But yes, perhaps a separate engine would be better.

Or, for that matter, a combination of engine for storage and Clients for command pushing...

Comment 3

[Mike Connor [:mconnor]]

Recapping some IRC discussion:

Engine == commands, just grab and parse new commands, discard stuff that doesn't match that client ID.
Allows commands targeting multiple clients, and means clientID doesn't have to get exposed to the server.
Also means we can support infinite commands per client.

Comment 4

[Gregory Szorc [:gps]]

Attachment: "processRecord" command support, v0

Here is an untested and likely bug-ridden implementation of a new "processRecord" command API which stores record data in arbitrary collections. I included a very stub implementation of the APIs for sending tabs with state to give an idea how it works.

There are some obvious issues with the patch. The 'options' argument to sendProcessRecordCommand() is one thing that makes me cringe.

I'm submitting for feedback on the concept of the new command and how the records are stored, transferred, etc. I know the patch is full of bugs and likely has syntax errors. So, please, no nits unless they are relatively high level.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 562617 [details] [diff] [review]
"processRecord" command support, v0

Review of attachment 562617 [details] [diff] [review]:
-----------------------------------------------------------------

Note that you can update the Bug XXXXXX and description now:

  Bug 686704: Send Tab to Device - Support for session state

---
I gave detailed comments as I went through, just because Splinter makes it easy. Please don't interpret them as raining on your parade; just saving time later.
---

More broadly:

* You should have some tests to go along with this. There are a couple of syntax errors that implies that this code hasn't even been run, which is generally not a good sign. It's easier to start with 100% coverage and get worse over time than to go the other way!

* The functionality you've added can be summarized as "put a big fat command in a collection; add a small indirection 'load' command to the clients engine". This seems to carry a gentle form of RPC semantics: you're instructing the client to pull down a command of arbitrary size and run it. That's quite possibly not a good idea: maybe I'm on a low-bandwidth connection? Perhaps I want to introspect the command before I decide if I want to spend the time to download and run it?

I would suggest flipping this around a little. Have the big fat payload go to the collection, and put a real command, just as you did for your first draft of send-tab-to-device, into the clients engine. That command can refer to bigfatpayloads/someguid. You can still generalize the command with some kind of dispatch semantics if you want (e.g., specify an engine and a command name, so we don't end up with a switch statement in the clients engine), but don't force a download before the command can be examined.

Doing it this way allows, say, Fennec to prompt the user:

  Open <some slashdot URI> sent from Greg's Desktop?

before downloading the whole tab state payload. The command could even refer to multiple blobs, if you someday go over the 256KB limit:

  {engine: "tabs",
   command: "open",
   tabstate: "tabstates/abcdefghijkl",
   localstorage: "localstorage/foobarbaznoo"}

This approach also opens up interesting possibilities, such as sending different page-processing commands to different device types (Read It Later vs open now, for example) whilst preserving the same state object.

Thoughts?

::: services/sync/modules/engines/clients.js
@@ +52,5 @@
>  
>  const CLIENTS_TTL = 1814400; // 21 days
>  const CLIENTS_TTL_REFRESH = 604800; // 7 days
>  
> +// Filename used to store outbound record commands until sync

Closing period.

@@ +55,5 @@
>  
> +// Filename used to store outbound record commands until sync
> +const OUTBOUND_RECORD_COMMANDS_FILENAME = "outbound_record_commands";
> +
> +// Collection holding records storing tab state for tab loading.

Argh, English!

// The collection that holds records which store tab state for tab loading.

would be a minor improvement.

@@ +165,5 @@
> +    SyncEngine.prototype._uploadOutgoing.call(this);
> +
> +    let cb = Async.makeSyncCallback();
> +    Utils.jsonLoad(OUTBOUND_RECORD_COMMANDS_FILENAME, this, cb);
> +    let json = Async.waitForSyncCallback(cb);

_uploadOutgoing has to block, so yes, you have to spin the event loop.

I'd much rather you only do it once, though! Chain jsonLoad and jsonSave, and spin on the callback inside jsonSave (or invoke it on the load failure case, of course).

@@ +167,5 @@
> +    let cb = Async.makeSyncCallback();
> +    Utils.jsonLoad(OUTBOUND_RECORD_COMMANDS_FILENAME, this, cb);
> +    let json = Async.waitForSyncCallback(cb);
> +
> +    if (!json || !json.keys().length) {

Do you mean

  Object.keys(json).length

?

@@ +174,5 @@
> +
> +    for (let [guid, v] in Iterator(json)) {
> +      let record = new WBORecord(v[0], guid);
> +      record.ttl = v[1];
> +      record.payload = JSON.stringify(v[2]);

Let's use destructuring here.

  let [collection, ttl, payload] = v;

Also, IIRC the payload of a WBORecord is an object, not a JSON string…

@@ +176,5 @@
> +      let record = new WBORecord(v[0], guid);
> +      record.ttl = v[1];
> +      record.payload = JSON.stringify(v[2]);
> +
> +      record.upload(this.storageURL + v[0] + "/" + guid);

This could be more efficient when you're dealing with more than one object at a time.

You make one HTTP request per object. You concatenate the storage URL once per object.

If you take a look at SyncEngine._uploadOutgoing, you'll see a way to use Collection to batch your uploads.

Yes, it's likely there's only be a small number of objects… right up until the day we allow you to send *all* of the tabs in this folder, or this window, to another device.

@@ +371,5 @@
>      }
>    },
>  
>    /**
> +   * Send a remote record command to a set of clients

Closing period.

@@ +376,5 @@
> +   *
> +   * Commands sent using this function result in:
> +   *
> +   *   1) A new record consisting of the data specified being added to the
> +   *      collection specified

I will do a grammar overhaul prior to r+ :D

@@ +392,5 @@
> +   * @param clients
> +   *        Client ID or IDs to send command to. Can be specified as:
> +   *          string    single Client ID to send command to
> +   *          array     list of Client IDs to send command to
> +   *          undefined send command to all remote clients

Normally we avoid this kind of polymorphism. At the very least let's require an array here, or some constant Clients.ALL object.

@@ +400,5 @@
> +   *          deleteOnRead - If true, the client is instructed to delete the
> +   *                         record after reading. By default, this is set to
> +   *                         true if the command is being sent to one client
> +   *                         and false if sent to multiple clients.
> +   */

It strikes me that some control over TTL would be useful. Other than deletion, TTL is the only mechanism offered by this approach for preventing garbage overrunning the user's quota.

@@ +416,5 @@
> +      destinationClients.push(clients);
> +    }
> +
> +    // We assemble the full record and queue it for upload.
> +    if (options === undefined) {

if (!options) ...

or, better

  options = options || { ... }

Otherwise I can pass null for options, and you'll get

  Error: invalid 'in' operand null

because !(null === undefined).

@@ +424,5 @@
> +    }
> +
> +    if ('deleteOnRead' in options) {
> +      record.deleteOnRead = options.deleteOnRead;
> +    else if (destinationClients.length) {

Missing }.

@@ +475,5 @@
> +    let record = remoteRecord.fetch(uri);
> +
> +    if (collection == LOAD_TAB_COLLECTION) {
> +      this._handleLoadTabCommand(record);
> +    }

This strikes me as inverted control.

How about instead of adding what amounts to a glorified switch statement, you use OO dispatch?

Have the command specify an engine name (e.g., "tabs").

When you receive a message, look up the engine (Engines.get(name)). If you get an engine, see if it has a .handleMessage method in its prototype chain. Hand over the message.

You can provide a default handler on SyncEngine if you wish.

The reason I make this remark is that you're pitching this as a generic capability: put a record somewhere else, and then have an engine handle it. If that's the case, have the engine handle it!

Comment 6

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #5)
> * You should have some tests to go along with this.

Per my note on desired feedback, I wanted high-level feedback before implementing tests to save refactor time if feedback was negative.

> There are a couple of syntax errors that implies that this code hasn't even
> been run.

Yup. I called this out when I submitted the diff.
 
> Perhaps I want to introspect the command before I decide if I
> want to spend the time to download and run it?

Yeah. I purposefully designed the data model to make this possible (but putting the bulk data in arbitrary collections so the client could key off the collection name before fetching the record). I just implemented it improperly. It has been fixed on my local machine.

> I would suggest flipping this around a little. Have the big fat payload go
> to the collection, and put a real command, just as you did for your first
> draft of send-tab-to-device, into the clients engine.

Umm, that's what I did.

> Doing it this way allows, say, Fennec to prompt the user:
> 
>   Open <some slashdot URI> sent from Greg's Desktop?
> 
> before downloading the whole tab state payload. The command could even refer
> to multiple blobs, if you someday go over the 256KB limit:
> 
>   {engine: "tabs",
>    command: "open",
>    tabstate: "tabstates/abcdefghijkl",
>    localstorage: "localstorage/foobarbaznoo"}
> 
> This approach also opens up interesting possibilities, such as sending
> different page-processing commands to different device types (Read It Later
> vs open now, for example) whilst preserving the same state object.
> 
> Thoughts?

OK, now we're getting to some interesting feedback! There are a few areas to explore here.

At what level of command processing do we wish to bake in the processing semantics? We could go with individual commands for every little thing (like tab sending), but a lot of the small metadata in the individual command as arguments, and reference the larger blob in some remote collection. Going up a level, we could go with what is implemented here: a generic "processRecord" command that is effectively a pointer. The base command has minimal metadata (like flags for semantics for delete on read, ignore on unknown, etc). We could even go with a hybrid of this approach, where we use per-action commands and move the smaller metadata to the command itself.

Your remark about automatic dispatch of commands is also interesting. I like this approach. It keeps the Clients Engine domain specific and allows other engines to staple on functionality without modifying core code. Another advantage of this method is that fetching of referenced record (if that is needed at all), is totally up to the other engine. We may provide a helper API, but the Clients Engine won't need to fetch the record before control is handed off to someone else. And, we won't enforce semantics of how a record is referenced. I'm really liking this.

Now, the above two paragraphs are somewhat in conflict and the implementation seems to be mutually exclusive. We have 1) create a special command with strong semantics that handles data stored in remote records 2) add an 'engine' key to command records that, when present, forwards the command to another engine for processing, having it deal with remote records if necessary.

To further complicate things, commands all exist within the clients record. This has known issues (race conditions, size limitations). If we open up a generic way for engines to send their own commands without modifying the Clients Engine, we give other engines (some possibly provided by 3rd party extensions) the opportunity to shoot themselves (and the core) in the foot. Do we want to open this door?

I agree that a generic and extensible command processing feature is nice (and I want it). But, do we want to cross that bridge now. Should it be a blocker for this bug?

Comment 7

[Philipp von Weitershausen [:philikon]]

These are good ideas, but can we please build abstractions when we need them? As long as the record format stays stable, we can make the client code more pluggable later when we find more use cases for this. But (a) this stuff needs to ship (stat!) and (b) I don't feel at all like creating abstractions based on 1 case. Just my 2 cents. :)

Comment 8

[Richard Newman [:rnewman]]

Summary of where I think this should go:

* sendTabToDevice command, which includes a pointer to a remote record.
* The method which handles sendTabToDevice (just like the existing sendURIToDevice command) is responsible for fetching and otherwise owning the remote record.
* By all means provide an argument channel for things like "delete this when you're done".

This gives us a decent separation of control, and a starting point for possible future extension.

One thing that hasn't been discussed is wipeServer. For the first time there won't be a 1:1 mapping between engine names and collections; you're going to have to touch Service._freshStart to ensure that the appropriate dependent collections get cleared.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #6)
> > * You should have some tests to go along with this.
> 
> Per my note on desired feedback, I wanted high-level feedback before
> implementing tests to save refactor time if feedback was negative.

At the risk of flogging a dead horse: a couple of "success branch" tests are a really good way to illustrate how an API will be used, and to help you design it. Furthermore, tests help you refactor!

Tests are your friend :)

> Yup. I called this out when I submitted the diff.

You can't put code in front of me or Philipp without having us point out nits :)

Comment 10

[Gregory Szorc [:gps]]

Attachment: Implement "displayTab" command, v1

Here is the implementation of the new command, taking the direction that was decided on in IRC.

I fully expect a f- because there are many items that need feedback. These are explicitly marked with TODO.

There are a few tests that need to be implemented. These are also documented with TODO. One (the failing tab state record upload/download one) requires feedback on proper error handling before it is implemented (this question is posed as a TODO). If you can think of additional tests, please say so.

It is my hope that the feedback from this attachment will lead directly to additional coding which will produce a patch that can be r+ (if I do my end right), so please use scrutiny.

Comment 11

[Philipp von Weitershausen [:philikon]]

Comment on attachment 564446 [details] [diff] [review]
Implement "displayTab" command, v1

(I haven't looked at the impl at all, just giving some feedback on some of the comments:)

>+// TODO philikon wants a better name. what is it?
>+const SESSION_STATE_COLLECTION = "session-states";

Yeah, session state sounds like we're syncing sessionstore.js or something, but we're not. We're sending a tab link over the wire. The *tab* state is just a bonus. I don't imagine all clients will support it (e.g. Blackberry Sync, Firefox Home for iOS, etc.).

I suggest SendTab* for object names and 'sendtab' for collection.

>+  /**
>+   * Sends a tab to another client.
>+   *
>+   * This can be thought of as a more powerful version of
>+   * sendURIToClientForDisplay(). This version takes an options
>+   * argument which controls how the sending works.
>+   *
>+   * Calling this function with session state will eventually result in a
>+   * record in another collection being created. This record is only
>+   * referenced in the command. It is protected against orphanage by a
>+   * server-side expiration TTL.
>+   *
>+   * Session state (as obtained from NSISessionStore) may optionally be sent
>+   * with the command. Session state may vary between application types (e.g.
>+   * desktop vs. mobile).

So how are you proposing to deal with the differences between the tab state format? E.g. I want to send a tab from desktop to mobile (most common use case). How would that work? I think there's no way around defining what this format looks like ourselves and then adding some appropriate translators. Also, what if the if the nsISessionStore format changes between versions? All of a sudden records can't be synced anymore.

There's also an architectural purity argument to be made here: none of the other record formats are tied to a specific Firefox API. They're specifically defined to be app-agnostic. Just saying "this format is whatever nsISessionStore deems to be the format" would be a crass deviation from that principle.

(Also, nit: it's nsISessionStore with a lower case 'ns').

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 564446 [details] [diff] [review]
Implement "displayTab" command, v1

Review of attachment 564446 [details] [diff] [review]:
-----------------------------------------------------------------

Lots and lots of feedback. You're going in the right direction, so you get f+.

Tests on error -- including partial error -- are an absolute requirement for this getting r+.

::: services/sync/modules/engines/clients.js
@@ +53,5 @@
>  const CLIENTS_TTL = 1814400; // 21 days
>  const CLIENTS_TTL_REFRESH = 604800; // 7 days
>  
> +// TODO philikon wants a better name. what is it?
> +const SESSION_STATE_COLLECTION = "session-states";

const TAB_STATE_COLLECTION = "sendtab";

@@ +56,5 @@
> +// TODO philikon wants a better name. what is it?
> +const SESSION_STATE_COLLECTION = "session-states";
> +
> +// TODO verify this TTL is appropriate
> +const SESSION_STATE_TTL = 604800; // 7 days

TAB_STATE_TTL.

@@ +76,5 @@
> +  * It is used by the send tab command. Tab states are stored in their own
> +  * collection because they could be large and could exhaust space in the
> +  * clients record.
> +  */
> +function SessionStateRec(collection, id) {

SendTabRecord.

@@ +179,5 @@
>  
> +  // We override the default implementation to additionally upload tab state
> +  // records.
> +  _uploadOutgoing: function _uploadOutgoing() {
> +    SyncEngine.prototype._uploadOutgoing.call(this);

I think this is backward.

According to my reading of this, what you do is:

* Upload commands by invoking the prototype method.
* Attempt to upload tab state records.

So, what happens when you upload a bunch of tab open requests, take five minutes to upload the tab state payload for the first, and the other client processes the commands in the mean time? Oops.

This takes careful handling.

You don't want to upload a command record without the tab state having been uploaded. We have a failed-record system; use it.

However, you don't want to stall the processing of other commands while you wait for tab state to upload, perhaps unsuccessfully (for quota reasons).

A few possible remediations for this:

* Upload non-dependent commands, then process the dependent commands one by one, attempting to send the tab data for each prior to sending the command.
* Similar, but batch the tab data and cross your fingers.
* Do what you do now, but have the consumer queue the command and try again later if it gets a 404 fetching the tab state.

@@ +186,5 @@
> +      return;
> +    }
> +
> +    let collection = new Collection(
> +      this.storageURL + SESSION_STATE_COLLECTION);

If you look in engines.js, you'll see an idiom like this:

  get cryptoKeysURL() this.storageURL + "crypto/keys",

I suggest you do something similar, so you can say

  let collection = new Collection(this.tabStateURL);

throughout this file.

@@ +188,5 @@
> +
> +    let collection = new Collection(
> +      this.storageURL + SESSION_STATE_COLLECTION);
> +
> +    // TODO limit upload to N records at a time?

Yes. See engines.js:1138; MAX_UPLOAD_RECORDS.

@@ +194,5 @@
> +      let [ttl, state] = v;
> +
> +      let record = new SessionStateRec(guid, SESSION_STATE_COLLECTION);
> +      // TODO [gps] I thought .id was populated automagically by the ctor? The
> +      // fake HTTP server doesn't like it unless the next line is defined. Huh?

Define "doesn't like it"?

Did you write a trivial test that instantiates a SessionStateRec and pokes .id?

It certainly looks like the constructors chain appropriately. It's possible that there's a bug; I see CryptoWrapper also sets .id itself.

Note that your hackery should be inside the SessionStateRec ctor in any case...

@@ +205,5 @@
> +      collection.pushData(record);
> +    }
> +
> +    // TODO probably need error checking on this. What's appropriate?
> +    collection.post();

The idiom goes something like

        let resp = collection.post();
        if (!resp.success) {
          this._log.debug("Uploading records failed: " + resp);
          resp.failureCode = ENGINE_UPLOAD_FAIL;
          throw resp;
        }

but ask philikon if his SyncError stuff is ready yet.

@@ +214,1 @@
>    // Always process incoming items because they might have commands

While you're here, add a '.' to these evil fragmented comments.

@@ +453,5 @@
> +   * record in another collection being created. This record is only
> +   * referenced in the command. It is protected against orphanage by a
> +   * server-side expiration TTL.
> +   *
> +   * Session state (as obtained from NSISessionStore) may optionally be sent

s/NS/ns.

This is not NeXTSTEP :)

@@ +454,5 @@
> +   * referenced in the command. It is protected against orphanage by a
> +   * server-side expiration TTL.
> +   *
> +   * Session state (as obtained from NSISessionStore) may optionally be sent
> +   * with the command. Session state may vary between application types (e.g.

… tab state. This applies throughout.

@@ +457,5 @@
> +   * Session state (as obtained from NSISessionStore) may optionally be sent
> +   * with the command. Session state may vary between application types (e.g.
> +   * desktop vs. mobile). This function currently sends along the local client
> +   * ID as part of the command. The receiver can infer the session state format
> +   * by looking up the sender's type by its ID.

*Scooby Doo impression*

Nope. Define a record format that includes the information you need to build a tab on any device. Sync needs to be device agnostic; a dump of an object isn't adequate.

Just think: the alternative is that you send a mobile record to a desktop, and the *desktop* needs to know how to do the translation. Now you've got N*M*V translations (for each pairwise translation of client types and versions), rather than N translations to a common format.

@@ +460,5 @@
> +   * ID as part of the command. The receiver can infer the session state format
> +   * by looking up the sender's type by its ID.
> +   *
> +   * @param uri
> +   *        String URI to send to the remote client

'.'

@@ +467,5 @@
> +   * @param options
> +   *        Object containing metadata to control how sending works. Can
> +   *        contain the following keys:
> +   *          sessionState - Tab's session state (as obtained from
> +   *          NSISessionStore). If not defined, no session state is sent.

s/NS/ns

@@ +478,5 @@
> +
> +    options = options || {};
> +
> +    if (!("ttl" in options)) {
> +      options.ttl = SESSION_STATE_TTL;

Perhaps call this DEFAULT_TAB_STATE_TTL, then?

@@ +481,5 @@
> +    if (!("ttl" in options)) {
> +      options.ttl = SESSION_STATE_TTL;
> +    }
> +
> +    let args = {uri: uri, senderID: this.localID};

Split line, please. It'll grow…

@@ +490,5 @@
> +    // record, outside of the clients collection.
> +    if ("sessionState" in options) {
> +      let guid = Utils.makeGUID();
> +      args.stateID = guid;
> +      this._store._outgoingTabStateRecords[guid] = [options.ttl, options.sessionState];

Why did you elect to use an array here instead of an object? Just pass options!

@@ +508,5 @@
> +    // Commands may or may not have tab state. If they do, it is stored in a
> +    // foreign record, which we'll need to fetch.
> +    if ("stateID" in args) {
> +      let record = new SessionStateRec(SESSION_STATE_COLLECTION, args.stateID);
> +      let uri = this.storageURL + SESSION_STATE_COLLECTION + "/"

Again, this.tabStateURL.

@@ +513,5 @@
> +                + args.stateID;
> +
> +      this._log.debug("Fetching remote record from: " + uri);
> +
> +      // This is synchronous, but spins the event loop. It also throws on

s/but/and!

@@ +514,5 @@
> +
> +      this._log.debug("Fetching remote record from: " + uri);
> +
> +      // This is synchronous, but spins the event loop. It also throws on
> +      // error.

I don't believe it does.

  // Get thyself from your URI, then deserialize.
  // Set thine 'response' field.
  fetch: function fetch(uri) {
    let r = new Resource(uri).get();
    if (r.success) {
      this.deserialize(r);   // Warning! Muffles exceptions!
    }
    this.response = r;
    return this;
  },

Resource.get() will throw in some circumstances, but not a 404:

--
[21:30:57.706] new (Components.utils.import("resource://services-sync/resource.js").Resource)("http://twinql.com/40440400000").get().status;
[21:30:57.852] 404

You definitely want to check for success here... and then you have to decide what happens when you can't get the data for a command!

@@ +522,5 @@
> +      state = record.sessionState;
> +      this._log.debug("Received tab state record: " + args.stateID);
> +
> +      // TODO what do we do about the record? do we let the server auto-expire?
> +      // Do we perform an async delete?

Good question. Are they ever shared between commands? How big are they?

If a user is sending 5 tabs per day, and each one carries 1MB of encrypted+base64ed+JSONified data, he's going to hit his quota by Friday.

In that case you definitely need to consider cleanup. Something like accumulating the IDs and doing a batched async (RESTRequest) DELETE with ids= at the end of a sync would suffice, mm?

@@ +540,5 @@
>  }
>  ClientStore.prototype = {
>    __proto__: Store.prototype,
>  
> +  // Holds tab state records for "displayTab" commands that haven't been sent

"haven't yet been sent."

@@ +541,5 @@
>  ClientStore.prototype = {
>    __proto__: Store.prototype,
>  
> +  // Holds tab state records for "displayTab" commands that haven't been sent
> +  // yet. Record ID -> payload object

'.'

@@ +548,1 @@
>    create: function create(record) this.update(record),

Please use explicit  { return ...; }. It'll make Emacs happy.

@@ +548,3 @@
>    create: function create(record) this.update(record),
>  
>    update: function update(record) {

Please fix this while you're here: brackets and cuddling and such.

You get extra points if you clean up and make the whole file follow coding standards in a part 0, but I am aware of the awfulness of rebase hell if you try to do that with hg mq, so a part 2 is fine :)

@@ +581,5 @@
>    },
>  
>    wipe: function wipe() {
> +    this._remoteClients           = {};
> +    this._outgoingTabStateRecords = {};

<3 indenting.

::: services/sync/tests/unit/test_clients_engine.js
@@ +797,5 @@
> +  do_check_true(Clients.processIncomingCommands());
> +});
> +
> +// TODO add test for multiple send tab commands in one go
> +// TODO add test exercising failures in tab state record upload and download

Yes.

I am curious about the error strategy here.

Comment 13

[Gregory Szorc [:gps]]

I found another bug:

in sendTabToClient(), we create the outgoing state record before we send the command. If there is an error sending the command (unknown client ID, perhaps), we are left with an orphaned tab state record which gets uploaded on next sync.

Comment 14

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #12)
> @@ +179,5 @@
> >  
> > +  // We override the default implementation to additionally upload tab state
> > +  // records.
> > +  _uploadOutgoing: function _uploadOutgoing() {
> > +    SyncEngine.prototype._uploadOutgoing.call(this);
> So, what happens when you upload a bunch of tab open requests, take five
> minutes to upload the tab state payload for the first, and the other client
> processes the commands in the mean time? Oops.

Good observation!
 
> A few possible remediations for this:
>
> * Upload non-dependent commands, then process the dependent commands one by
> one, attempting to send the tab data for each prior to sending the command.

Unfortunately, commands are all in one array in the client record. So, correct me if I'm wrong, but if we do this approach, we have a race condition between the sending client uploading the "displayTab" commands and the receiving client(s) uploading the record signifying they processed the original, non-displayTab commands which the sending client already sent during that sync.

> * Similar, but batch the tab data and cross your fingers.

Are you suggesting batching the clients and tab data collection data? I already batch the tab data records in one POST. I don't think it is possible to modify 2 collections in one HTTP request.

> * Do what you do now, but have the consumer queue the command and try again
> later if it gets a 404 fetching the tab state.

That's a possibility without too much controversy. Easy enough to implement with timers and callbacks too.

> However, you don't want to stall the processing of other commands while you
> wait for tab state to upload, perhaps unsuccessfully (for quota reasons).

Is this really that large of an issue? When the user initiates a send tab command, that will incur a sync via score trigger. Is it not safe to assume then that the syncs transferring send tab commands will be transferring this data and none other (at least for the vast majority of the time)? One exception is the case where send tab is initiated and the Sync server is unavailable. What are the others? Do we care? We have other sync scheduling issues today and we don't care too much about them (at least the bugs aren't resolved). Is this one warranting the extra level of scrutiny?

(I'm just throwing ideas out here. I don't have a solid opinion yet.)

> @@ +194,5 @@
> > +      let [ttl, state] = v;
> > +
> > +      let record = new SessionStateRec(guid, SESSION_STATE_COLLECTION);
> > +      // TODO [gps] I thought .id was populated automagically by the ctor? The
> > +      // fake HTTP server doesn't like it unless the next line is defined. Huh?
> 
> Define "doesn't like it"?
> 
> Did you write a trivial test that instantiates a SessionStateRec and pokes
> .id?
> 
> It certainly looks like the constructors chain appropriately. It's possible
> that there's a bug; I see CryptoWrapper also sets .id itself.

The problems I encountered manifested in the test code. The test HTTP server (the new one) didn't like the POST to a collection URI. It auto-created a WBO at a collection URI or something. I'll poke around to identify the root.

> @@ +490,5 @@
> > +    // record, outside of the clients collection.
> > +    if ("sessionState" in options) {
> > +      let guid = Utils.makeGUID();
> > +      args.stateID = guid;
> > +      this._store._outgoingTabStateRecords[guid] = [options.ttl, options.sessionState];
> 
> Why did you elect to use an array here instead of an object? Just pass
> options!

Yeah, this is my low-level background manifesting. I surely can throw an object around.
  
> @@ +514,5 @@
> > +
> > +      this._log.debug("Fetching remote record from: " + uri);
> > +
> > +      // This is synchronous, but spins the event loop. It also throws on
> > +      // error.
> 
> I don't believe it does.
> 
>   // Get thyself from your URI, then deserialize.
>   // Set thine 'response' field.
>   fetch: function fetch(uri) {
>     let r = new Resource(uri).get();
>     if (r.success) {
>       this.deserialize(r);   // Warning! Muffles exceptions!
>     }
>     this.response = r;
>     return this;
>   },

Go down the rabbit hole and you will find that Resource.get() calls Resource._request() which calls AsyncResource._doRequest() while spinning the event loop.

> @@ +522,5 @@
> > +      state = record.sessionState;
> > +      this._log.debug("Received tab state record: " + args.stateID);
> > +
> > +      // TODO what do we do about the record? do we let the server auto-expire?
> > +      // Do we perform an async delete?
> 
> Good question. Are they ever shared between commands? How big are they?
> 
> If a user is sending 5 tabs per day, and each one carries 1MB of
> encrypted+base64ed+JSONified data, he's going to hit his quota by Friday.
> 
> In that case you definitely need to consider cleanup. Something like
> accumulating the IDs and doing a batched async (RESTRequest) DELETE with
> ids= at the end of a sync would suffice, mm?

Yeah, I definitely need to get some data on how big the state records will be. Unless we staple extra functionality out of reusing tab states, I too was thinking doing a batched async delete at the end of a sync would be best.

Comment 15

[Gregory Szorc [:gps]]

Attachment: Part 1: add APIs to engines to define collections

When we add the "sendtab" collection to Clients, this will necessitate some extra logic for wiping multiple collections registered to one engine. This patch provides that functionality. It is a little more complicated than I would like because the Clients engine is special and not treated as part of the regular set of engines by EngineManagerSvc. But, there's not much getting around that.

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 566095 [details] [diff] [review]
Part 1: add APIs to engines to define collections

Review of attachment 566095 [details] [diff] [review]:
-----------------------------------------------------------------

Needs to be re-interpreted on top of Bug 684798, which will make this simpler.

::: services/sync/modules/engines.js
@@ +430,5 @@
> +
> +  /**
> +   * Returns an array of all the names of collections belonging to all engines.
> +   */
> +  get allCollectionNames() {

You can eliminate this.

@@ +450,5 @@
> +    let collections = [];
> +    for each (let engine in this.getAll()) {
> +      for each (let collection in engine.wipeCollectionNames) {
> +        collections.push(collection);
> +      }

And this.

@@ +619,5 @@
>      Svc.Prefs.set(this.name + ".syncID", value);
>    },
>  
> +  /**
> +   * Returns an array of all collections this engine is responsible for.

"for which this engine is responsible."

You should document that either (a) this returns a new array each time, suitable for mutating, or (b) the array should be treated as immutable.

@@ +621,5 @@
>  
> +  /**
> +   * Returns an array of all collections this engine is responsible for.
> +   *
> +   * This set is used for determining what server collections to wipe when

s/what/which.

@@ +622,5 @@
> +  /**
> +   * Returns an array of all collections this engine is responsible for.
> +   *
> +   * This set is used for determining what server collections to wipe when
> +   * clearing all server data, for example.

s/for example//

@@ +624,5 @@
> +   *
> +   * This set is used for determining what server collections to wipe when
> +   * clearing all server data, for example.
> +   */
> +  get allCollectionNames() {

What's wrong with "collections" here?

  Engines.get("bookmarks").collections;

seems much neater than

  Engines.get("bookmarks").allCollectionNames;

@@ +636,5 @@
> +   * This is typically the same as allCollectionNames. It is only different in
> +   * specific circumstances, such as the Clients engine/collection, which is
> +   * treated specially.
> +   */
> +  get wipeCollectionNames() {

… and here, "collectionsToWipe". "wipeCollections" sounds like verb + noun, which implies that you're going to wipe the collections.

@@ +774,5 @@
>  
>      // Delete any existing data and reupload on bad version or missing meta.
>      // No crypto component here...? We could regenerate per-collection keys...
>      if (needsWipe) {
> +      this.wipeServer();

I believe this will conflict with changes in current s-c, because we already fixed this.

::: services/sync/modules/service.js
@@ +1569,5 @@
>      Records.set(this.metaURL, meta);
>  
>      // Wipe everything we know about except meta because we just uploaded it
> +    let names = Clients.allCollectionNames.concat(Engines.allCollectionNames);
> +    this.wipeServer(names);

This was just changed in another patch; we now DELETE /storage instead, and do it before meta/global. You can remove this, which is why EngineManagerSvc no longer needs to care about this stuff.

Comment 17

[Gregory Szorc [:gps]]

Attachment: Implement "displayTab" command, v2

Here is the core clients engine code to support the "displayTab" command. It requires the patch in bug 699631 to work properly. There are a few TODO items, so I'm expecting r-. Other than that, please bit nitpicky.

The overall patch will be split into 3 components:

1) Core clients engine support for sending tabs, with tab state (this patch)
2) "middleware" JS to actually assemble and restore tab state to/from browser objects on desktop and mobile
3) UX hooking into above

I'm designing the patches such that they are built on top of each other. 3 requires 2 requires 1. I would prefer to check in patches as they are available to prevent bit rot. I can't guarantee we won't revisit the previous patches when implementing the latter phases. But, I think it's best to make progress and land stuff.

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 572101 [details] [diff] [review]
Implement "displayTab" command, v2

Review of attachment 572101 [details] [diff] [review]:
-----------------------------------------------------------------

Some nits and some broader questions to address.


Nit: 

  hg qref -m "Bug 686704 - Send Tab to Device: support for displayTab command in clients engine. r=rnewman

::: services/sync/modules/engines/clients.js
@@ +53,4 @@
>  const CLIENTS_TTL = 1814400; // 21 days
>  const CLIENTS_TTL_REFRESH = 604800; // 7 days
>  
> +const TAB_STATE_COLLECTION = "sendtab";

What's wrong with "tabstate"?

@@ +55,5 @@
>  
> +const TAB_STATE_COLLECTION = "sendtab";
> +
> +// TODO verify TTL is appropriate
> +const DEFAULT_TAB_STATE_TTL = 604800; // 7 days

Not sure that "verify" is the right word here :)

"Decide" is more accurate!

I would suggest that, given that we delete records after processing the command, a fairly long value is acceptable. Whether 7 days counts as "fairly long" is debatable.

I encourage you to simply make an informed decision here, and justify your working in a brief comment (either in the code or in the bug).

@@ +75,5 @@
> +  * It is used by the send tab command. Tab states are stored in their own
> +  * collection because they could be large and could exhaust space in the
> +  * clients record.
> +  */
> +function SendTabRecord(collection, id) {

So if this is the record for individual tab states, why isn't it called TabStateRecord?

@@ +179,5 @@
> +
> +  /**
> +   * Obtain the URL of the tab state collection.
> +   *
> +   * @return string

Capitalize string for consistency.

@@ +183,5 @@
> +   * @return string
> +   *         URI to tabstate collection.
> +   */
> +  get tabStateURL() {
> +    return this.storageURL + TAB_STATE_COLLECTION;

Nit: be consistent. URL or URI.

@@ +206,5 @@
> +  // records.
> +  _uploadOutgoing: function _uploadOutgoing() {
> +    // We upload the tab state records first because there could be a race
> +    // condition between another client processing the command to pull the
> +    // state and this client uploading the records.

*gasps for breath*

How about:

  // There might be a race condition between our upload of tab state records
  // and another client processing the command to fetch them.
  // Upload the tab state records first to avoid this.

@@ +216,5 @@
> +    if (!Object.keys(this._store._outgoingSendTabRecords).length) {
> +      return;
> +    }
> +
> +    let getTabRecord = function(id) {

function getTabRecord(id) {

Add a comment that this will be invoked with `this` bound to the engine.

@@ +228,5 @@
> +      return record;
> +    };
> +
> +    // The value of lastSync is irrelevant.
> +    let result = this._uploadOutgoingRecords(

"_uploadTabStateRecords" (see earlier remark).

@@ +232,5 @@
> +    let result = this._uploadOutgoingRecords(
> +      Object.keys(this._store._outgoingSendTabRecords),
> +      this.tabStateURL,
> +      this.lastSync,
> +      getTabRecord

Ugh, so misshapen. Can't wait to rip out the old engine code.

@@ +237,5 @@
> +    );
> +
> +    // Technically we should only wipe out records which were successfully
> +    // uploaded. But with tab state, we have one shot at it. If it doesn't
> +    // work, we move forward.

What are the consequences?

Perhaps more importantly, uploading records returns an object that includes failed IDs. Why don't you use that?

@@ +473,5 @@
>      Svc.Obs.notify("weave:engine:clients:display-uri", subject);
> +  },
> +
> +  /**
> +   * Sends a tab to another client as a new tab.

Last four words are either redundant or inaccurate (e.g., if the client decides a tab is already open).

@@ +484,5 @@
> +   * record in another collection being created. This record is only
> +   * referenced in the command. It is protected against orphanage by a
> +   * server-side expiration TTL.
> +   *
> +   * Tab state may is a complicated beast. As far as this API is concerned,

s/may //

@@ +486,5 @@
> +   * server-side expiration TTL.
> +   *
> +   * Tab state may is a complicated beast. As far as this API is concerned,
> +   * tab state is treated as a black box. However, various clients expect
> +   * a specific format. ext/TabState.js contains the code for interacting

Err, I don't think this is true, mm?

@@ +507,5 @@
> +
> +    options = options || {};
> +
> +    if (!("ttl" in options)) {
> +      options.ttl = DEFAULT_TAB_STATE_TTL;

Y'know you're mutating an input object here, right? Don't do that. Unpack the input options into named `let`s.

@@ +552,5 @@
> +   *
> +   *   uri      - string URI to display
> +   *   tabState - Object defining tab state. May not be defined. The tab state
> +   *              should be treated as a black box. The format is not
> +   *              well-defined outside the Clients engine at this time.

It's OK for this set of methods to ignore the contents of tab state, but "should" implies that the caller should, too. Rephrase to clarify intent.

@@ +559,5 @@
> +   * In Firefox, the notification is handled by BrowserGlue. The handler then
> +   * calls back into this engine (restoreTab) with the subject object and a
> +   * browser instance and the tab is restored. This may seem like a very
> +   * roundabout way of doing things. However, the alternative is UI code would
> +   * be tightly integrated with Sync, which is theoretically supposed to remain

s/theoretically //

@@ +560,5 @@
> +   * calls back into this engine (restoreTab) with the subject object and a
> +   * browser instance and the tab is restored. This may seem like a very
> +   * roundabout way of doing things. However, the alternative is UI code would
> +   * be tightly integrated with Sync, which is theoretically supposed to remain
> +   * application agnostic.

Hyphenated phrase.

@@ +577,5 @@
> +      this._log.debug("Fetching remote record from: " + uri);
> +
> +      // This is synchronous and spins the event loop. It also throws on
> +      // some errors.
> +      record.fetch(uri);

If so, then now so does processIncomingCommands. Do you want this to throw if fetching throws, or to proceed on?

@@ +580,5 @@
> +      // some errors.
> +      record.fetch(uri);
> +
> +      // TODO is this the best way to test for properly fetched record?
> +      if (record.ciphertext) {

record.response.success, for the basics. But checking for ciphertext (and IV and hmac!) is fine.

@@ +581,5 @@
> +      record.fetch(uri);
> +
> +      // TODO is this the best way to test for properly fetched record?
> +      if (record.ciphertext) {
> +        record.decrypt();

decrypt() can throw. Beyond that, we generally assume that records which decrypt successfully will be well-formed.

I would be inclined to wrap the whole fetch/decrypt/process section in a try…catch, and just allow decrypt to throw on missing ciphertext as well as other failures.

@@ +588,5 @@
> +
> +        // Issue an async delete request for the resource. We don't care
> +        // what the response is.
> +        let resource = new AsyncResource(uri);
> +        resource.delete(function() {});

You can roll those two lines together.

::: services/sync/tests/unit/test_clients_sendtab.js
@@ +14,5 @@
> +  let users = {};
> +  users[username] = password;
> +  return serverForUsers(users, {
> +    meta: {global: {engines: {
> +      clients: {version: Clients.version, syncID: Clients.syncID}

meta: {
  global: {
    engines: {
      clients: {version: Clients.version,
                syncID: Clients.syncID
      }
    }
  }
}

Yeah, I know.

@@ +23,5 @@
> +
> +function configure_client_and_get_server(username) {
> +  const password = "password";
> +
> +  Svc.Prefs.set("clusterURL", "http://localhost:8080/");

Set serverURL first. (See Bug 700071 for why.)

@@ +47,5 @@
> +  else {
> +    run_next_test();
> +  }
> +
> +  // We never get here.

Yes we do. It's just that it ends our current event loop cycle without doing anything else of note.

Kill this comment :)

@@ +51,5 @@
> +  // We never get here.
> +}
> +
> +add_test(function test_send_tab_to_client() {
> +  _("Ensure sendURIToClient() sends commands properly.");

Either the name of the function or the comment is wrong…

@@ +448,5 @@
> +function run_test() {
> +  initTestLogging("Trace");
> +  Log4Moz.repository.getLogger("Sync.Engine.Clients").level = Log4Moz.Level.Trace;
> +  run_next_test();
> +}

Could you move run_test to be in front of all of the add_tests? Makes appending later much easier.

Comment 19

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #18)
>   hg qref -m "Bug 686704 - Send Tab to Device: support for displayTab
> command in clients engine. r=rnewman

Created patch from Git. I'm not bothering with the overhead of Git<->Mercurial until the patch is ready to land.

> What's wrong with "tabstate"?

See comment #12. You are contradicting yourself!

> So if this is the record for individual tab states, why isn't it called
> TabStateRecord?

See comment #12. More contradiction.
 
> @@ +179,5 @@
> > +
> > +  /**
> > +   * Obtain the URL of the tab state collection.
> > +   *
> > +   * @return string
> 
> Capitalize string for consistency.

Wuh? I didn't think we capitalized the types in the Javadoc-style docs unless the types themselves were capitalized.

> @@ +237,5 @@
> > +    );
> > +
> > +    // Technically we should only wipe out records which were successfully
> > +    // uploaded. But with tab state, we have one shot at it. If it doesn't
> > +    // work, we move forward.
> 
> What are the consequences?
> 
> Perhaps more importantly, uploading records returns an object that includes
> failed IDs. Why don't you use that?

My implementation is much simpler and cleaner than the alternative.

If we want to differentiate between successful and failed records, this means we'd have to cancel out the commands for failed records *during* sync. This feels wrong to me. Also, if we somehow fail a tab state upload and send the command, the tab state record could be orphaned on the server for its TTL. We shouldn't do this.

I think the "best effort" method as currently coded strikes a good balance and is conveniently also the simplest. If you insist on stapling on extra complexity, I can do that. But, I'd much prefer to wait until commands are coded properly in the next version of the clients engine to address it.

> @@ +577,5 @@
> > +      this._log.debug("Fetching remote record from: " + uri);
> > +
> > +      // This is synchronous and spins the event loop. It also throws on
> > +      // some errors.
> > +      record.fetch(uri);
> 
> If so, then now so does processIncomingCommands. Do you want this to throw
> if fetching throws, or to proceed on?

I don't know. IMO we should probably skip over failed record fetches since that is the "SLA" on tab state uploads (unless we decide to change that - see above).

Comment 20

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #19)

> Created patch from Git. I'm not bothering with the overhead of
> Git<->Mercurial until the patch is ready to land.

Just don't forget :D

> > What's wrong with "tabstate"?
> 
> See comment #12. You are contradicting yourself!

Heh. Well, either is better than "session-states" :)

See below.
 
> > So if this is the record for individual tab states, why isn't it called
> > TabStateRecord?
> 
> See comment #12. More contradiction.

Don't assume it's a contradiction! Things change over time, y'know, and the prior comments were on early-days draft.

That was a question. If these are records that hold stuff for send tab to device, including but not limited to tab state, then SendTabRecord is appropriate. If all they do is hold tab state, and nothing else, and never will, then TabStateRecord is appropriate.

That's why this is a question: do you ever think we'll want to jam more data for a "send tab" operation into these records? If no, then we can change the names to "tabstate" (or "tabstates", maybe) and "TabStateRecord". If yes, then "sendtab" etc. are fine.

> > Capitalize string for consistency.
> 
> Wuh? I didn't think we capitalized the types in the Javadoc-style docs
> unless the types themselves were capitalized.

Ahem:

+   * @param uri
+   *        String URI to send to the remote client.
+   * @param client
+   *        String client ID to send the command to. Must be defined.

I don't care which you do, so long as you're consistent.

> > Perhaps more importantly, uploading records returns an object that includes
> > failed IDs. Why don't you use that?
> 
> My implementation is much simpler and cleaner than the alternative.

Yes, but it might have consequences, which is why I asked :)

> If we want to differentiate between successful and failed records, this
> means we'd have to cancel out the commands for failed records *during* sync.

Or best-effort reupload later, just as we would for other kinds of records.

Consider: you choose "send tab to device" because you're on a flaky wireless network. The upload of the tab state fails. Should we skip it? Fail the sync?

What if the user then immediately bookmarks the page, giving us another opportunity to sync (instant sync)?

I think it's important that this feature is not just robust, but also humane, in the face of faults…

> This feels wrong to me. Also, if we somehow fail a tab state upload and send
> the command, the tab state record could be orphaned on the server for its
> TTL. We shouldn't do this.

I presume you're talking about the case of a retry? (If we fail the tab state upload, it won't be on the server at all…)

If we retry, we already know the current command set, so we can avoid uploading records for which the command has already been processed, no?

> I think the "best effort" method as currently coded strikes a good balance
> and is conveniently also the simplest. If you insist on stapling on extra
> complexity, I can do that. But, I'd much prefer to wait until commands are
> coded properly in the next version of the clients engine to address it.

Not insisting, just requesting a thorough consideration (and documentation, whether in code, bug, or feature page) of the constraints.

Think of it this way: at some point QA is going to come back and say "I was testing send to device, and I didn't get any of my form fields". Ideally the feature page will provide enough information about how failure modes affect behavior that they can look at a log and understand that things are working fine.

If you're using a best-effort one-try upload, you're diverging from how Sync currently works. I'd like to see us either retry, or document the hell out of the decision for QA and user expectations. If we can do the former without too much confusion or complexity

Make sense?

> > If so, then now so does processIncomingCommands. Do you want this to throw
> > if fetching throws, or to proceed on?
> 
> I don't know. IMO we should probably skip over failed record fetches since
> that is the "SLA" on tab state uploads (unless we decide to change that -
> see above).

WFM. Need to catch there, then.

Comment 21

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #20)
> That's why this is a question: do you ever think we'll want to jam more data
> for a "send tab" operation into these records? If no, then we can change the
> names to "tabstate" (or "tabstates", maybe) and "TabStateRecord". If yes,
> then "sendtab" etc. are fine.

I don't have the gift of foresight. I like keeping the door open to future changes, so I prefer "SendTabRecord" over "TabStateRecord." I think both names are acceptable and the name is not exported, so it is easy enough to change in the future if we change functionality. I'm more concerned about the name of server collection, which we can't just change.

As far as tab state record uploading, if there is an error on upload (during sync), an exception is thrown. This is unhanded by my code thus surfacing the error to the main sync handler thus triggering a sync abort. I think the behavior is sufficient.

Comment 22

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #21)

> I don't have the gift of foresight. I like keeping the door open to future
> changes, so I prefer "SendTabRecord" over "TabStateRecord." I think both
> names are acceptable and the name is not exported, so it is easy enough to
> change in the future if we change functionality. I'm more concerned about
> the name of server collection, which we can't just change.

WFM.

> As far as tab state record uploading, if there is an error on upload (during
> sync), an exception is thrown. This is unhanded by my code thus surfacing
> the error to the main sync handler thus triggering a sync abort. I think the
> behavior is sufficient.

Please make sure you write some tests for this; a handler throwing a 401 for upload (and blanking the server to mimic a node reallocation) would be nice. We really want to be confident (and have documented!) the failure cases for this.

Comment 23

[Paul [pwd]]

Did this stall?

Comment 24

[Gregory Szorc [:gps]]

(In reply to Paul [sabret00the] from comment #23)
> Did this stall?

This feature has been stalled due to work on the rewrite of Firefox for mobile devices (native Fennec). Once Fennec has stabilized, this feature will be revisited.

If you want to use a simple version of this feature, you can try out https://addons.mozilla.org/en-US/firefox/addon/send-tab-to-device/

Comment 25

[Richard Newman [:rnewman]]

CCing the session store folks.