Closed Bug 1189998 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
e10s + ---
firefox46 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(4 files)

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.
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?
Flags: needinfo?(nsm.nikhil)
Sounds reasonable. Can we get rid of most of nsIPushNotificationService, except the iniatilization parts that start PushService?
Flags: needinfo?(nsm.nikhil)
Blocks: 1131813
Status: NEW → ASSIGNED
Blocks: 1188981
Bug 1189998, Part 1 - Consolidate Push client interfaces. r?mt,dragana
Attachment #8696763 - Flags: review?(martin.thomson)
Attachment #8696763 - Flags: review?(dd.mozilla)
Bug 1189998, Part 2 - Migrate Push service callers. r?mt
Attachment #8696764 - Flags: review?(martin.thomson)
Bug 1189998, Part 3 - Update consolidated Push tests. r?mt
Attachment #8696765 - Flags: review?(martin.thomson)
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.
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 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.
Attachment #8696763 - Flags: review?(martin.thomson) → review+
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.
Attachment #8696764 - Flags: review?(martin.thomson) → review+
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.
Attachment #8696765 - Flags: review?(martin.thomson) → review+
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/
Attachment #8696763 - Attachment description: MozReview Request: Bug 1189998, Part 1 - Consolidate Push client interfaces. r?mt,dragana → MozReview Request: Bug 1189998, Part 1 - Consolidate Push client interfaces. r=mt?dragana
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/
Attachment #8696764 - Attachment description: MozReview Request: Bug 1189998, Part 2 - Migrate Push service callers. r?mt → MozReview Request: Bug 1189998, Part 2 - Migrate Push service callers. r=mt
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/
Attachment #8696765 - Attachment description: MozReview Request: Bug 1189998, Part 3 - Update consolidated Push tests. r?mt → MozReview Request: Bug 1189998, Part 3 - Update consolidated Push tests. r=mt
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 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 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 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.
Attachment #8696763 - Flags: review?(dd.mozilla) → review+
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/
Attachment #8696763 - Attachment description: MozReview Request: Bug 1189998, Part 1 - Consolidate Push client interfaces. r=mt?dragana → MozReview Request: Bug 1189998, Part 1 - Consolidate Push client interfaces. r=mt,dragana
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 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/
Bug 1189998, Part 4 - Add authentication secret to Push data test. r?dragana
Attachment #8697599 - Flags: review?(dd.mozilla)
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 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
Attachment #8697599 - Flags: review?(dd.mozilla) → review+
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 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 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 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/
Attachment #8697599 - Attachment description: MozReview Request: Bug 1189998, Part 4 - Add authentication secret to Push data test. r?dragana → MozReview Request: Bug 1189998, Part 4 - Add authentication secret to Push data test. r=dragana
Blocks: 1233433
Blocks: 1233509
Depends on: 1235050
You need to log in before you can comment on or make changes to this bug.