1189998 - [e10s] `nsIPushNotificationService` can't be used from the content process

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

To reproduce:

1. Enable e10s.
2. Register a service worker with a push subscription.
3. Open `about:serviceworkers`.

The "Push Endpoint" field will say "Waiting...", and an error will be printed to the console. I think it's trying to initialize the push service in the content process, when it should send a message to the initialized service in the parent.

Comment 1

[Lina Butler [:lina]]

So `nsIPushClient` already has IPC logic, and works regardless of whether the caller runs in the parent or child. I vote we just replace `nsIPushNotificationService` with `nsIPushClient`. What do you think, Nikhil?

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Sounds reasonable. Can we get rid of most of nsIPushNotificationService, except the iniatilization parts that start PushService?

Comment 3

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1189998, Part 1 - Consolidate Push client interfaces. r=mt,dragana

Bug 1189998, Part 1 - Consolidate Push client interfaces. r?mt,dragana

Comment 4

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1189998, Part 2 - Migrate Push service callers. r=mt

Bug 1189998, Part 2 - Migrate Push service callers. r?mt

Comment 5

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1189998, Part 3 - Update consolidated Push tests. r=mt

Bug 1189998, Part 3 - Update consolidated Push tests. r?mt

Comment 6

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba08dc616f81

Comment 7

[Lina Butler [:lina]]

This patch is a start at cleaning up the Push module. The first part merges `nsIPushClient` and `nsIPushNotificationService` into an `nsIPushService` component that works in both processes. It also moves the IPC logic out of `PushService.jsm`. The new `nsIPushSubscription` interface isn't really necessary for the DOM API, but useful for chrome callers like FxA and Hello.

Martin, Dragana: I'm sorry in advance for the size and scope of this patch. I owe you dinner and/or drinks at Mozlando.

Comment 8

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/27431/#review24737

::: dom/push/Push.js:207
(Diff revision 1)
> +    let keyLen = outKeyLen.data;

Should be `outKeyLen.value`.

::: dom/push/PushService.jsm:519
(Diff revision 1)
> +    /*

Oops, this should be removed.

::: dom/push/PushService.jsm:1108
(Diff revision 1)
> -  _validatePageRecord: function(aMessage) {
> +  _validatePageRecord: function(pageRecord) {

I think we can remove this. `_toPageRecord` already takes care of validating the scope and origin attributes.

Comment 9

[Martin Thomson [:mt:]]

Comment on attachment 8696763 [details]
MozReview Request: Bug 1189998, Part 1 - Consolidate Push client interfaces. r=mt,dragana

https://reviewboard.mozilla.org/r/27431/#review24735

That's a pretty big impovement.  r+ conditional on the other parts being OK, of course.

::: dom/interfaces/push/nsIPushService.idl:25
(Diff revision 1)
> +  void getP256dhPublicKey([optional] out uint32_t keyLen,
> +                          [array, size_is(keyLen), retval] out octet key);

You will need to add one for the authenticationSecret, unless you want to parameterize this, which I recommend.

::: dom/push/Push.js:205
(Diff revision 1)
> +    let outKeyLen = {};
> +    let key = subscription.getP256dhPublicKey(outKeyLen);
> +    let keyLen = outKeyLen.data;

That is horrific, I guess that's the only way though :(

::: dom/push/Push.manifest:6
(Diff revision 1)
> -component {32028e38-903b-4a64-a180-5857eb4cb3dd} PushNotificationService.js
> -contract @mozilla.org/push/NotificationService;1 {32028e38-903b-4a64-a180-5857eb4cb3dd}
> -category app-startup PushNotificationService @mozilla.org/push/NotificationService;1
> +component {daaa8d73-677e-4233-8acd-2c404bd01658} PushComponents.js
> +contract @mozilla.org/push/Service;1 {daaa8d73-677e-4233-8acd-2c404bd01658}
> +category app-startup nsIPushService @mozilla.org/push/Service;1
>  
> -component {66a87970-6dc9-46e0-ac61-adb4a13791de} PushNotificationService.js
> +component {66a87970-6dc9-46e0-ac61-adb4a13791de} PushComponents.js

Generate new UUIDs for this rather than using an old one.

::: dom/push/PushService.jsm:519
(Diff revision 1)
> +    /*
>      if (event != CHANGING_SERVICE_EVENT) {
>        // if only serverURL is changed we can keep listening for broadcast
>        // messages and queue them.
>        let ppmm = Cc["@mozilla.org/parentprocessmessagemanager;1"]
>                     .getService(Ci.nsIMessageBroadcaster);
>  
>        kCHILD_PROCESS_MESSAGES.forEach(msgName =>
>          ppmm.addMessageListener(msgName, this)
>        );
>      }
> +    */

