Closed Bug 1214338 Opened 9 years ago Closed 9 years ago

Implement Android GCM-based PushService protocol

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox44 affected, firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox44 --- affected
firefox47 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

This ticket tracks implementing a PushService protocol, analogous to PushService{Http2,WebSocket} that co-ordinates with Fennec's GCM and Autopush implementation to register, unregister, and deliver push messages.
Comment on attachment 8726003 [details] MozReview Request: Bug 1214338 - Implement Android GCM-based PushService protocol. r=rnewman r?kitcambridge https://reviewboard.mozilla.org/r/37753/#review34493 This looks really good, Nick! Thanks so much for doing this. ::: dom/push/PushServiceAndroidGCM.jsm:76 (Diff revision 1) > + if (prefs.get("debug") && serverURI.scheme == "http") { I wonder if it makes sense to have the `prefs.get("debug")` check in `PushService._findService` (https://dxr.mozilla.org/mozilla-central/rev/4ea7408b3eef059aa248f4b00328f8fdb4475112/dom/push/PushService.jsm#350), and have it skip `isURIPotentiallyTrustworthy` if that's the case. ::: dom/push/PushServiceAndroidGCM.jsm:91 (Diff revision 1) > + return; Nit: please move this `return` one level out. ::: dom/push/PushServiceAndroidGCM.jsm:132 (Diff revision 1) > + cryptoParams = { This will need some minor changes to handle the new encryption scheme. `PushCrypto.getCryptoParams` can help clean this up: cryptoParams = getCryptoParams({ encryption_key: data.enckey, crypto_key: data.cryptokey, encryption: data.enc, }); ::: dom/push/PushServiceAndroidGCM.jsm:193 (Diff revision 1) > + if (subscriptions.hasOwnProperty(record.channelID)) { Nit: For consistency, please use `record.keyID`. ::: dom/push/PushServiceAndroidGCM.jsm:235 (Diff revision 1) > + return PushCrypto.generateKeys() Ugh, sorry about all the duplication. It would be nice if `PushService.jsm` could take care of this, so we don't have to generate the keys everywhere. ::: dom/push/PushServiceAndroidGCM.jsm:244 (Diff revision 1) > + quota: record.maxQuota, It's OK to remove this line; `maxQuota` is no longer used. ::: dom/push/PushServiceAndroidGCM.jsm:248 (Diff revision 1) > + p256dhPrivateKey: exportedKeys[1], We'll want to generate the secret here, too: `authenticationSecret: PushCrypto.generateAuthenticationSecret()`. ::: dom/push/PushServiceAndroidGCM.jsm:257 (Diff revision 1) > + channelID: record.channelID, Nit: record.keyID ::: dom/push/PushServiceAndroidGCM.jsm:275 (Diff revision 1) > +// Should we expose the channelID in this way? To both places? Actually, I think you can remove both these overrides. These methods are only used to send the subscription information over IPC, which `nsIPushService` rewraps in an `nsIPushSubscription` (https://dxr.mozilla.org/mozilla-central/source/dom/push/PushComponents.js#75,119,139,353). ::: dom/push/moz.build:17 (Diff revision 1) > 'PushServiceHttp2.jsm', Do you think we can skip these modules for Fennec, too? ::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java:166 (Diff revision 1) > + data.put("enckey", bundle.getString("enckey")); I think we'll want to forward `cryptokey` to Gecko here, too. autopush should send `"cryptokey"` xor `"enckey"` in the body.
Attachment #8726003 - Flags: review?(kcambridge) → review+
https://reviewboard.mozilla.org/r/37753/#review34833 ::: dom/push/PushServiceAndroidGCM.jsm:76 (Diff revision 1) > + if (prefs.get("debug") && serverURI.scheme == "http") { I'd be happy to unify this, but I also want to do GCM-specific things in response to "dom.push.debug" toggling, so I'm not going to lift it to the service for this ticket. ::: dom/push/PushServiceAndroidGCM.jsm:235 (Diff revision 1) > + return PushCrypto.generateKeys() I agree, and previously filed Bug 1214370 to track it. Not this ticket, though :) ::: dom/push/PushServiceAndroidGCM.jsm:275 (Diff revision 1) > +// Should we expose the channelID in this way? To both places? Dropped. ::: dom/push/moz.build:17 (Diff revision 1) > 'PushServiceHttp2.jsm', I think we can, although I want to run tests against them. I'll try disabling them and see how we go. ::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java:166 (Diff revision 1) > + data.put("enckey", bundle.getString("enckey")); Updated.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Attachment #8726003 - Flags: review?(rnewman) → review+
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

Created:
Updated:
Size: