Closed Bug 1287643 Opened 8 years ago Closed 8 years ago

Implement FxA Push Endpoint registration on Fennec

Categories

(Firefox for Android Graveyard :: Firefox Accounts, defect, P1)

Product:
Firefox for Android Graveyard
Component:
Firefox Accounts
Type:
defect

Tracking

(firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: eoger, Assigned: eoger)

References

Details

(Whiteboard: [send-tab][MobileAS][fxsync])

Attachments

(2 files, 1 obsolete file)

Bug 1287643 - FxA Push registration and handling of device disconnection message.
58 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1287643 - Add Push crypto params to FxAccountDevice.
58 bytes, text/x-review-board-request
nalexander
: review+
Details 
In order to support new features like "better latency for send tabs to device" or "improve timeliness of Sync", we need the FxA background service to be able to subscribe to push notifications and send the subscription details to the fxa-auth-server.
This is what I've been able to come up with so far but now I'm stuck.
Sorry about the bad code it is very much a POC.
So far the push registration is done by sending messages between chrome and gecko and is "working", because the FxaRegistrator knows that it has to wait for these messages.

The next step is to pass impromptu decrypted push messages from gecko to chrome. We could use the same messaging mechanism, but I guess in that case we would need a service to listen to the messages in Java?
Am I going in the right direction here?
Flags: needinfo?(nalexander)
Comment on attachment 8773457 [details]
Bug 1287643 - Add Push crypto params to FxAccountDevice.

