Closed
Bug 1190661
Opened 9 years ago
Closed 9 years ago
Send push only to child processes on e10s
Categories
(Core :: DOM: Push Subscriptions, defect)
Core
DOM: Push Subscriptions
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: nsm, Assigned: nsm)
References
Details
Attachments
(1 file, 1 obsolete file)
8.07 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
See commit message in incoming attachment for description.
Assignee | ||
Comment 1•9 years ago
|
||
Bug 1190661 - Send push only to child processes when in e10s mode. r?smaug Earlier, the in-process child process message manager and any content process child process message managers received the push event. This is because broadcastAsyncMessage is used, but on e10s we want ServiceWorkers to run in the child process, so the push should only be dispatched to it. This patch introduces a list of child process listeners in the PushService (running on the parent). PushServiceChildPreload sends a message to the PushService iff it is running in a child process. If there are non-zero child listeners, the PushService will send a message to all of them, otherwise it will fall back to broadcastAsyncMessage. We currently do not add support for precise targeting of child processes. This is because until Bug 1182117 is fixed, all child process ServiceWorkerManagers maintain all the service worker registrations internally. Yes this is a bug, but that is the way things are right now. This makes it impossible to distinguish which child should handle the notification for a given origin. Considering we don't ship multi-process e10s, I would like to land this right now. When Bug 1182117 is fixed, we can remove this code since the ServiceWorkerManager will manage registrations in the parent, and it will know which child process is running which ServiceWorker.
Attachment #8642781 -
Flags: review?(bugs)
Updated•9 years ago
|
tracking-e10s:
--- → +
Comment 2•9 years ago
|
||
You leak stuff in _childListeners. Looks like we've added "child-process-shutdown" (CHILD_PROCESS_SHUTDOWN_MESSAGE) to notify process message manager parents of the child process shutting down. So you want to listen for that and remove the relevant mm from _childListeners. Other option would be to observe ipc:content-shutdown notification, but that seems to be harder to use in this case.
Updated•9 years ago
|
Attachment #8642781 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 3•9 years ago
|
||
removes child listeners.
Assignee: nobody → nsm.nikhil
Attachment #8642781 -
Attachment is obsolete: true
Attachment #8643286 -
Flags: review?(bugs)
Updated•9 years ago
|
Attachment #8643286 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 4•9 years ago
|
||
url: https://hg.mozilla.org/integration/mozilla-inbound/rev/b8a39e65c778d7c6a6c213e46e5655207f51bded changeset: b8a39e65c778d7c6a6c213e46e5655207f51bded user: Nikhil Marathe <nsm.nikhil@gmail.com> date: Thu Jul 30 20:00:21 2015 -0700 description: Bug 1190661 - Send push only to child processes when in e10s mode. r=smaug Earlier, the in-process child process message manager and any content process child process message managers received the push event. This is because broadcastAsyncMessage is used, but on e10s we want ServiceWorkers to run in the child process, so the push should only be dispatched to it. This patch introduces a list of child process listeners in the PushService (running on the parent). PushServiceChildPreload sends a message to the PushService iff it is running in a child process. If there are non-zero child listeners, the PushService will send a message to all of them, otherwise it will fall back to broadcastAsyncMessage. We currently do not add support for precise targeting of child processes. This is because until Bug 1182117 is fixed, all child process ServiceWorkerManagers maintain all the service worker registrations internally. Yes this is a bug, but that is the way things are right now. This makes it impossible to distinguish which child should handle the notification for a given origin. Considering we don't ship multi-process e10s, I would like to land this right now. When Bug 1182117 is fixed, we can remove this code since the ServiceWorkerManager will manage registrations in the parent, and it will know which child process is running which ServiceWorker.
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b8a39e65c778
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•