Closed
Bug 1214338
Opened 9 years ago
Closed 9 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•9 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•9 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•9 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•9 years ago
|
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment 5•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Updated•9 years ago
|
Attachment #8726003 -
Flags: review?(rnewman) → review+
Updated•4 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
•