Closed Bug 1238990 Opened 4 years ago Closed 4 years ago

service worker should not propagate updates across processes

Categories

(Core :: DOM: Service Workers, defect)

32 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
Hmm, I guess we still need some way for about:serviceworkers to trigger an update.
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.
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 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
Attachment #8707029 - Flags: review?(ehsan)
Attachment #8707030 - Flags: review?(ehsan)
Attachment #8707032 - Flags: review?(ehsan)
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.
Attachment #8707030 - Flags: review?(ehsan)
Attachment #8707029 - Flags: review?(ehsan) → review+
Attachment #8707032 - Flags: review?(ehsan) → review+
Attachment #8707030 - Attachment is obsolete: true
Attachment #8707032 - Attachment is obsolete: true
Attachment #8707138 - Flags: review+
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
Blocks: 1239113
Blocks: 1238954
Blocks: 1232136
https://hg.mozilla.org/mozilla-central/rev/8f6fa255e10d
https://hg.mozilla.org/mozilla-central/rev/0a9ab8439294
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
No longer blocks: 1232136
Duplicate of this bug: 1232136
You need to log in before you can comment on or make changes to this bug.