1372655 - Notify other clients when uploading the local clients record for the first time

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Android Sync, enhancement, P1)

Description

[Edouard Oger [:eoger]]

This is basically the Android equivalent of bug 1363412.

This would trigger a clients collection sync on other devices immediately after ours connects, allowing the other clients to send a tab right away.

Comment 2

[Edouard Oger [:eoger]]

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

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

Comment 3

[:Grisha Kruglov]

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

https://reviewboard.mozilla.org/r/164612/#review170318

Thanks for the patch! I think it generally does what you want (sans the double-notify bit); re-flag me once you clean it up.

::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:936
(Diff revision 1)
> +      body.put("payload", payload);
> +    }
> +    if (TTL != null) {
> +      body.put("TTL", TTL);
> +    }
> +    notifyDevices(sessionToken, body, delegate);

All right, this is officially starting to move into "hacky" territory.

Perhaps make a builder for the payload body, and write some unit tests for it? Should be pretty easy.

Subtle errors are easily introduced in such code over time, and making sure boundaries between systems are solid is important - that's where we often see the bugs crop up.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:191
(Diff revision 1)
> +        }
>  
>          return;
>        }
>        checkAndUpload();
> +      if (isFirstLocalClientRecordUpload) {

I think it's possible that we'll notify some clients twice?

e.g. all was going well - our local client record was on the server, etc.

Then some roque client comes in and writes over the collection, erasing our client record.

Locally, say we're sending a tab. So we modified some client record.

We download client records, don't see our record, notify clients we just modified, but also mark this operation as "first local client record upload" and notify everyone again.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:200
(Diff revision 1)
> +
> +    private void notifyAllClients() {
> +      notifyClients(null);
>      }
>  
> -    private void notifyClients(final List<String> devicesToNotify) {
> +    // Null devicesToNotify means "Send to everyone"

Try to write your methods in a way that you don't pass around nulls on purpose. Another potential source of bugs.

Comment 4

[Edouard Oger [:eoger]]

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

https://reviewboard.mozilla.org/r/164612/#review170318

> I think it's possible that we'll notify some clients twice?
> 
> e.g. all was going well - our local client record was on the server, etc.
> 
> Then some roque client comes in and writes over the collection, erasing our client record.
> 
> Locally, say we're sending a tab. So we modified some client record.
> 
> We download client records, don't see our record, notify clients we just modified, but also mark this operation as "first local client record upload" and notify everyone again.

Even if this happens, which is pretty unlikely, this is fine since we are just sending a "sync your clients collection" message.

Comment 5

[Edouard Oger [:eoger]]

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

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

Comment 6

[:Grisha Kruglov]

(In reply to Edouard Oger [:eoger] from comment #4)
> Even if this happens, which is pretty unlikely, this is fine since we are
> just sending a "sync your clients collection" message.

And are you confident all other clients will behave correctly/sanely in that case, now and in the future? E.g., see recent iOS push message hurdles.

Comment 7

[:Grisha Kruglov]

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

https://reviewboard.mozilla.org/r/164612/#review171742

OK, but tests would still be nice ;-)

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:135
(Diff revision 2)
>      @Override
>      public void handleRequestSuccess(SyncStorageResponse response) {
> +      final Context context = session.getContext();
> +      final Account account = FirefoxAccounts.getFirefoxAccount(context);
> +      final boolean hasAccount = account != null;
> +      final AndroidFxAccount fxAccount = hasAccount ? new AndroidFxAccount(context, account) : null;

Something's gone really wrong if you don't have an account at this point.

Comment 8

[Edouard Oger [:eoger]]

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

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24a164a008b9
Notify other clients when uploading the local clients record for the first time. r=Grisha

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/24a164a008b9

Comment 11

[Alexandru Simonca [:asimonca], Release Desktop QA]

Verified Fixed in Beta 56.0b7 on Windows 10 and in Firefox Beta 56.0b7 on Android v7.0 on an LG V20 device.