Fennec equivalent of bug 1289932. Let's speed up send tabs to device on Android. This patch should be a bit more tricky, because we need to be careful not to drain to device battery/data usage by Syncing all the time. Conversation that might help make decisions here: https://github.com/mozilla/fxa-auth-server/issues/1316

Action for eoger is to better describe the work required here at a high level.

I'm hijacking this to be a P1 for the initial breakdown from eoger.

Comment on attachment 8785064 [details] Bug 1295348 - Send/Handle push messages for send tab to device on Fennec. https://reviewboard.mozilla.org/r/74402/#review72208 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:182 (Diff revision 1) > checkAndUpload(); > } > > + @SuppressWarnings("unchecked") > + private void notifyClients(final List<String> devicesToNotify) { > + ExecutorService executor = Executors.newSingleThreadExecutor(); Not super happy about this

Got greedy and implemented the patch myself: - It's hitting the /account/devices/notify endpoint after we sync the clients collection (and we wrote other clients records). - It's asking for an immediate sync of the clients collection if we receive a collection_changed push notification.

I'm a bit surprised to see that it's the responsibility of the client to notify the other clients about changes. Is this by design? I'm not familiar with the overall sync architecture and therefore just would like to know why it's not the server sending the notification. :)

Comment on attachment 8785064 [details] Bug 1295348 - Send/Handle push messages for send tab to device on Fennec. Flagging grisha additionally, who is working on Android's sync code currently.

