Closed
Bug 1166350
Opened 9 years ago
Closed 9 years ago
Push notification should use the correct principal when calling ServiceWorkerManager
Categories
(Core :: DOM: Service Workers, defect)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: baku, Assigned: nsm)
References
Details
Attachments
(1 file, 1 obsolete file)
This is a follow up of bug 1162088.
Assignee | ||
Comment 2•9 years ago
|
||
Bug 1166350 - Push fixes for principals. r?dougt Fix xpcshell tests. Add support for webapps-clear-data. Trash old regs on idb version upgrade. Use principal for permission check. use principal in PushSubscription.
Attachment #8614332 -
Flags: review?(dougt)
Assignee | ||
Comment 3•9 years ago
|
||
Comment on attachment 8614332 [details] MozReview Request: Bug 1166350 - Push fixes for principals. r?dougt,bholley Bug 1166350 - Push fixes for principals. r?dougt Fix xpcshell tests. Add support for webapps-clear-data. Trash old regs on idb version upgrade. Use principal for permission check. use principal in PushSubscription.
Attachment #8614332 -
Flags: review?(amarchesini)
Assignee | ||
Comment 4•9 years ago
|
||
Andrea for the change to about:serviceWorkers, Doug for everything else.
Comment 5•9 years ago
|
||
https://reviewboard.mozilla.org/r/9907/#review8735 ::: dom/push/Push.js:148 (Diff revision 1) > + .getService(Ci.nsIAppsService); appsService is declared but not used. ::: dom/push/Push.js:224 (Diff revision 1) > - this._pageURL.spec); > + subscription._principal = this._principal; why don't you just pass the principal into the constructor? ::: dom/push/Push.js:237 (Diff revision 1) > + subscription._principal = this._principal; and here. ::: dom/push/PushService.jsm:100 (Diff revision 1) > - objectStore.createIndex("scope", "scope", { unique: true }); > + objectStore.createIndex("scope", "scope"); Why are we droping the uniqueness here? ::: dom/push/PushService.jsm:104 (Diff revision 1) > + objectStore.createIndex("originAttributes", "originAttributes"); maybe *this* should be unique? ::: dom/push/PushService.jsm:477 (Diff revision 1) > - let scope = appsService.getScopeByLocalId(data.appId); > + // FIXME(nsm): Refactor this into an originsuffix comparator in ChromeUtils.webidl. bug number? ::: dom/push/PushService.jsm:1525 (Diff revision 1) > + function makeRemoteBrowser() { This shoudl be factored out into a utility thing. Also should be reviewed by bz/jst/ehsan/etc
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Doug Turner (:dougt) from comment #5) > https://reviewboard.mozilla.org/r/9907/#review8735 > > ::: dom/push/Push.js:148 > (Diff revision 1) > > + .getService(Ci.nsIAppsService); > > appsService is declared but not used. I'll fix this. > > ::: dom/push/Push.js:224 > (Diff revision 1) > > - this._pageURL.spec); > > + subscription._principal = this._principal; > > why don't you just pass the principal into the constructor? > > ::: dom/push/Push.js:237 > (Diff revision 1) > > + subscription._principal = this._principal; > > and here. These two were already fixed in the updated patch on reviewboard. You may be looking at the older diff. > > ::: dom/push/PushService.jsm:100 > (Diff revision 1) > > - objectStore.createIndex("scope", "scope", { unique: true }); > > + objectStore.createIndex("scope", "scope"); > > Why are we droping the uniqueness here? For example, the facebook app and facebook website may both use SWs at the same scope, in which case only the appId disambiguates. The new unique identifier is { scope, originAttributes }. > > ::: dom/push/PushService.jsm:104 > (Diff revision 1) > > + objectStore.createIndex("originAttributes", "originAttributes"); > > maybe *this* should be unique? Unfortunately not, an origin can have multiple scopes each with a push ID. > > ::: dom/push/PushService.jsm:477 > (Diff revision 1) > > - let scope = appsService.getScopeByLocalId(data.appId); > > + // FIXME(nsm): Refactor this into an originsuffix comparator in ChromeUtils.webidl. > > bug number? > > ::: dom/push/PushService.jsm:1525 > (Diff revision 1) > > + function makeRemoteBrowser() { > > This shoudl be factored out into a utility thing. Also should be reviewed > by bz/jst/ehsan/etc Ah... actually this may not work in non-e10s and I'm going to fall back to the old broadcast method for now.
Comment 7•9 years ago
|
||
> Ah... actually this may not work in non-e10s and I'm going to fall back to the
old broadcast method for now.
Can you do both? broadcast really is a bad thing, right?
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Doug Turner (:dougt) from comment #7) > > Ah... actually this may not work in non-e10s and I'm going to fall back to the > old broadcast method for now. > > Can you do both? broadcast really is a bad thing, right? I'm going to fall back so we can land this patch. I'll followup.
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8614332 -
Attachment is obsolete: true
Attachment #8614332 -
Flags: review?(dougt)
Attachment #8614332 -
Flags: review?(amarchesini)
Attachment #8615723 -
Flags: review?(dougt)
Attachment #8615723 -
Flags: review?(amarchesini)
Reporter | ||
Comment 10•9 years ago
|
||
Comment on attachment 8615723 [details] [diff] [review] patch Review of attachment 8615723 [details] [diff] [review]: ----------------------------------------------------------------- I think it better to implement the ChromeUtils.webidl and use strings everywhere. Otherwise we land this code and then we have to change it all again. The code is ok from me, but up to you to propose another patch. ::: dom/interfaces/push/nsIPushNotificationService.idl @@ +29,5 @@ > * > * If the server drops a subscription, a `push-subscription-change` observer > * will be fired, with the subject set to `null` and the data set to |scope|. > * Servers may drop subscriptions at any time, so callers should recreate > * subscriptions if desired. Now we support CreateSuffix() and PopulateFromSuffix(). What about if you use strings here and in the rest of the code? ::: dom/push/Push.js @@ +216,5 @@ > switch (aMessage.name) { > case "PushService:Register:OK": > { > let subscription = new this._window.PushSubscription(json.pushEndpoint, > + this._scope, this._principal); indent it in a new line. @@ +228,5 @@ > { > let subscription = null; > try { > subscription = new this._window.PushSubscription(json.registration.pushEndpoint, > + this._scope, this._principal); same here. ::: dom/push/PushService.jsm @@ +98,5 @@ > // index to fetch records per scope, so we can identify endpoints > // associated with an app. > + objectStore.createIndex("scope", "scope"); > + > + // In the new security model, origins uniquely distinguish pages and apps I don't know if it will be the 'new' security model forever :)
Attachment #8615723 -
Flags: review?(amarchesini)
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #10) > Comment on attachment 8615723 [details] [diff] [review] > patch > > Review of attachment 8615723 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think it better to implement the ChromeUtils.webidl and use strings > everywhere. > Otherwise we land this code and then we have to change it all again. > The code is ok from me, but up to you to propose another patch. I don't understand what you are suggesting here. CreateSuffix() and PopulateSuffix() are not accessible to JS. I could directly access originSuffix on the principal and use that everywhere and pass that as a string to ServiceWorkerManager, where it could convert it to OriginAttributes in C++. I do agree that having the equality checking logic in PushService is not good and I am going to add a |bool ChromeUtils.originAttributesEqual(attrs1, attrs2)|. How does that sound?
Flags: needinfo?(amarchesini)
Reporter | ||
Comment 12•9 years ago
|
||
> ChromeUtils.originAttributesEqual(attrs1, attrs2)|. How does that sound?
Sounds good to me.
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8615723 [details] [diff] [review] patch This needed a fair amount of modification due to the split up and refactor of the PushService. I'll have a new patch up soon.
Attachment #8615723 -
Attachment is obsolete: true
Attachment #8615723 -
Flags: review?(dougt)
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8614332 [details] MozReview Request: Bug 1166350 - Push fixes for principals. r?dougt,bholley Bug 1166350 - Push fixes for principals. r?dougt,bholley Fix xpcshell tests. Add support for webapps-clear-data. Trash old regs on idb version upgrade. Use principal for permission check. use principal in PushSubscription.
Attachment #8614332 -
Attachment description: MozReview Request: Bug 1166350 - Push fixes for principals. r?dougt → MozReview Request: Bug 1166350 - Push fixes for principals. r?dougt,bholley
Attachment #8614332 -
Attachment is obsolete: false
Attachment #8614332 -
Flags: review?(dougt)
Attachment #8614332 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 15•9 years ago
|
||
Bobby for ChromeUtils.originAttributesToSuffix, Doug for everything else.
Comment 16•9 years ago
|
||
Comment on attachment 8614332 [details] MozReview Request: Bug 1166350 - Push fixes for principals. r?dougt,bholley https://reviewboard.mozilla.org/r/9909/#review10237 r=me on the ChromeUtils.\* stuff.
Attachment #8614332 -
Flags: review?(bobbyholley) → review+
Updated•9 years ago
|
Attachment #8614332 -
Flags: review?(dougt) → review?(kcambridge)
Comment 17•9 years ago
|
||
Comment on attachment 8614332 [details] MozReview Request: Bug 1166350 - Push fixes for principals. r?dougt,bholley https://reviewboard.mozilla.org/r/9909/#review10249 ::: dom/push/PushDB.jsm:174 (Diff revision 3) > + debug("FOUND " + JSON.stringify(cursor.value)); µ-nit: Leftover `debug` call? ::: dom/push/PushService.jsm:123 (Diff revision 3) > + _lookupOrPutPendingRequest: function(aPageRecord) { Nice refactoring! ::: dom/push/PushServiceHttp2.jsm:391 (Diff revision 3) > + if (aNewVersion != aOldVersion) { Let's maybe check if we're upgrading from 1 to 2? Or we can leave that for later, when we bump from 2 to 3. ::: dom/push/PushServiceHttp2.jsm:387 (Diff revision 3) > - debug("upgradeSchemaHttp2()"); > + debug("upgradeSchemaWS()"); Typo? ::: dom/interfaces/push/nsIPushNotificationService.idl:35 (Diff revision 3) > - jsval register(in string scope, [optional] in string pageURL); > + jsval register(in string scope, in jsval originAttributes); Making it a `jsval` instead of a `string` allows `undefined`, right? ::: dom/push/PushService.jsm:870 (Diff revision 3) > + if (!aMessage.target.assertContainApp(pageRecord.manifestURL)) { Doesn't look like `manifestURL` is sent anywhere in `Push.js`...or is that automatically added to the record? ::: dom/push/PushService.jsm:875 (Diff revision 3) > + var principal = aMessage.principal; µ-nit: s/var/let. ::: dom/push/PushService.jsm:268 (Diff revision 3) > + this._send("unregister", {channelID: records.channelID}); s/_send/_sendRequest, I think. Please add a `catch` with logging, too; IIRC, Web Push can return rejections. ::: dom/push/PushDB.jsm:150 (Diff revision 3) > + // Perform a unique match against { scope, originAttributes } If you indexed on `["originAttributes", "scope"]`, could you use `IDBKeyRange.only([aPageRecord.originAttributes, aPageRecord.scope])` here? ::: dom/push/PushDB.jsm:216 (Diff revision 3) > + return this._getAllByKey("originAttributes", aOriginAttributes); ...And `IDBKeyRange.bound([aOriginAttributes, " "], [aOriginAttributes, "~"])` here? LGTM! Just a few minor nits, a question about the IDB schema, and sending the `manifestURL`.
Attachment #8614332 -
Flags: review?(kcambridge)
Assignee | ||
Comment 18•9 years ago
|
||
https://reviewboard.mozilla.org/r/9909/#review10285 > Making it a `jsval` instead of a `string` allows `undefined`, right? This is checked in PushService. > If you indexed on `["originAttributes", "scope"]`, could you use `IDBKeyRange.only([aPageRecord.originAttributes, aPageRecord.scope])` here? Thanks! I had no idea IDB allowed indices over multiple values. > Doesn't look like `manifestURL` is sent anywhere in `Push.js`...or is that automatically added to the record? I miss static typing :/ > Let's maybe check if we're upgrading from 1 to 2? Or we can leave that for later, when we bump from 2 to 3. We are always junking the old database right now since we don't ship this, so this check seems acceptable. > ...And `IDBKeyRange.bound([aOriginAttributes, " "], [aOriginAttributes, "~"])` here? I don't know what you mean by " " and "~". Instead I've created another index on originAttributes for the webapps-clear-data case. OK?
Assignee | ||
Comment 19•9 years ago
|
||
Comment on attachment 8614332 [details] MozReview Request: Bug 1166350 - Push fixes for principals. r?dougt,bholley Bug 1166350 - Push fixes for principals. r?dougt,bholley Fix xpcshell tests. Add support for webapps-clear-data. Trash old regs on idb version upgrade. Use principal for permission check. use principal in PushSubscription.
Attachment #8614332 -
Flags: review+ → review?(kcambridge)
Comment 20•9 years ago
|
||
https://reviewboard.mozilla.org/r/9909/#review10295 > I don't know what you mean by " " and "~". Instead I've created another index on originAttributes for the webapps-clear-data case. OK? Oh, sorry. `" "` and `"~"` uses IDB's lexicographic sorting; `"\x00"` and `"\xff"` would have the same effect. If the index is on `["originAttributes", "scope"]`, `IDBKeyRange.bound([aOriginAttributes, " "], [aOriginAttributes, "~"])` would get all records with those attributes, and any scope. You could then drop the second index on `originAttributes`, and the `mozGetAll()` call. Your approach works equally well, though.
Comment 21•9 years ago
|
||
Comment on attachment 8614332 [details] MozReview Request: Bug 1166350 - Push fixes for principals. r?dougt,bholley https://reviewboard.mozilla.org/r/9909/#review10297 Ship It!
Attachment #8614332 -
Flags: review?(kcambridge) → review+
Assignee | ||
Comment 22•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c79076bf05a
Assignee | ||
Comment 23•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/98c7277a8fb5
Comment 24•9 years ago
|
||
Backed out for xpcshell failures. https://treeherder.mozilla.org/logviewer.html#?job_id=11062977&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ab4a5c0acf
Assignee | ||
Comment 25•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/964de5d960fa
Assignee | ||
Comment 26•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2547747a19 https://hg.mozilla.org/integration/mozilla-inbound/rev/662d5b37ad5e
Assignee | ||
Comment 27•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=689aec56e9d8
Comment 28•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dc2547747a19 https://hg.mozilla.org/mozilla-central/rev/662d5b37ad5e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•