1707924 - Background TaskQueues created after beginning shutdown will cause a hang

Status: RESOLVED WORKSFORME

(Core :: XPCOM, defect)

Description

[Kris Wright :KrisWright]

There is an order of events that causes a hang:

• A background TaskQueue is created after we begin threads shut down
• The task queue enters shut down, and attempts to [cancel delayed runnables] (https://searchfox.org/mozilla-central/rev/37edd2782e67e716dd07a85016da07b4d6275e5d/xpcom/threads/TaskQueue.cpp#163)
• The task queue tail dispatches a runnable which clears its delayed runnables
• Because delayed runnables are canceled before we stop servicing new threads, most TaskQueue clear this step. But if you create one at this part of shutdown, you dispatch it to a threadpool that cannot create new threads, so if there are no active threads this dispatch will not be serviced.

We could just change how to dispatch delayed runnable cancel tasks, but I think the best solution is to outright forbid the creation of new background TaskQueues after we have begun shutdown. Since this problem is specific to the way background task queue shut down, I don't want to change the behavior of any unrelated task queue who dispatch delayed runnables and shut down on their own. It also isn't a good idea to allow creation of background TaskQueue to dispatch with at a point when dispatch may not be possible (ie no thread to service it).

Comment 1

[Kris Wright :KrisWright]

Attachment: Bug 1707924 - Disallow creation of new TaskQueues after we have begun threads shutdown

I discovered today an order of events that causes a hang:

• A background TaskQueue is created after we begin threads shut down
• The task queue enters shut down, and attempts to [cancel delayed runnables] (https://searchfox.org/mozilla-central/rev/37edd2782e67e716dd07a85016da07b4d6275e5d/xpcom/threads/TaskQueue.cpp#163)
• The task queue tail dispatches a runnable which clears its delayed runnables
• Because delayed runnables are canceled before we stop servicing new threads, most TaskQueue clear this step. But if you create one at this part of shutdown, you dispatch it to a threadpool that cannot create new threads, so if there are no active threads this dispatch will not be serviced.

We could just change how to dispatch delayed runnable cancel tasks, but I think the best solution is to outright forbid the creation of new background TaskQueues after we have begun shutdown. Since this problem is specific to the way background task queue shut down, I don't want to change the behavior of any unrelated task queue who dispatch delayed runnables and shut down in their own time.

Comment 2

[Kris Wright :KrisWright]

(In reply to Kris Wright :KrisWright from comment #0)

• The task queue tail dispatches a runnable which clears its delayed runnables

I want to leave a note here for clarity that this is not a true tail dispatch, but the callsite will actually do a regular dispatch at the tail. This is done to guarantee a dispatch sync.

Comment 3

[Kris Wright :KrisWright]

Attachment: Bug 1707924 - Assert on DelayedRunnable dispatch on non debug builds.

This assertion currently only happens on debug builds, but attempts to dispatch delayed runnables after we have started or finished cancelling all delayed runnables in a TaskQueue may be causing shutdown hangs.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6213e0b8024
Disallow creation of new TaskQueues after we have begun threads shutdown r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/7fda47840322
Assert on DelayedRunnable dispatch on non debug builds. r=xpcom-reviewers,nika

Comment 5

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1707924) for multiple failures with crash [@ BackgroundEventTarget::CreateBackgroundTaskQueue(char const*)]. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer?job_id=338255576&repo=autoland&lineNumber=1810

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=7fda478403223401a397c1dfc0d251e31a9be923

Backout:
https://hg.mozilla.org/integration/autoland/rev/683e736385c43ea939b81b1e0a0ac50e673bfe31

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:KrisWright, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 7

[Kris Wright :KrisWright]

This bug is related to a few other bugs which are being updated.

Comment 8

[Jens Stutte [:jstutte]]

FWIW, just a note on gXPCOMThreadsShutDown in general: The condition gXPCOMThreadsShutDown == true is now equivalent to mozilla::AppShutdown::GetCurrentShutdownPhase() >= mozilla::ShutdownPhase::XPCOMShutdownThreads, meaning that we could remove this additional global if we want (and check for this condition from everywhere we want).

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Do we still plan to do something here, or should we close this bug out?

Comment 10

[Kris Wright :KrisWright]

From my understanding this behavior shouldn't work during shutdown anymore. I think we're good to close it out.