https://reviewboard.mozilla.org/r/66206/#review64172

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/fxa/login/MockFxAccountClient.java:190
(Diff revision 1)
>          if (existingDevice != null) {
> +          String deviceName = existingDevice.name;
>            if (!TextUtils.isEmpty(deviceToRegister.name)) {
> -            existingDevice.name = deviceToRegister.name;
> +            deviceName = deviceToRegister.name;
>            } // We could also update the other fields..
> +          FxAccountDevice device = new FxAccountDevice(deviceName, existingDevice.id, existingDevice.type,

Not needed now, but consider a builder/modifier pattern of `newDevice = oldDevice.withCryptoParams(...)` in future.  It's a nice way to keep immutable data (good) with clear APIs (also good).
Attachment #8773457 - Flags: review+
Comment on attachment 8773458 [details]
Bug 1287643 - POC registration.

https://reviewboard.mozilla.org/r/66208/#review64174

Sorry for the delayed response -- it took me a while to muster the energy to engage with this.

What I think the current code is doing is as follows.  The FxA/device
manager Java code tries to start Gecko and message pass with it in order
to register and maintain a Push subscription.  This is similar to what
happens on Desktop (reading
https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsPush.js).

Android was designed from the beginning to be able to register Push
subscriptions and receive Push messages without interacting with Gecko
at all.  (The only thing we did not anticipate was deciding not to
implement the crypto scheme in Java.)  I would like, therefore, to try
to use the Java API rather than try to start Gecko to do the Push
registration.  I haven't understood the caching completely, but I am
concerned we'd start Gecko frequently (potentially every Sync), at great
memory and battery cost.

Step 1 will be to ensure that the PushService is available during FxA
device registration (even when Gecko is not).  See
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java#192
for doing this using reflection.  We should be safe to get this
reference and talk to it exactly like we do at
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/gcm/GcmMessageListenerService.java#33,
even though that method registers Gecko event listeners (and Gecko
won't be around).  You can check with jchen if this is not clear.

Step 2 will be to use the PushManager in the PushService to register a
special Push channel subscription (just like at
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/push/PushService.java#338).

I see two forks here.  Fork a), which is what I envisioned originally,
is to register with a service that is FxA specific (so not
`SERVICE_WEBPUSH`), and to handle incoming messages with the new service
specially around
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/push/PushService.java#155.
You can see that this code ferries the message to Gecko, starting it if
necessary; jchen is the expert here.  This would be the point to do the
decryption, if we were going to do it in Java.  In this fork, we will
need to add a new "access point" into the JavaScript PushCrypto that
just does the decoding here, and sends the decoded bits back to the Java
side, ready to delegate to FxA.  This is, conceptually, similar to
having a Push endpoint registered for the "pseudo-origin"
chrome://fxa-device-update, like on Desktop.  I think this would look
re-use the existing "android-push-service" service start intent (see
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/push/PushService.java#176)
and would just add a new Messaging.jsm message
("PushServiceAndroidGCM:ReceivedPushMessageToDecode") around
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/push/PushService.java#215.
Then, around
https://dxr.mozilla.org/mozilla-central/source/dom/push/PushServiceAndroidGCM.jsm#122,
rather than deliver the message into Gecko's guts, we'd just decode it
and send the data back to Java.  Awkward?  Yes.

Fork b), is to register a special SERVICE_WEBPUSH endpoint (say for
"chrome://fxa-device-update", like on Desktop).  The tricky thing here
is that the Gecko registration will not be in place if you do only the
Java side (which is all we would do in fork a) above).  In this case,
some variation on what you have written would be needed (although I
think a lot of the message passing and PushService initialization could
be greatly simplified).

Looking back, I suppose Step 3 is to handle incoming messages; I mostly
covered that above.

I am hand waving `appServerKey` and where any other crypto bits that
this new SERVICE_FXA_PUSH would need are stored.  (I think they are
stored explicitly with the device configuration in your earlier patch,
but I may be getting confused.  If the registration process needs to
/generate/ them, then life gets harder, since now we need Gecko for that
too... sigh.)

Eduard, can you read my fork a) above and see if you can make progress
with that, and then what questions you have?  Thanks!
Attachment #8773458 - Flags: review-
Comment on attachment 8773456 [details]
Bug 1287643 - FxA Push registration and handling of device disconnection message.

https://reviewboard.mozilla.org/r/66204/#review64494

Fold this into some larger work.
Attachment #8773456 - Flags: review-
https://reviewboard.mozilla.org/r/66208/#review64496

Marking this as not ready to land.  Re-flag me whenever.
Whiteboard: [send-tab] → [send-tab][MobileAS s1.1]
Comment on attachment 8773457 [details]
Bug 1287643 - Add Push crypto params to FxAccountDevice.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66206/diff/1-2/
Attachment #8773457 - Attachment description: Bug 1287643 - Add Push crypto params to FxAccountDevice. r? → Bug 1287643 - Add Push crypto params to FxAccountDevice.
Attachment #8773456 - Attachment description: Bug 1287643 - Add systemRecord property to Android Push subscriptions. r? → Bug 1287643 - FxA Push registration and handling of device disconnection message.
Attachment #8773456 - Flags: review- → review?(nalexander)
Comment on attachment 8773456 [details]
Bug 1287643 - FxA Push registration and handling of device disconnection message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66204/diff/1-2/
Attachment #8773458 - Attachment is obsolete: true
Thanks a lot for your explanations Nick, it unblocked me and I was able to finish a first implementation of the patch. This patch does the registration, the delivery and the handling of FxA push messages.
I implemented an action for the device_disconnected fxa message.

> If the registration process needs to
/generate/ them, then life gets harder, since now we need Gecko for that
too... sigh.)

Yup, this is why I'm keeping the subscription part within gecko, sorry :) (but we don't do this very often with the caching so we're ok here)
Comment on attachment 8773456 [details]
Bug 1287643 - FxA Push registration and handling of device disconnection message.

https://reviewboard.mozilla.org/r/66204/#review65476

This is looking much better.  Page jchen to discuss what the real way to go from Java -> Gecko is, given that Gecko needs to be started if it's not running.  Messaging.jsm, starting the Gecko thread, installing listeners -- that feels wrong to me.  (jchen may, of course, correct me :)

