1238990 - service worker should not propagate updates across processes

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently when we trigger an update we "propagate" it through the parent process.  This sends the update trigger to all child processes.  This is bad for a number of reasons:

1) Multiple children can run the same update at the same time.  For example, this could result in simultaneous, overlapping install events.
2) This can result in an actor in the parent process running content script.  This is bad for security.

Instead we should remove the update propagation mechanism completely.  Instead, the update should just run in the current process.  This will work because:

a) All the updates trigger in the child, so all updates will run in a child.
b) Any service worker script changes will result in a StoreRegistration() call that is propagated to other processes.

There will still be the possibility for independent child processes to perform the same update, but they would both have to get individual triggers at the same time.  This is much less likely (although still possible given our 24 hour gating, etc.)

In the long run our e10s refactor will fix this whole mess.  I want to remove the current propagation mechanism now, though, because I think it creates a lot more race conditions and is generally incorrect at the moment.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Hmm, I guess we still need some way for about:serviceworkers to trigger an update.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 ServiceWorkerManager should trigger automatic updates in current process. r=ehsan

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Release assert that updates are only propagated from the parent process. r=ehsan

I can't remove all the PropagateSoftUpdate() code, but we can assert that its only ever called from the parent process where the about:serviceworkers tab runs.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Try to ensure service worker jobs do not run during shutdown. r=ehsan

Triggering the updates locally by default actually causes us to hit the shutdown leak from bug 1232558.  The P1 patch from that bug seems to fix the problem, though.

Here is a partial try to see if this solves the intermittent in bug 1232136:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88adbb91662e

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8707029 [details] [diff] [review]
P1 ServiceWorkerManager should trigger automatic updates in current process. r=ehsan

20+ green triggers on M-e10s(5).  It seems to fix that intermittent.  I'm hopeful this will fix many of our other e10s intermittents as well.

A wider try build:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=47cf9f4ba255

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8707030 [details] [diff] [review]
P2 Release assert that updates are only propagated from the parent process. r=ehsan

It seems about:serviceworkers runs in the child, not the parent.  Let me try something else here.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Try to ensure service worker jobs do not run during shutdown. r=ehsan

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

I'm going to split the work to fix up about:serviceworkers into a separate bug.  It will still function as it previously did until then.

New try with just these two patches:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96492ade8eb6

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f6fa255e10d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a9ab8439294

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f6fa255e10d
https://hg.mozilla.org/mozilla-central/rev/0a9ab8439294