Closed Bug 1166350 Opened 5 years ago Closed 5 years ago

Push notification should use the correct principal when calling ServiceWorkerManager

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

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.
I have a mostly working patch.
Assignee: nobody → nsm.nikhil
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)
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)
Andrea for the change to about:serviceWorkers, Doug for everything else.
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
(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.
> 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?
(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.
Attached patch patch (obsolete) — Splinter Review
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)
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)
(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)
> ChromeUtils.originAttributesEqual(attrs1, attrs2)|. How does that sound?

Sounds good to me.
Flags: needinfo?(amarchesini)
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)
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)
Bobby for ChromeUtils.originAttributesToSuffix, Doug for everything else.
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+
Attachment #8614332 - Flags: review?(dougt) → review?(kcambridge)
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)
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?
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)
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 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+
https://hg.mozilla.org/mozilla-central/rev/dc2547747a19
https://hg.mozilla.org/mozilla-central/rev/662d5b37ad5e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.