Closed
Bug 1214338
Opened 9 years ago
Closed 8 years ago
Implement Android GCM-based PushService protocol
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox44 affected, firefox47 fixed)
RESOLVED
FIXED
Firefox 47
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.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/37753/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37753/
Attachment #8726003 -
Flags: review?(rnewman)
Attachment #8726003 -
Flags: review?(kcambridge)
Comment 2•8 years ago
|
||
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+
Assignee | ||
Comment 3•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment 5•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/49acabe9d6b6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Updated•8 years ago
|
Attachment #8726003 -
Flags: review?(rnewman) → review+
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•