Don't comment code, delete it.

::: dom/push/PushService.jsm:572
(Diff revision 1)
> +    */

As above.

::: dom/push/PushService.jsm:1091
(Diff revision 1)
> -  receiveMessage: function(aMessage) {
> +  registerListener: function(listener) {

Nit: If you are changing these so much, change the declaration to the short form: registerListener(listener) {

::: dom/push/PushService.jsm:1097
(Diff revision 1)
> -      console.debug("receiveMessage: Possibly removing child listener");
> +    console.debug("unregisterListener: Possibly removing child listener");

Do you need so many debug statements?  Probably just remove all but the first.

Also:
    this._childListeners = this._childListeners.filter(x => x !== listener);

::: dom/push/PushService.jsm:1113
(Diff revision 1)
> -    pageRecord.originAttributes =
> +    if (typeof pageRecord.originAttributes != "string") {

I've a fairly strong preference for === and !== over == and !=.

::: dom/push/PushService.jsm:1123
(Diff revision 1)
>      return this._checkActivated()
> -      .then(_ => this._db.getByIdentifiers(aPageRecord))
> +      .then(_ => this._validatePageRecord(aPageRecord))
> +      .then(pageRecord => this._db.getByIdentifiers(pageRecord))

I'm seeing three methods invoked at the same time here, and below.  Might be worth wrapping this.

::: dom/push/PushService.jsm:1124
(Diff revision 1)
> -      .then(_ => this._db.getByIdentifiers(aPageRecord))
> +      .then(_ => this._validatePageRecord(aPageRecord))

Since this is a synchronous call, can you make it synchronous.  I know that makes this a little uglier, but unless something is actually async, I prefer to avoid dispatches.

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8696764 [details]
MozReview Request: Bug 1189998, Part 2 - Migrate Push service callers. r=mt

https://reviewboard.mozilla.org/r/27433/#review24739

::: dom/interfaces/push/nsIPushService.idl:109
(Diff revision 1)
> +  void clearAll(in nsIPrincipal principal,
> +                in nsIPushClearResultCallback callback);
> +
> +  void clearForDomain(in DOMString domain,
> +                      in nsIPushClearResultCallback callback);

Some comments on these would be nice.

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8696765 [details]
MozReview Request: Bug 1189998, Part 3 - Update consolidated Push tests. r=mt

https://reviewboard.mozilla.org/r/27435/#review24741

::: dom/push/test/xpcshell/test_register_case.js:42
(Diff revision 1)
> -            channelID: request.channelID,
> +            channelID: channelID,

This seems like a gratuitous change.

::: dom/push/test/xpcshell/test_service_parent.js:53
(Diff revision 1)
> +  PushService.init({

This whole block is copy-pasted from the previous.  Please extract.

Comment 12

[Lina Butler [:lina]]

Comment on attachment 8696763 [details]
MozReview Request: Bug 1189998, Part 1 - Consolidate Push client interfaces. r=mt,dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27431/diff/1-2/

Comment 13

[Lina Butler [:lina]]

Comment on attachment 8696764 [details]
MozReview Request: Bug 1189998, Part 2 - Migrate Push service callers. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27433/diff/1-2/

Comment 14

[Lina Butler [:lina]]

Comment on attachment 8696765 [details]
MozReview Request: Bug 1189998, Part 3 - Update consolidated Push tests. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27435/diff/1-2/

Comment 15

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd63c6154205

Comment 16

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6cf1774809c

Comment 17

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=869dad4eec85

Comment 18

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=afb1907dbeab

Comment 19

[Lina Butler [:lina]]

Comment on attachment 8696763 [details]
MozReview Request: Bug 1189998, Part 1 - Consolidate Push client interfaces. r=mt,dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27431/diff/2-3/

Comment 20

[Lina Butler [:lina]]

Comment on attachment 8696764 [details]
MozReview Request: Bug 1189998, Part 2 - Migrate Push service callers. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27433/diff/2-3/

Comment 21

[Lina Butler [:lina]]

Comment on attachment 8696765 [details]
MozReview Request: Bug 1189998, Part 3 - Update consolidated Push tests. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27435/diff/2-3/

Comment 22

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00ddfcff73b8

Comment 23

[Dragana Damjanovic [:dragana]]

Comment on attachment 8696763 [details]
MozReview Request: Bug 1189998, Part 1 - Consolidate Push client interfaces. r=mt,dragana

https://reviewboard.mozilla.org/r/27431/#review24841

with fixing comments below.  Do we have a test for the comments below, (the parts that seems not to be right)? We need tests to catch the error with "auth"?

::: dom/push/Push.js:207
(Diff revision 2)
> -    if (keyLen) {
> +    let authSecret = this._getKey(subscription, "secret");

should this be "auth" instead og secret?

::: dom/push/Push.js:226
(Diff revision 2)
> +  },

"name" is not used , and this does not look right.

::: dom/push/PushService.jsm:518
(Diff revision 2)
>  

you can remove event parameter from startService all together.

Comment 24

[Lina Butler [:lina]]

Comment on attachment 8696763 [details]
MozReview Request: Bug 1189998, Part 1 - Consolidate Push client interfaces. r=mt,dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27431/diff/3-4/

Comment 25

[Lina Butler [:lina]]

Comment on attachment 8696764 [details]
MozReview Request: Bug 1189998, Part 2 - Migrate Push service callers. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27433/diff/3-4/

Comment 26

[Lina Butler [:lina]]

Comment on attachment 8696765 [details]
MozReview Request: Bug 1189998, Part 3 - Update consolidated Push tests. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27435/diff/3-4/

Comment 27

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1189998, Part 4 - Add authentication secret to Push data test. r=dragana

Bug 1189998, Part 4 - Add authentication secret to Push data test. r?dragana

Comment 28

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/27431/#review24841

> "name" is not used , and this does not look right.

Yes, this was totally wrong. Thanks for catching that! I updated and uncommented the test in part 4.

Comment 29

[Dragana Damjanovic [:dragana]]

Comment on attachment 8697599 [details]
MozReview Request: Bug 1189998, Part 4 - Add authentication secret to Push data test. r=dragana

https://reviewboard.mozilla.org/r/27657/#review24919

lgtm

Comment 30

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb024874e231243244f641a0d0ab579a0d31a8b5
Bug 1189998, Part 1 - Consolidate Push client interfaces. r=mt,dragana

https://hg.mozilla.org/integration/mozilla-inbound/rev/d52e69a8f8d2ea88269ef645b755363b48e6e6c5
Bug 1189998, Part 2 - Migrate Push service callers. r=mt

https://hg.mozilla.org/integration/mozilla-inbound/rev/86064f1cf15d143c05e270bde4c3bff6f36a5de7
Bug 1189998, Part 3 - Update consolidated Push tests. r=mt

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8846bf9c3f47a999d04c7c150c99070c7ead2be
Bug 1189998, Part 4 - Add authentication secret to Push data test. r=dragana

Comment 31

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ef251e4e0599 - 100% ASan mochitest-push leak, https://treeherder.mozilla.org/logviewer.html#?job_id=18515657&repo=mozilla-inbound, and fairly frequent 10.6 xpcshell https://treeherder.mozilla.org/logviewer.html#?job_id=18515504&repo=mozilla-inbound

Comment 32

[Lina Butler [:lina]]

Comment on attachment 8696763 [details]
MozReview Request: Bug 1189998, Part 1 - Consolidate Push client interfaces. r=mt,dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27431/diff/4-5/

Comment 33

[Lina Butler [:lina]]

Comment on attachment 8696764 [details]
MozReview Request: Bug 1189998, Part 2 - Migrate Push service callers. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27433/diff/4-5/

Comment 34

[Lina Butler [:lina]]

Comment on attachment 8696765 [details]
MozReview Request: Bug 1189998, Part 3 - Update consolidated Push tests. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27435/diff/4-5/

Comment 35

[Lina Butler [:lina]]

Comment on attachment 8697599 [details]
MozReview Request: Bug 1189998, Part 4 - Add authentication secret to Push data test. r=dragana

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27657/diff/1-2/

Comment 36

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f821dec83eed0bb6a017d02059603cda90efb9bd
Bug 1189998, Part 1 - Consolidate Push client interfaces. r=mt,dragana

https://hg.mozilla.org/integration/mozilla-inbound/rev/9aaa73b4c48744df025d8a397bafb8560a2f570e
Bug 1189998, Part 2 - Migrate Push service callers. r=mt

https://hg.mozilla.org/integration/mozilla-inbound/rev/61cacf08b5594fa94f330e71a412e3fb04a72286
Bug 1189998, Part 3 - Update consolidated Push tests. r=mt

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c123f941024717f4f8cdff17d7b7b767ccfd393
Bug 1189998, Part 4 - Add authentication secret to Push data test. r=dragana

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f821dec83eed
https://hg.mozilla.org/mozilla-central/rev/9aaa73b4c487
https://hg.mozilla.org/mozilla-central/rev/61cacf08b559
https://hg.mozilla.org/mozilla-central/rev/7c123f941024