(In reply to Sebastian Kaspari (:sebastian) from comment #6) > I'm a bit surprised to see that it's the responsibility of the client to > notify the other clients about changes. Is this by design? I'm not familiar > with the overall sync architecture and therefore just would like to know why > it's not the server sending the notification. :) The Sync service is an opaque whiteboard. It knows the collection written to ("tabs", "clients", "bookmarks") but it knows nothing about the content written (new record, changed record, or even deleted record). However, even with that context, I think it makes sense for the *FxA* server (perhaps with client help!) to actually push the notification outbound -- and my bet is that is what's done.

(In reply to Sebastian Kaspari (:sebastian) from comment #6) > I'm a bit surprised to see that it's the responsibility of the client to > notify the other clients about changes. Is this by design? I'm not familiar > with the overall sync architecture and therefore just would like to know why > it's not the server sending the notification. :) This is by design, like Nick said it's actually the FxA server that does the push notification. Maybe at some point we will change the way we do things, but for now that will do :)

Okay, got it. So every client (desktop, iOS) will have to implement this (Call the /notify endpoint after editing the collection) in order for this to work reliably, I guess?

Comment on attachment 8785064 [details] Bug 1295348 - Send/Handle push messages for send tab to device on Fennec. https://reviewboard.mozilla.org/r/74402/#review73120 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:63 (Diff revision 1) > > public static final String COLLECTION_NAME = "clients"; > public static final String STAGE_NAME = COLLECTION_NAME; > public static final int CLIENTS_TTL_REFRESH = 604800000; // 7 days in milliseconds. > public static final int MAX_UPLOAD_FAILURE_COUNT = 5; > + public static final Integer NOTIFY_TAB_SENT_TTL_SECS = 1 * 3600; // 1 hour nit: Multiplication with 1? ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:182 (Diff revision 1) > checkAndUpload(); > } > > + @SuppressWarnings("unchecked") > + private void notifyClients(final List<String> devicesToNotify) { > + ExecutorService executor = Executors.newSingleThreadExecutor(); Are you sure the thread doesn't survive and we end up with multiple threads? (-> shutdown() is never called for this executor)

Comment on attachment 8785064 [details] Bug 1295348 - Send/Handle push messages for send tab to device on Fennec. https://reviewboard.mozilla.org/r/74402/#review73124 Can I test this somehow locally? I assume that by default my builds won't have a push registration matching our server(s)?

(In reply to Sebastian Kaspari (:sebastian) from comment #6) > I'm a bit surprised to see that it's the responsibility of the client to > notify the other clients about changes. Is this by design? I'm not familiar > with the overall sync architecture and therefore just would like to know why > it's not the server sending the notification. :) Sebastian, you can find more context around the decision to trigger PNs from clients in these threads: https://mail.mozilla.org/pipermail/sync-dev/2016-May/001355.html https://github.com/mozilla/fxa-auth-server/issues/1316

Comment on attachment 8785064 [details] Bug 1295348 - Send/Handle push messages for send tab to device on Fennec. https://reviewboard.mozilla.org/r/74402/#review73120 > Are you sure the thread doesn't survive and we end up with multiple threads? (-> shutdown() is never called for this executor) shutdown() is called in the finalize method of the implementation of ExecutorService, we just need to wait for GC here (but I'll be happy to do it explicitely if you think it's valuable).

Comment on attachment 8785064 [details] Bug 1295348 - Send/Handle push messages for send tab to device on Fennec. https://reviewboard.mozilla.org/r/74402/#review73124 You can test it with the latest desktop nightly and send tabs between desktop and Android.

Comment on attachment 8785064 [details] Bug 1295348 - Send/Handle push messages for send tab to device on Fennec. https://reviewboard.mozilla.org/r/74402/#review73240 Since Sebastian tagged me, drive by review. ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:863 (Diff revision 1) > > resource.get(); > } > + > + @Override > + @SuppressWarnings("unchecked") Isolate "unchecked" code. ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:879 (Diff revision 1) > + > + final BaseResource resource; > + final ExtendedJSONObject body = new ExtendedJSONObject(); > + JSONArray to = new JSONArray(); > + to.addAll(deviceIds); > + body.put("to", to); Why not do `"to": "all"` here? https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#post-v1accountdevicesnotify suggests that `all` is appropriate if we want to notify everyone except for ourselves. Or are we concerned that our local list of devices might somehow be more complete/correct than what FxA has? ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:114 (Diff revision 1) > String pushCallback = subscription.getString("pushCallback"); > String pushPublicKey = subscription.getString("pushPublicKey"); > String pushAuthKey = subscription.getString("pushAuthKey"); > > final AndroidFxAccount fxAccount = AndroidFxAccount.fromContext(context); > - final byte[] sessionToken = getSessionToken(fxAccount); > + final byte[] sessionToken = fxAccount.getSessionToken(); I suppose in some bizzare case when an account removal is intertwined with fxa registration, `fxAccount` will be null! Perhaps add a sanity check while you're at it? ::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:611 (Diff revision 1) > } catch (Exception e) { > throw new IllegalStateException("could not get state", e); > } > } > > + @Nullable If we don't throw here, we do return a non-null session token (looking at the `TokensAndKeysState`'s constructor), so I don't think this annotation is correct. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:63 (Diff revision 1) > > public static final String COLLECTION_NAME = "clients"; > public static final String STAGE_NAME = COLLECTION_NAME; > public static final int CLIENTS_TTL_REFRESH = 604800000; // 7 days in milliseconds. > public static final int MAX_UPLOAD_FAILURE_COUNT = 5; > + public static final Integer NOTIFY_TAB_SENT_TTL_SECS = 1 * 3600; // 1 hour How did you arrive at 1hr as the TTL for this push message? I'm curious if it's documented somewhere to ensure other clients follow suit? While devices are offline, this push message will live for an hour before being purged from the (gcm, apns) servers. So, devices have an hour to "come online" (from push server's POV) to receive the "sync your collections" command. I think generally we want to keep this time window as small as possible, to reduce possibility of situations like this: - device A "sends a tab", tells FxA to deliver push message to device B - device B is on a spotty connection, and disconects from GCM. Its keep-alive window is around 15m, so it might not reconnect to push server for a while - device B does manage to sync though (connection is spotty, not absent), and receives the tab. Hurray! - sometime later, device B finally managed to re-register with GCM, and receives the push message telling it to sync off-cycle, which is now a wasted effort Also, nits: Just an `int` should be fine, and there's also https://developer.android.com/reference/java/util/concurrent/TimeUnit.html if you care to use it. e.g.: `TimeUnit.SECONDS.convert(1L, TimeUnit.HOURS)` ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:162 (Diff revision 1) > clientsDelegate.setClientsCount(clientsCount); > > // If we upload remote records, checkAndUpload() will be called upon > // upload success in the delegate. Otherwise call checkAndUpload() now. > if (toUpload.size() > 0) { > + // toUpload is cleared in uploadRemoteRecords, save what we need here `toUpload` is kind of a poor name choice here, it doesn't aid in understanding what this code actually does. Perhaps while you're looking at this stuff, consider renaming it to something like `clientsWithNewLocalCommands` or something similar? ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:170 (Diff revision 1) > + if (!TextUtils.isEmpty(record.fxaDeviceId)) { > + devicesToNotify.add(record.fxaDeviceId); > + } > + } > + > uploadRemoteRecords(); Does the `uploadRemoteRecords` call block until all client records (with commands) have been uploaded? My main concern here is that we don't want to push "get your commands now!" message to other clients before we actually uploaded our commands. If it `uploadRemoteRecords` is indeed async, things get a bit more complicated - you'll need to ensure `notifyClients` gets called only once, and after all uploads are done. If `uploadRemoteRecords` blocks, add a comment here saying so as a warning to future maintainers. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:180 (Diff revision 1) > return; > } > checkAndUpload(); > } > > + @SuppressWarnings("unchecked") Please isolate code that provokes unchecked warnings (payload build up?) into its own little method. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:190 (Diff revision 1) > + if (account == null) { > + return; > + } > + final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account); > + final FxAccountClient fxAccountClient = new FxAccountClient20(fxAccount.getAccountServerURI(), executor); > + ExtendedJSONObject payload = new ExtendedJSONObject(); Add a link to the API doc somewhere in a comment here for reference, https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#post-v1accountdevicesnotify ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:199 (Diff revision 1) > + JSONArray collections = new JSONArray(); > + collections.add("clients"); > + data.put("collections", collections); > + payload.put("data", data); > + try { > + fxAccountClient.notifyDevices(fxAccount.getSessionToken(), devicesToNotify, payload, NOTIFY_TAB_SENT_TTL_SECS, new FxAccountClient20.RequestDelegate<ExtendedJSONObject>() { Perhaps try/catch just the .getSessionToken call for clarity?

Comment on attachment 8785064 [details] Bug 1295348 - Send/Handle push messages for send tab to device on Fennec. https://reviewboard.mozilla.org/r/74402/#review73240 > Why not do `"to": "all"` here? https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#post-v1accountdevicesnotify suggests that `all` is appropriate if we want to notify everyone except for ourselves. > > Or are we concerned that our local list of devices might somehow be more complete/correct than what FxA has? We do not want to notify everyone here, only the devices who had their client record written with a new command.

Patch amended with your comments, thanks!

Comment on attachment 8785064 [details] Bug 1295348 - Send/Handle push messages for send tab to device on Fennec. https://reviewboard.mozilla.org/r/74402/#review73374 A few nits, but the code looks like it's doing what you intended! ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:865 (Diff revision 3) > > resource.get(); > } > + > + @Override > + public void notifyDevices(byte[] sessionToken, List<String> deviceIds, ExtendedJSONObject payload, Long TTL, RequestDelegate<ExtendedJSONObject> delegate) { @NonNull sessionToken, deviceIds ::: mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java:901 (Diff revision 3) > + post(resource, body); > + } > + > + @NonNull > + @SuppressWarnings("unchecked") > + private ExtendedJSONObject createNotifyDevicesBody(List<String> deviceIds, ExtendedJSONObject payload, Long TTL) { `@NonNull List<String> deviceIds` ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:166 (Diff revision 3) > // If we upload remote records, checkAndUpload() will be called upon > // upload success in the delegate. Otherwise call checkAndUpload() now. > - if (toUpload.size() > 0) { > + if (modifiedClientsToUpload.size() > 0) { > + // modifiedClientsToUpload is cleared in uploadRemoteRecords, save what we need here > + final List<String> devicesToNotify = new ArrayList<>(); > + for(ClientRecord record : modifiedClientsToUpload) { nit: missing space will probably fail our checkstyle, ("for (") ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:172 (Diff revision 3) > + if (!TextUtils.isEmpty(record.fxaDeviceId)) { > + devicesToNotify.add(record.fxaDeviceId); > + } > + } > + > + // This method is synchronous, there's no risk to notify the clients "no risk of notifying" ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:185 (Diff revision 3) > } > checkAndUpload(); > } > > + private void notifyClients(final List<String> devicesToNotify) { > + ExecutorService executor = Executors.newSingleThreadExecutor(); nit here and elsewhere: final everywhere that's appropriate, otherwise it's a bit of a mixed bag and you start to wonder why some things are final and some aren't.

Thanks Grisha!

Comment on attachment 8785064 [details] Bug 1295348 - Send/Handle push messages for send tab to device on Fennec. https://reviewboard.mozilla.org/r/74402/#review73534

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1f51d3cd5250 Send/Handle push messages for send tab to device on Fennec. r=Grisha,sebastian