Closed Bug 1300112 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
e10s + ---
firefox-esr45 --- disabled
firefox52 --- wontfix
firefox-esr52 --- disabled
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: bkelly, Assigned: catalinb)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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.
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.
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.
tracking-e10s: --- → ?
Sounds like this is something we may revisit after bug 1297594, so carrying over P2 from that bug. Sounds right?
Flags: needinfo?(bkelly)
Priority: -- → P2
Sounds reasonable.
Flags: needinfo?(bkelly)
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.
Assignee: nobody → catalin.badea392
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 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?
Attachment #8844917 - Flags: review?(kit) → review+
See Also: → 1297594
(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.
Pushed by catalin.badea392@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6707f63c9c8d
Avoid duplicate push notifications by sending push messages to only one content process. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/6707f63c9c8d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
I can verify this seems to fix my problem in comment 0.  Catalin, can you get this uplifted FF54?
Flags: needinfo?(catalin.badea392)
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.
Flags: needinfo?(catalin.badea392)
Attachment #8844917 - Flags: approval-mozilla-aurora?
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+.
Attachment #8844917 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1297594
(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.
Flags: qe-verify-
Duplicate of this bug: 1199036
You need to log in before you can comment on or make changes to this bug.