Closed
Bug 1300112
Opened 8 years ago
Closed 8 years ago
push notifications do not show correctly on windows with multiple e10s child processes enabled
Categories
(Core :: DOM: Notifications, defect, P2)
Core
DOM: Notifications
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: bkelly, Assigned: catalinb)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
lina
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•8 years ago
|
||
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•8 years ago
|
||
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.
Reporter | ||
Comment 3•8 years ago
|
||
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:
--- → ?
Updated•8 years ago
|
Comment 4•8 years ago
|
||
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
Reporter | ||
Comment 6•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → catalin.badea392
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8844917 -
Flags: review?(kit)
Assignee | ||
Comment 8•8 years ago
|
||
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•8 years ago
|
||
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+
Updated•8 years ago
|
Assignee | ||
Comment 10•8 years ago
|
||
(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•8 years ago
|
||
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
Comment 12•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Reporter | ||
Updated•8 years ago
|
Blocks: ServiceWorkers-bandaids
Reporter | ||
Comment 13•8 years ago
|
||
I can verify this seems to fix my problem in comment 0. Catalin, can you get this uplifted FF54?
status-firefox52:
--- → wontfix
status-firefox53:
--- → wontfix
status-firefox54:
--- → affected
status-firefox-esr45:
--- → disabled
status-firefox-esr52:
--- → disabled
Flags: needinfo?(catalin.badea392)
Assignee | ||
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
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+
Comment 16•8 years ago
|
||
bugherder uplift |
Comment 18•8 years ago
|
||
(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-
You need to log in
before you can comment on or make changes to this bug.
Description
•