1300112 - push notifications do not show correctly on windows with multiple e10s child processes enabled

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

I recently enabled multiple e10s child processes by setting:

  dom.ipc.processCount

I'm using a very high value so I get a new process per tab.

In this configuration I noticed that twitter push notifications will briefly flash on the screen, but do not remain for the normal time duration.

You can see it with this demo as well:

  https://gauntface.github.io/simple-push-demo/

Note, with the demo I got a single flashed notification, but then don't see anything after the first push.  I think its removing them so fast they don't actually paint.

Its pretty clear the service worker is running in this case.  Its not obvious why the notifications are being removed so quickly, though.

This is on windows 10.  I would not be surprised if it worked differently on other platforms.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Actually, the behavior is a bit worse.

I just got one of these flashing twitter notifications in window 1 while I was focused on window 2.  I tried to click on my twitter tab in window 1 and it acted as though I clicked on the notification.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Note that it's expected from bug 1297594 that all N (alive) content processes are going to have a serviceworker spun up and attempt to service the push effectively in parallel.  Because of the serialization of the DOM Cache and other APIs, it's quite possible that some of the ServiceWorkers may partially perceive the actions of the others which could add complexity beyond the notifications API receiving duplicate requests in a small time window.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

That could very well be it.  I just got an irccloud notification and it worked fine.  I don't think irccloud uses service workers, though.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Sounds like this is something we may revisit after bug 1297594, so carrying over P2 from that bug. Sounds right?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Sounds reasonable.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

So, I noticed I get notifications on my nightly on my mac which also has multi-e10s enabled.  The difference between windows and mac is that windows has the GPU process.  So I suspect this is due to the GPU process.

Comment 7

[Cătălin Badea (:catalinb)]

Attachment: Avoid duplicate push notifications by sending push messages to only one content process.

Comment 8

[Cătălin Badea (:catalinb)]

I looked into whether we can be smarter about it and send the push events to a content process that has windows with the same origin, but that's that not possible without sending an IPC message to the child to query the list of open windows.

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8844917 [details] [diff] [review]
Avoid duplicate push notifications by sending push messages to only one content process.

Review of attachment 8844917 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM; just some questions for my own knowledge:

* I assume the first active `ContentParent` will always have the service worker?
* Will moving `ServiceWorkerManager` to the parent eventually help with this?

Comment 10

[Cătălin Badea (:catalinb)]

(In reply to Kit Cambridge [:kitcambridge] from comment #9)
> Comment on attachment 8844917 [details] [diff] [review]
> Avoid duplicate push notifications by sending push messages to only one
> content process.
> 
> Review of attachment 8844917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM; just some questions for my own knowledge:
> 
> * I assume the first active `ContentParent` will always have the service
> worker?
With a few exceptions, yes. We might get into some cases where a service worker was registered in a different content process and the registration was not propagated to other content processes.

> * Will moving `ServiceWorkerManager` to the parent eventually help with this?
Yes.

This is a hack to mitigate the effects of shipping multi-process e10s.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6707f63c9c8d
Avoid duplicate push notifications by sending push messages to only one content process. r=kitcambridge

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6707f63c9c8d

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

I can verify this seems to fix my problem in comment 0.  Catalin, can you get this uplifted FF54?

Comment 14

[Cătălin Badea (:catalinb)]

Comment on attachment 8844917 [details] [diff] [review]
Avoid duplicate push notifications by sending push messages to only one content process.

Approval Request Comment
[Feature/Bug causing the regression]: Service Workers may display double notifications with multi-e10s
[User impact if declined]: Websites using push notifications, might display them twice.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: none.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It affects only a small component of our SW implementation.
[String changes made/needed]: no.

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8844917 [details] [diff] [review]
Avoid duplicate push notifications by sending push messages to only one content process.

Fix duplicated push notifications with multi-e10s and was verified locally by dev. Aurora54+.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/98b5cf38def1

Comment 18

[Andrei Vaida [:avaida]]

(In reply to Cătălin Badea (:catalinb) from comment #14)
> [Is this code covered by automated tests?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Cătălin's assessment on manual testing needs and the fact that this fix has automated coverage.