Separate FireControllerChangeOnDocument() from the iteration of ServiceWorkerManager::mControlledDocuments in ServiceWorkerManager::FireControllerChange()

RESOLVED FIXED in Firefox 58

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: bevis, Assigned: bevis)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

It's possible to remove the document from oncontrollerchange handler while iterating ServiceWorkerManager::mControlledDocuments which hits the assertion in debug build at:
http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/xpcom/ds/PLDHashTable.h#132

This is 100% reproducible at 
http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/testing/web-platform/tests/service-workers/service-worker/skip-waiting-using-registration.https.html#41
if the microtask-checkpoint at the end of JS callback is introduced by bug 1193394.
This is to fix the assertion failure explained in comment 0.

The call stack of the assertion failure is here for your reference:
https://treeherder.mozilla.org/logviewer.html#?job_id=141242133&repo=try&lineNumber=34346

May I have your review for this change?

Thanks!
Attachment #8924113 - Flags: review?(bugmail)
Comment on attachment 8924113 [details] [diff] [review]
(v1) Patch: Fire events after iterating ServiceWorkerManager::mControlledDocuments is done.

Review of attachment 8924113 [details] [diff] [review]:
-----------------------------------------------------------------

I was looking at this to see how it might conflict with my other patches.  Going to just steal the review.  Looks good to me.

::: dom/workers/ServiceWorkerManager.cpp
@@ +3363,5 @@
>  ServiceWorkerManager::FireControllerChange(ServiceWorkerRegistrationInfo* aRegistration)
>  {
>    AssertIsOnMainThread();
> +
> +  nsTArray<nsCOMPtr<nsIDocument>> documents;

AutoTArray please.  Probably 8 to 16 is more than enough for the typical case.
Attachment #8924113 - Flags: review?(bugmail) → review+
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15f1ca2cd693
Fire events after iterating ServiceWorkerManager::mControlledDocuments is done. r=bkelly
https://hg.mozilla.org/mozilla-central/rev/15f1ca2cd693
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.