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

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch: Fire events after iterating ServiceWorkerManager::mControlledDocuments is done.

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!

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Treeherder result looks fine, btw:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7a8236b60d03bf66ec3052ecffb1446d268e4e3&group_state=expanded

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15f1ca2cd693
Fire events after iterating ServiceWorkerManager::mControlledDocuments is done. r=bkelly

Comment 5

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/15f1ca2cd693