1363412 - Do a quick sync of the clients collection on device connected

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Edouard Oger [:eoger]]

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.

Comment 1

[Edouard Oger [:eoger]]

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.

Comment 2

[Edouard Oger [:eoger]]

Attachment: Bug 1363412 - Notify other clients when uploading the local clients record for the first time.

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

Comment 3

[Edouard Oger [:eoger]]

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.

Comment 4

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

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.

Comment 5

[Edouard Oger [:eoger]]

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

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

(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 7

[Edouard Oger [:eoger]]

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

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

Comment 8

[Edouard Oger [:eoger]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

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

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