1289932 - Desktop implementation of faster "send tabs to device" using Push

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Edouard Oger [:eoger]]

After sending a tab to another device using Sync, we should send a push notification to the target device using https://github.com/mozilla/fxa-auth-server/pull/1358 and handle this message accordingly on the receiving end by triggering a clients collection sync.

Comment 1

[Edouard Oger [:eoger]]

Attachment: Bug 1289932 - Send/Handle push messages for send tab to device.

Review commit: https://reviewboard.mozilla.org/r/68678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68678/

Comment 2

[Edouard Oger [:eoger]]

Comment on attachment 8777083 [details]
Bug 1289932 - Send/Handle push messages for send tab to device.

This is very naive, care for some feedback on this Mark? (it does reproduce bug 1289287 every time though!)

Comment 3

[Mark Hammond [:markh] [:mhammond]]

https://reviewboard.mozilla.org/r/68678/#review66224

This is looking good, although I do think we can decouple things a little better

::: services/fxaccounts/FxAccounts.jsm:407
(Diff revision 1)
>    },
>  
>    /**
> +   * Notifies other devices that a tab was sent.
> +   */
> +  notifyTabSent: function(deviceIds) {

I think this is binding fxa and weave too tightly. I think "notifyDevices" is the correct abstraction here rather than the more specific "tab sent". FxA has no concept of a "tab".

IOW, I think it makes sense to have the formulation of the payload itself, and the TTL, in Sync code, with the idea being that if Sync wants to later send different notifications, FxA shouldn't need to change.

::: services/fxaccounts/FxAccountsPush.js:161
(Diff revision 1)
>      let payload = message.data.json();
>      switch (payload.command) {
>        case ON_DEVICE_DISCONNECTED_NOTIFICATION:
>          this.fxAccounts.handleDeviceDisconnection(payload.data.id);
>          break;
> +      case ON_COLLECTION_CHANGED_NOTIFICATION:

similarly here, can we turn this into the inverse of the above - ie, a more generic "ON_DEVICE_NOTIFICATION" and this code just echos the message into an observer notification. Somewhere in Sync itself we listen for the notification, check it's got message="sync:collection_changed" message and triggers the sync?

::: services/sync/modules/engines.js:1488
(Diff revision 1)
>          for (let key in resp.obj.success) {
>            let id = resp.obj.success[key];
>            delete this._modified[id];
>          }
> +
> +        if (this.name == "clients") {

It seems cleaner to me that we create a new method on the engine called, say, onRecordsWritten(succeeded, failed), where both params are, say, resp.obj.success and resp.obj.failed. The default implementation would be a noop but the clients engine would use it as you are doing here.

Comment 4

[Edouard Oger [:eoger]]

Comment on attachment 8777083 [details]
Bug 1289932 - Send/Handle push messages for send tab to device.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68678/diff/1-2/

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8777083 [details]
Bug 1289932 - Send/Handle push messages for send tab to device.

https://reviewboard.mozilla.org/r/68678/#review66544

::: services/sync/modules/engines/clients.js:248
(Diff revision 2)
>    _uploadOutgoing() {
>      this._clearedCommands = null;
>      SyncEngine.prototype._uploadOutgoing.call(this);
>    },
>  
> +  _onRecordsWritten(succeeded, failed) {

it looks like this will send a push notificaiton when we write our own record, which I don't think we want to do.

I think a test would also be valuable - maybe add a new private method to this object called, say, _notifyDevices that takes the list of IDs, and you can mock it in your tests. Then a test in test_clients_engine or similar that arranges for both our device and another record to be written and verify this new internal method is called with only the other ID.

::: services/sync/modules/service.js:491
(Diff revision 2)
>  
>    observe: function observe(subject, topic, data) {
>      switch (topic) {
> +      case "sync:collection_changed":
> +        if (data.includes("clients")) {
> +          this.sync([]); // [] = clients collection only

If we are currently Syncing but have completed the clients collection, I don't think this will work correctly (ie, we will still end up waiting for the next Sync to see the tab). However, that's enough of an edge-case and so painful to fix I think we should do it in a followup - can you please open a bug blocking this to fix that case?

Comment 6

[Edouard Oger [:eoger]]

Comment on attachment 8777083 [details]
Bug 1289932 - Send/Handle push messages for send tab to device.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68678/diff/2-3/

Comment 7

[Edouard Oger [:eoger]]

Thank you Mark, I added tests for this.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8777083 [details]
Bug 1289932 - Send/Handle push messages for send tab to device.

https://reviewboard.mozilla.org/r/68678/#review66986

::: services/sync/modules/service.js:489
(Diff revision 3)
>  
>    // nsIObserver
>  
>    observe: function observe(subject, topic, data) {
>      switch (topic) {
> +      case "sync:collection_changed":

Sorry, I should have mentioned this before :( In theory this should be in polices.js in the SyncScheduler, but that will require some work to make it know about syncing specific engines.

So please add a comment here along the lines of "ideally this observer would be in the scheduler, but it's not able to only sync specific engines. We should move this there once it is"

::: services/sync/tests/unit/test_clients_engine.js:1097
(Diff revision 3)
> +  function clientWBO(id) {
> +    return user.collection("clients").wbo(id);
> +  }
> +
> +  _("Create remote client record 1");
> +  server.insertWBO("foo", "clients", new ServerWBO(remoteId, encryptPayload({

it's not clear to me that this test is causing the local record to be written.

Could you please change the test so that this is clear? It is probably enough to ensure that collection.count() == 2 before the sync (which is the 2 remote  records you added for the test) and 3 after the sync (ie, plus 1 for the local record being written as part of the sync).

::: services/sync/tests/unit/test_clients_engine.js:1136
(Diff revision 3)
> +    Svc.Prefs.resetBranch("");
> +    Service.recordManager.clearCache();
> +
> +    try {
> +      let collection = server.getCollection("foo", "clients");
> +      collection.remove(remoteId);

I doubt this line is necessary - I think you should either remove it, or clear the entire collection (there should definately be remoteId2, and as above, there should also be the local ID if the test is testing what we expect)

Comment 9

[Edouard Oger [:eoger]]

Comment on attachment 8777083 [details]
Bug 1289932 - Send/Handle push messages for send tab to device.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68678/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5acd4f7ea715
Send/Handle push messages for send tab to device. r=markh

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5acd4f7ea715