::: dom/push/PushServiceAndroidGCM.jsm:132
(Diff revision 2)
> -      this._mainPushService.receivedPushMessage(
> +        this._mainPushService.receivedPushMessage(
> -        data.channelID, "", message, cryptoParams, (record) => {
> +          data.channelID, "", message, cryptoParams, (record) => {
> -          // Always update the stored record.
> +            // Always update the stored record.
> -          return record;
> +            return record;
> -        });
> +          });
> +      } else /* topic == "PushServiceAndroidGCM:ReceivedPushMessageToDecode" */ {

Be explicit here, duplicating the condition.  No sense getting caught out later.

::: dom/push/PushServiceAndroidGCM.jsm:133
(Diff revision 2)
> -        data.channelID, "", message, cryptoParams, (record) => {
> +          data.channelID, "", message, cryptoParams, (record) => {
> -          // Always update the stored record.
> +            // Always update the stored record.
> -          return record;
> +            return record;
> -        });
> +          });
> +      } else /* topic == "PushServiceAndroidGCM:ReceivedPushMessageToDecode" */ {
> +        this._mainPushService.getByKeyID(data.channelID)

Not loving this duplication. Can we expose this more usefully?  Since Gecko is running, can we make the "to decode" flow be "standard" for the special origin "chrome://fxa-device-push", and have that origin have a service worker that knows how to message back to Java?

I worry about duplication and divergent implementations.

::: dom/push/PushServiceAndroidGCM.jsm:152
(Diff revision 2)
> +          )
> +        })
> +        .then(message => {
> +          message = this._decoder.decode(message);
> +          Messaging.sendRequest({
> +            __messagingguid__: data.__messagingguid__,

I expect you can use `Messaging.addListener` to avoid manually handling these GUIDs.  `addListener` is internally backed by the observer service, but it's nice to avoid code duplication.

If that's posssible, move `PushServiceAndroidGCM:ReceivedPushMessage` to `addListener` as well.

::: dom/push/PushServiceAndroidGCM.jsm:164
(Diff revision 2)
> +        })
> +        .catch(err => {
> +          Messaging.sendRequest({
> +            __messagingguid__: data.__messagingguid__,
> +            type: "PushServiceAndroidGCM:ReceivedPushMessageToDecode:Response",
> +            status: "failure"

You're muffling `err` here.  Log it and/or send it through for logging on the Java side.

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java:156
(Diff revision 2)
>              // could try to drop the remote subscription?
>              Log.e(LOG_TAG, "No subscription found for chid: " + chid + "; ignoring message.");
>              return;
>          }
>  
> +        boolean isWebPush = SERVICE_WEBPUSH.equals(subscription.service);

nit: `isFxAPush` as well.  Be consistent, pave the way for the next consumer, etc.

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java:295
(Diff revision 2)
>                  // instead of being queued.
>                  canSendPushMessagesToGecko = true;
>                  for (JSONObject pushMessage : pendingPushMessages) {
>                      final String profileName = pushMessage.optString("profileName", null);
>                      final String profilePath = pushMessage.optString("profilePath", null);
> +                    final boolean isWebPush = pushMessage.optBoolean("isWebPush", true);

Prefer sets to booleans, so `service` to `isWebPush`.

::: mobile/android/base/java/org/mozilla/gecko/util/Messaging.java:17
(Diff revision 2)
> +import java.util.HashMap;
> +import java.util.Map;
> +import java.util.UUID;
> +
> +/**
> + * This class tries to reproduce what Messaging.jsm does on the chrome side

I feel like this whole class is not necessary.  `addListener` already exists; modulo Gecko-load-order-life-cycle, this shouldn't be needed.  And if Gecko isn't loaded, the existing Push stuff shows how to start Gecko by sending an Intent that starts an XPCOM service, etc.

Ask for jchen's feedback on this.

::: mobile/android/chrome/content/FxAccountPush.js:54
(Diff revision 2)
> +      });
> +    });
> +  },
> +
> +  observe(subject, topic, data) {
> +    Log.i(LOG_TAG, `observed topic=${topic}, data=${data}, subject=${subject}`);

This could be PII, right?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:35
(Diff revision 2)
> +                Log.e(LOG_TAG, "Could not parse JSON", e);
> +                return;
> +            }
> +        }
> +        if (message == null) {
> +            // The empty signal is used to check the verification state of the account, but I can't

nit: fix "but I can't".

Generally, elaborate on this.  Do you mean that the service might push an empty body and expect a response with the verification state?

There's no reason you can't register with Push before Married, BTW -- if you can do it on any other platform, you should be able to get the tokens, etc you need to do it here...

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountDeletedService.java:69
(Diff revision 2)
>        return;
>      }
>  
>  
> +    // Unsubscribe push
> +    Messaging messaging = Messaging.getInstance();

This approach sadly doesn't make much sense.  Gecko might not be running; you'll need to launch an Intent with data to start it and ferry the messages over.

Yes, this is very complicated -- but that's the technical stack we have :(
Attachment #8773456 - Flags: review?(nalexander) → review-
Whiteboard: [send-tab][MobileAS s1.1] → [send-tab][MobileAS s1.1][fxsync]
Comment on attachment 8773456 [details]
Bug 1287643 - FxA Push registration and handling of device disconnection message.

Jchen could you take a look at this (especially the communication between gecko and java)?
Attachment #8773456 - Flags: feedback?(nchen)
https://reviewboard.mozilla.org/r/66204/#review66032

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java:214
(Diff revision 2)
>  
> -            if (canSendPushMessagesToGecko) {
> +        if (canSendPushMessagesToGecko) {
> +            if (isWebPush) {
>                  sendMessageToGeckoService(data);
>              } else {
> -                Log.i(LOG_TAG, "Service not initialized, adding message to queue.");
> +                decodeAndHandleFxAMessage(context, data);

Instead of adding `decodeAndHandleFxAMessage` (and Messaging.java), you should continue to use sendMessageToGeckoService. Just register for "PushServiceAndroidGCM:ReceivedPushMessageToDecode:Response" inside FxAccountPushHandler.java to receive the response.

::: mobile/android/base/java/org/mozilla/gecko/util/Messaging.java:19
(Diff revision 2)
> +import java.util.UUID;
> +
> +/**
> + * This class tries to reproduce what Messaging.jsm does on the chrome side
> + */
> +public class Messaging implements BundleEventListener {

Again, I don't think it's necessary to add Messaging.java just for this use case. Also, we try to use reflection as sparingly as possible, and unfortunately this class uses too much reflection.
Attachment #8773456 - Flags: feedback?(nchen) → feedback+
Comment on attachment 8773457 [details]
Bug 1287643 - Add Push crypto params to FxAccountDevice.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66206/diff/2-3/
Attachment #8773456 - Flags: review?(nalexander)
Attachment #8773456 - Flags: review-
Attachment #8773456 - Flags: feedback+
Comment on attachment 8773456 [details]
Bug 1287643 - FxA Push registration and handling of device disconnection message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66204/diff/2-3/
Thank you both for your feedback, I addressed most of your comments.
Flags: needinfo?(nalexander)
Comment on attachment 8773456 [details]
Bug 1287643 - FxA Push registration and handling of device disconnection message.

https://reviewboard.mozilla.org/r/66204/#review67212
Attachment #8773456 - Flags: review?(nalexander) → review+
Whiteboard: [send-tab][MobileAS s1.1][fxsync] → [send-tab][MobileAS s1.2][fxsync]
Comment on attachment 8773456 [details]
Bug 1287643 - FxA Push registration and handling of device disconnection message.

https://reviewboard.mozilla.org/r/66204/#review67362

::: mobile/android/base/moz.build:165
(Diff revision 3)
>      CONFIG['ANDROID_SUPPORT_ANNOTATIONS_JAR_LIB'],
>      CONFIG['ANDROID_SUPPORT_V4_AAR_LIB'],
>      CONFIG['ANDROID_SUPPORT_V4_AAR_INTERNAL_LIB'],
>      CONFIG['ANDROID_APPCOMPAT_V7_AAR_LIB'],
>      'constants.jar',
> +    'gecko-browser.jar',

Oh, yeah, this is never going to work.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:180
(Diff revision 3)
>        return null;
>      }
>    }
>  
>    @Nullable
> -  private static byte[] getSessionToken(final AndroidFxAccount fxAccount) throws InvalidFxAState {
> +  private byte[] getSessionToken(final AndroidFxAccount fxAccount) throws InvalidFxAState {

Why are these not static?  They need to be static.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountDeletedService.java:72
(Diff revision 3)
>            "deleted Account.");
>        return;
>      }
>  
>  
> +    // Fire up gecko and unsubscribe push

This is the only services -> gecko-browser dependency, and the Intent launching stuff is supposed to be the way to access Gecko without importing the code.  We should be able to make the intent to launch the push service *and message the observers from Gecko after it's launched* without needing the actual code.  That is, you should be able to glue together the intent either via reflection or via just inserting the correct values.  (You can fudge the profile stuff, perhaps assume it's always default, or investigate the ProfileProvider.)  jchen can help you set up the messaging after the intent is received by Gecko.
Comment on attachment 8773456 [details]
Bug 1287643 - FxA Push registration and handling of device disconnection message.

I corrected the circular dependency issue between gecko-browser and services by:

- Modifying GeckoService so it can send events right after starting the Gecko thread (by specifying them in the Intent).
  We also forge the Intent by ourselves.
  This allows us to remove the GeckoService/GeckoProfile direct references and the call to |GeckoAppShell.notifyObservers()| afterwards.

- Holding a weak reference to the app context instead of using |GeckoAppShell.getApplicationContext()| in the callback.

- Using reflection to call |EventDispatcher.getInstance().registerBackgroundThreadListener()| (I'm not super happy but that's the only place where we do it, so I'd say it's okay)
Attachment #8773456 - Flags: review?(nchen)
Sorry I should have commented after the PR, see my comment above ^
Attachment #8773456 - Flags: review?(nchen)
Comment on attachment 8773456 [details]
Bug 1287643 - FxA Push registration and handling of device disconnection message.

https://reviewboard.mozilla.org/r/66204/#review67810

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:294
(Diff revisions 3 - 4)
> +          InvocationTargetException, IllegalAccessException {
> +    // We have no choice but to use reflection here, sorry :(
> +    Class<?> eventDispatcher = Class.forName("org.mozilla.gecko.EventDispatcher");
> +    Method getInstance = eventDispatcher.getMethod("getInstance");
> +    Object instance = getInstance.invoke(null);
> +    Method registerBackgroundThreadListener = eventDispatcher.getMethod("registerBackgroundThreadListener",

Make sure you put "@ReflectionTarget" annotations on `EventDispatcher.getInstance` and `EventDispatcher.registerBackgroundThreadListener`

::: mobile/android/chrome/content/browser.js:158
(Diff revision 4)
>    ["Feedback", ["Feedback:Show"], "chrome://browser/content/Feedback.js"],
>    ["SelectionHandler", ["TextSelection:Get"], "chrome://browser/content/SelectionHandler.js"],
>    ["EmbedRT", ["GeckoView:ImportScript"], "chrome://browser/content/EmbedRT.js"],
>    ["Reader", ["Reader:AddToCache", "Reader:RemoveFromCache"], "chrome://browser/content/Reader.js"],
>    ["PrintHelper", ["Print:PDF"], "chrome://browser/content/PrintHelper.js"],
> +  ["FxAccountPush", ["FxAccountPush:Subscribe", "FxAccountPush:Unsubscribe", /* OBSERVER_TOPIC_SUBSCRIPTION_CHANGE */ "push-subscription-change"], "chrome://browser/content/FxAccountPush.js"],

Listening to "FxAccountPush:Subscribe" this way is probably not going to work if Fennec is not already running. browser.js is only loaded when the Fennec activity (GeckoApp) is running. GeckoService cannot load browser.js; only GeckoApp can. So FxAccountPush.js will only be loaded and listening to "FxAccountPush:Subscribe" when the user has already launched Fennec, which doesn't sound like what you want.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoService.java:36
(Diff revision 4)
> +    private static final String INTENT_EVENT = "org.mozilla.gecko.intent.INTENT_EVENT";
> +    private static final String INTENT_EVENT_DATA = "org.mozilla.gecko.intent.INTENT_EVENT_DATA";
> +
>      private static final String INTENT_ACTION_UPDATE_ADDONS = "update-addons";
>      private static final String INTENT_ACTION_CREATE_SERVICES = "create-services";
> +    private static final String INTENT_ACTION_NOTIFY_EVENT = "notify-event";

If you want to use a different observer topic than the category name, instead of adding "notify-event", I think you should modify "create-services" to add an extra `topic` parameter. See [1] on how to add a new parameter to createServices.

[1] https://hg.mozilla.org/integration/fx-team/rev/079e427f6ac3

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoService.java:157
(Diff revision 4)
> -        final String profileDir = intent.getStringExtra(INTENT_PROFILE_DIR);
> +        String profileDir = intent.getStringExtra(INTENT_PROFILE_DIR);
>  
> -        if (profileName == null || profileDir == null) {
> -            throw new IllegalArgumentException("Intent must specify profile.");
> +        if (profileName == null && profileDir == null) {
> +            GeckoProfile geckoProfile = GeckoProfile.get(getApplicationContext());
> +            profileName = geckoProfile.getName();
> +            profileDir = geckoProfile.getDir().getAbsolutePath();

We intentionally don't want a "default profile" option. You should figure out how to get the profile name/dir associated with the current Fx accounts.
Attachment #8773456 - Flags: review?(nchen)
See Also: → 1293798
Comment on attachment 8773456 [details]
Bug 1287643 - FxA Push registration and handling of device disconnection message.

https://reviewboard.mozilla.org/r/66204/#review67810

> If you want to use a different observer topic than the category name, instead of adding "notify-event", I think you should modify "create-services" to add an extra `topic` parameter. See [1] on how to add a new parameter to createServices.
> 
> [1] https://hg.mozilla.org/integration/fx-team/rev/079e427f6ac3

But if I modify the topic we will never launch the push service (see https://dxr.mozilla.org/mozilla-central/source/dom/push/PushComponents.js#88).
I need to send another topic here (after we launch the push service), this is why I'm adding another key to the Intent. Is there a better way to do this?
Flags: needinfo?(nchen)
(In reply to Edouard Oger [:eoger] from comment #25)
> Comment on attachment 8773456 [details]
> Bug 1287643 - FxA Push registration and handling of device disconnection
> message.
> 
> But if I modify the topic we will never launch the push service (see
> https://dxr.mozilla.org/mozilla-central/source/dom/push/PushComponents.
> js#88).
> I need to send another topic here (after we launch the push service), this
> is why I'm adding another key to the Intent. Is there a better way to do
> this?

Why not launch the push service from your own observer handler? That way you're not relying on the "android-push-service" handler.

Or you can stay with "android-push-service", and send it some data to tell it to subscribe to FxA push for you. That also lets you avoid using "FxAccountPush:Subscribe", which is probably not going to work because it depends on browser.js being loaded by GeckoApp.
Flags: needinfo?(nchen)
Thank you Jim, I addressed your comments
Blocks: 1295348
Addressed jchen's last comments on IRC about the magic ##auto## value in GeckoService (we pass null instead now)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e72a4a5ae5ff
Add Push crypto params to FxAccountDevice. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/816516be0183
FxA Push registration and handling of device disconnection message. r=nalexander
Keywords: checkin-needed
Backed out in https://hg.mozilla.org/integration/autoland/rev/def48ff46458 for (at least, so far) ASan and Win7 timeouts in test_notification_ack.js and five others, https://treeherder.mozilla.org/logviewer.html#?job_id=2041530&repo=autoland
Flags: needinfo?(s.kaspari)
Comment on attachment 8773456 [details]
Bug 1287643 - FxA Push registration and handling of device disconnection message.

https://reviewboard.mozilla.org/r/66204/#review69914

Sorry for the drive-by! I left some comments about the dom/push side of things.

::: dom/push/PushComponents.js:92
(Diff revision 6)
>      }
>      if (topic === "android-push-service") {
>        // Load PushService immediately.
>        this._handleReady();
> +      if (data === "android-fxa-subscribe") {
> +        Services.obs.notifyObservers(null, "PushServiceAndroidGCM:FxASubscribe", null);

Push backend initialization is async (on top of `PushService` initialization, which is also async)...so this might notify observers before `PushServiceAndroidGCM` is listening for them. I'm really sorry that code is so complicated.

If I understand your patch correctly, though, it seems like you don't need all the service machinery just to decrypt a message. Instead of the observer notification, how would you feel about adding a separate Android-specific service that listens for "PushServiceAndroidGCM:ReceivedPushMessageToDecode" and "PushServiceAndroidGCM:FxASubscribe". That component could import `resource://gre/modules/PushCrypto.jsm`, and you could use the common crypto library without having to deal with the push service at all.

::: dom/push/PushService.jsm:895
(Diff revision 6)
>        cryptoParams.salt,
>        cryptoParams.rs,
>        record.authenticationSecret,
>        cryptoParams.padSize
> -    ).then(message => this._notifyApp(record, messageID, message), error => {
> +    )
> +    .catch(error => {

Please move this catch into `_decryptAndNotifyApp`, too.

::: dom/push/PushServiceAndroidGCM.jsm:318
(Diff revision 6)
>    reportDeliveryError: function(messageID, reason) {
>      console.warn("reportDeliveryError: Ignoring message delivery error",
>        messageID, reason);
>    },
> +
> +  _subscribeFxAPush() {

I'm not a fan of having FxA-specific stuff in the DOM API, but the push code is already rather entangled, so I'll leave it to you to decide. :-)

If you're up for it, please make a separate service to manage this (and "PushServiceAndroidGCM:ReceivedPushMessageToDecode") in http://searchfox.org/mozilla-central/source/mobile/android/components, kind of like how `FxAccountsPush.jsm` handles this on Desktop.

If not, you can do `this._mainPushService.register({ scope: FXA_PUSH_SCOPE, originAttributes: "", systemRecord: true }).then(record => { /* ... */ })` here, to avoid roundtripping through the XPCOM machinery. The interesting properties on the record are http://searchfox.org/mozilla-central/rev/b35975ec17c8227e827800fa2d9642655cb103a8/dom/push/PushRecord.jsm#36,41,43.
No longer depends on: 1296335
Kit can you give it a final look?
Flags: needinfo?(kcambridge)
Comment on attachment 8773456 [details]
Bug 1287643 - FxA Push registration and handling of device disconnection message.

https://reviewboard.mozilla.org/r/66204/#review70580

Looks awesome; thanks for doing this! A question about using the category instead of an extra observer notification, and a nit.

::: dom/push/PushComponents.js:88
(Diff revision 7)
>      if (topic === "sessionstore-windows-restored") {
>        Services.obs.removeObserver(this, "sessionstore-windows-restored");
>        this._handleReady();
>        return;
>      }
>      if (topic === "android-push-service") {

Would it work if you added `category android-push-service FxAccountsPush @mozilla.org/fxa-push;1` to the Android manifest? It *seems* like `NS_CreateServicesFromCategory` will do the right thing and create both the Push and FxAccountsPush services, without needing this observer. No worries if you already tried it and it didn't work.

::: dom/push/PushComponents.js:506
(Diff revision 7)
>     */
>    get isSystemSubscription() {
>      return !!this._props.systemRecord;
>    },
>  
> +  /** The private key used to decrypt incoming push messages */

Nit: Let's add ", in JWK format" to the end, for clarity.

::: dom/push/PushService.jsm:881
(Diff revision 7)
> -   * @param {String} messageID The message ID.
>     * @param {ArrayBuffer|Uint8Array} data The encrypted message data.
>     * @param {Object} cryptoParams The message encryption settings.
> -   * @returns {Promise} Resolves with an ack status code.
> +   * @returns {Promise} Resolves with the decrypted message.
>     */
> -  _decryptAndNotifyApp(record, messageID, data, cryptoParams) {
> +  _decryptMessage(data, record, cryptoParams) {

Thanks for cleaning this up.

::: dom/push/PushServiceAndroidGCM.jsm:96
(Diff revision 7)
> +      default:
> +        break;
>      }
> +  },
>  
> -    if (topic == "PushServiceAndroidGCM:ReceivedPushMessage") {
> +  _onPushMessageReceived(data) {

Nice refactoring here, too!

::: mobile/android/components/FxAccountsPush.js:112
(Diff revision 7)
> +    return new Promise((resolve, reject) => {
> +      PushService.getSubscription(FXA_PUSH_SCOPE,
> +        Services.scriptSecurityManager.getSystemPrincipal(),
> +        (result, subscription) => {
> +          if (!subscription) {
> +            return reject(null);

Nit: Please reject with an error, so that it shows up in the log.
Attachment #8773456 - Flags: review+
Flags: needinfo?(kcambridge)
Whiteboard: [send-tab][MobileAS s1.2][fxsync] → [send-tab][MobileAS][fxsync]
Thank you kit, that was super helpful!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/800f4b90c116
Add Push crypto params to FxAccountDevice. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/acaa4a5cbb8c
FxA Push registration and handling of device disconnection message. r=kitcambridge,nalexander
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/800f4b90c116
https://hg.mozilla.org/mozilla-central/rev/acaa4a5cbb8c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Flags: needinfo?(s.kaspari)
Iteration: --- → 1.3
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: