1174381 - ServiceWorkerManager shutdown violates XPCOM threading rules and leaks

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Since the recent landing of bug 1155153 (I think), the ServiceWorkerManager code is using DispatchToMainThread() from nsComponentManagerImpl::gComponentManager->FreeServices() in XPCOM shutdown - this happens after xpcom-shutdown-threads, nsThreadManager shutdown, etc.  At this point DispatchToMainThread() will always fail and leak, which is what I've been seeing with some dispatch changes I've been working on.

This is trying to send a Shutdown message via mActor (i.e. PBackground IPC).  Of course, it's way too late to do so at this point.  Likely this needs to occur at xpcom-shutdown or even xpcom-will-shutdown (earlier?).

It's especially ironic as the DispatchToMainThread call is being made from Mainthread.

Compare https://treeherder.mozilla.org/#/jobs?repo=try&revision=16d4ff3305c8 to https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c1a548aac7a, which are the same code under 'try' sitting on top of a different base (first is the 'after' shot; second Try link is the 'first' Try link.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Even slightly more ironically, if the Dispatch had succeeded I think it would have asserted, as SendShutdown() (in TeardownRunnable) is worker-thread-only (I tried replacing the Dispatch with just a call to the SendShutdown() directly and it asserted).  I'd previously verified (at least in a non-e10s run) that ~ServiceWorkerManager() runs on MainThread() (per comment 0 here).

Side note: AssertWorkerThread() called that late in shutdown causes an ASAN hit as well.  Of course, one shouldn't be doing that. (NI-ing bent on this - something in "MOZ_RELEASE_ASSERT(mWorkerLoopID == MessageLoop::current()->id(), "...."); is reading freed memory in that case, perhaps MessageLoop::current() is returning a stale ptr.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Andrea, Nikhil, do you have any ideas here?  I'm not that familiar with the ServiceWorkerManager threading model.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8623715 [details] [diff] [review]
patch

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +4425,5 @@
>      }
>  
>      RemoveAllRegistrations(principal);
>    } else if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {
> +    mShuttingdown = true;

This is mShuttingDown

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8623715 [details] [diff] [review]
patch

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +2352,5 @@
>        registration->Clear();
>        swm->RemoveRegistration(registration);
>      }
>  
> +    // Could it be that we are shutting down.

"We could be shutting down."

Comment 6

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7653e3d5f91

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for widespread assertion failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=11003275&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/557cd9bb68b6

Comment 8

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1019348c7786

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1019348c7786