988872 - DOMWorker threads are frequently not ended by the end of xpcom-shutdown-threads, various discussion about idle threads

Status: RESOLVED WORKSFORME

(Core :: DOM: Workers, defect, P3)

Description

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

With the patch from bug 988464, we frequently see 1-3 DOMWorker threads still active after xpcom-shutdown-threads (which is after xpcom-shutdown).  One run with an earlier version of the patch:

https://tbpl.mozilla.org/?tree=Try&rev=8ac1c136a95f

See https://wiki.mozilla.org/XPCOM_Shutdown 

xpcom-shutdown-threads: "Any non-main-thread event queues should be finished by the module which owns them. All threads running XPCOM code should be joined. Any special services that might be required for component loader shutdown should be obtained and cached by the end of this notification. Initial garbage collection should be performed at this stage."

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm kinda skeptical... We spin the event loop waiting for all worker threads in response to the 'xpcom-shutdown-threads' notification already:

https://mxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp#2292

In fact it's a frequent intermittent orange to hang on shutdown here.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ah, it looks like we were letting the thread manager take care of idle threads if we don't have any live workers.

Comment 3

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

A quick glance at ::Shutdown() and ::ShutdownIdleThreads() implies to me that this behavior hasn't changed, and so we can expect this may still be an issue once we land NS_ASSERTIONs on nsThread leakage.  Kyle, is there someone available to deal with this, or was it fixed and just wasn't obvious to a trivial once-over?

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Doesn't http://hg.mozilla.org/mozilla-central/annotate/d590b9601ba8/dom/workers/RuntimeService.cpp#l2107 take care of this?  Are we ending up in a situation where we have no live workers but do have idle threads, so we don't execute that code?

Comment 5

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #4)
> Doesn't
> http://hg.mozilla.org/mozilla-central/annotate/d590b9601ba8/dom/workers/
> RuntimeService.cpp#l2107 take care of this?  Are we ending up in a situation
> where we have no live workers but do have idle threads, so we don't execute
> that code?

Well, that appears unchanged (more-or-less) from when bent wrote comment 2, so my bet would be that yes, we have no live workers, but do/did have idle ones (at least when the phase of the moon is right).  We'll see if this repeats with current code.  I'll likely throw a bunch of retriggers at some of the tests to probe for new intermittents before landing.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Adjusting the condition to enter that block seems like a no-brainer.

Comment 7

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> Adjusting the condition to enter that block seems like a no-brainer.

Yeah, though I'm not sure what bent's comment really meant....  I don't see that the test to enter that block is based on active workers, just top-level workers.  However, I'll also note that the 

    mDomainMap.EnumerateRead(AddAllTopLevelWorkersToArray, &workers);
    if (!workers.IsEmpty()) {

appears to do little useful - workers is never used again, and all we do in there is clear out mIdleThreadArray & shut-them-down, and spin the event loop waiting for mDomainMap to be empty (so let's hope they're all in mIdleThreadArray).

Not being sure of how the top-level-worker part is used, it seems like it could skip this if there were no top-level workers but there *are* idle workers... if that's really possible, then yes we should adjust it.  However, the "spin until mDomainMap is empty" implies that all workers *must* be in the idle array, or that if somehow they're not, shutting down the ones in the idle array will clear them out (or maybe that they've been told to shutdown before Cleanup() was called, and we just need to spin until they're dead).  I note that on xpcom-shutdown if calls Shutdown(), which does Kill() on all top-level workers - however I don't know what Kill() on a worker does exactly (move to idle?  cause it to die and send a "remove-me-from-worker-lists" event to main?)

I suppose we could also add an assertion that mDomainMap is empty after this part of Cleanup, or an assertion that mIdleThreadArray is empty after this (or both).  Cleanup() is called from xpcom-thread-shutdown.

Comment 8

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

FYI, adjusting the condition to enter the block had no affect (which matches the analysis in comment 7).  
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7de1f0c0c1f4
DOM Worker threads are leaking in bunches of xpcshell tests.  They didn't leak when I ran those specific tests locally in xpcshell-test

Kyle, any other ideas?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Other than "debug it and see what's going on"?  no.

Comment 10

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

This is still happening: http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-3dc899f6817214841e1820ca4bb4cb5a7e747d48/try-linux-debug/try_ubuntu32_vm-debug_test-xpcshell-2-bm02-tests1-linux32-build1288.txt.gz

Comment 11

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

I tried a local xpcshell testrun with the if (!workers.IsEmpty()) short-circuited (to "if (1)"), and didn't see any leaks in xpcshell tests (though I prefer to use a better solution).  (line 1986 now)

Running a Try.

Comment 12

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

DOM Workers still leak in almost every mochitest - 1 to 8 of them depending on timing.

Comment 13

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

I suspect there are DOM Worker threads that aren't idle already at shutdown (and shutdown via mIdleThreadArray being Shutdown()), and while they're probably sent a Kill(), they may not end up calling NoteIdleThread() and calling Shutdown() before the world ends, and they're still alive at threadmanager-shutdown time.  

Probably it should use a ShutdownBlocker to allow all the DOM Worker threads to be cleaned up properly; once they've all been ShutDown(), we can release the blocker.

Comment 14

[Worcester12345]

"PERF" key word?

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

This bug seems to have largely ended up being about idle threads and those were removed by bug 1766306 so I am going to resolve this WFM since we have a sufficient number of other bugs with actionable stacks and metadata directly on the bug. (In particular, we have crash signatures for the RuntimeService inducing a crash where we have asked each worker why it's being a jerk via a control runnable.)