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

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Sync
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: eoger, Assigned: eoger)

Tracking

unspecified
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [send-tab])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.

Updated

2 years ago
Priority: -- → P1
Assignee: nobody → eoger
(Assignee)

Comment 1

2 years ago
Created attachment 8777083 [details]
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/
(Assignee)

Comment 2

2 years ago
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!)
Attachment #8777083 - Flags: feedback?(markh)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 4

2 years ago
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/
Attachment #8777083 - Attachment description: Bug 1289932 - Initial POC of improving the latency of send tab to devices using Web Push. f?markh → Bug 1289932 - Send/Handle push messages for send tab to device.
Attachment #8777083 - Flags: feedback?(markh) → review?(markh)
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?
Attachment #8777083 - Flags: review?(markh)
(Assignee)

Comment 6

2 years ago
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/
Attachment #8777083 - Flags: review?(markh)
(Assignee)

Updated

2 years ago
Depends on: 1292693
(Assignee)

Comment 7

2 years ago
Thank you Mark, I added tests for this.
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)
Attachment #8777083 - Flags: review?(markh) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 10

2 years ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5acd4f7ea715
Send/Handle push messages for send tab to device. r=markh
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5acd4f7ea715
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.