Do a quick sync of the clients collection on device connected

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Sync
P1
normal
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: eoger, Assigned: eoger)

Tracking

unspecified
Firefox 56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

10 months ago
When another device connects, we show a notification saying "X connected to the account", but when I open "Send Tab to device" I don't see that device.
This is pretty confusing, we could do a quick sync of the clients collection when another device connects.
(Assignee)

Comment 1

10 months ago
Actually when I think about it, that notification is sent when the 2nd device logs-in (BEFORE the first sync), which would lead to race conditions if the 1st device would sync at that moment.
I think the 2nd device should send a collection_modified push notification (just like when we send tabs) after it has done its first sync.

Updated

10 months ago
Priority: -- → P2
Comment hidden (mozreview-request)
(Assignee)

Comment 3

9 months ago
mozreview-review
Comment on attachment 8876827 [details]
Bug 1363412 - Notify other clients when uploading the local clients record for the first time.

https://reviewboard.mozilla.org/r/148152/#review152502

::: services/sync/tests/unit/test_clients_engine.js:1446
(Diff revision 1)
>  
> -    let notifiedIds;
>      engine.sendCommand("wipeAll", []);
>      engine._tracker.addChangedID(engine.localID);
> -    engine.getClientFxaDeviceId = (id) => "fxa-" + id;
> -    engine._notifyCollectionChanged = (ids) => (notifiedIds = ids);
> +    const getClientFxaDeviceId = sinon.stub(engine, "getClientFxaDeviceId", (id) => "fxa-" + id);
> +    const engineMock = sinon.mock(engine);

Explanation: I made these changes because _notifyCollectionChanged was not restored properly after this test.
(Assignee)

Updated

9 months ago
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: P2 → P1

Comment 4

9 months ago
mozreview-review
Comment on attachment 8876827 [details]
Bug 1363412 - Notify other clients when uploading the local clients record for the first time.

https://reviewboard.mozilla.org/r/148152/#review152650

Looks fine if you are happy with using a TTL here and have confirmed the sending device won't get this notification

::: services/fxaccounts/FxAccountsClient.jsm:434
(Diff revision 1)
>     *         Resolves to an empty object:
>     *         {}
>     */
>    notifyDevices(sessionTokenHex, deviceIds, payload, TTL = 0) {
>      const body = {
> -      to: deviceIds,
> +      to: deviceIds || "all",

I assume you checked that "all" works on the FxA side, but does it also exclude sending it back to the sender? We don't want to get this notification and re-sync ourself.

::: services/sync/modules/engines/clients.js:400
(Diff revision 1)
>      // Reconcile the status of the local records with what we just wrote on the
>      // server
>      for (let id of succeeded) {
>        const commandChanges = this._currentlySyncingCommands[id];
>        if (id == this.localID) {
> +        if (this.isFirstSync) {

Do we really need an unlimited TTL for this notification? I'd have thought an hour would be fine (which is what we use for tabs) - we could just change the constant name to reflect it's used for all such client collection notifications.
Attachment #8876827 - Flags: review?(markh) → review+
(Assignee)

Comment 5

9 months ago
mozreview-review-reply
Comment on attachment 8876827 [details]
Bug 1363412 - Notify other clients when uploading the local clients record for the first time.

https://reviewboard.mozilla.org/r/148152/#review152650

> I assume you checked that "all" works on the FxA side, but does it also exclude sending it back to the sender? We don't want to get this notification and re-sync ourself.

Good catch, I opened https://github.com/mozilla/fxa-auth-server/issues/1942.

> Do we really need an unlimited TTL for this notification? I'd have thought an hour would be fine (which is what we use for tabs) - we could just change the constant name to reflect it's used for all such client collection notifications.

Isn't 0 is instant TTL? (no storage whatsoever).

Comment 6

9 months ago
(In reply to Edouard Oger [:eoger] from comment #5)
> > Do we really need an unlimited TTL for this notification? I'd have thought an hour would be fine (which is what we use for tabs) - we could just change the constant name to reflect it's used for all such client collection notifications.
> 
> Isn't 0 is instant TTL? (no storage whatsoever).

Good question :) I was assuming it meant infinite (although I guess that wouldn't make much sense)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

9 months ago
Turns out I implemented an "excluded" parameter on the FxA /notify endpoint :)

Comment 9

8 months ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5cf6b350c71c
Notify other clients when uploading the local clients record for the first time. r=markh

Comment 10

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