1764119 - Don't run XPCOMShutdownFinal before nsThreadManager::Shutdown has shut down threads & run shutdown tasks

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

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

In bug 1637890, XPCOM shutdown was adjusted so that KillClearOnShutdown(XPCOMShutdownFinal) was fired before nsThreadManager::Shutdown(), in order to allow additional events to be processed on the main thread from ClearOnShutdown-ed static destructors. Types which require this behaviour should probably be moved back to a different phase, such as XPCOMShutdownLoaders or XPCOMShutdownThreads, and the final shutdown phase should be moved back to happening after we've shut down threads fully.

Due to this change having existed for a while, we may need to keep the main thread accepting events for XPCOMShutdownFinal, however we are already blocking new event dispatches to background threads at that point, so we can probably shut down background threads & run shutdown callbacks.

Comment 1

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

Attachment: Bug 1764119 - Part 1: Do final KillClearOnShutdown after XPCOM threads shutdown, r=#xpcom-reviewers

This patch moves where we perform the final KillClearOnShutdown to occur
after we've shut down non-main threads, but before the main thread stops
accepting events. This should help ensure that unsuspecting events,
including those triggered from nsIThreadShutdownTask tasks, don't run
after KillClearOnShutdown has been run on background or main threads.

This KillClearOnShutdown was moved to occur before
nsThreadManager::Shutdown() in bug 1637890, as there were examples of
KillClearOnShutdown callbacks which needed to be able to dispatch
main-thread runnables. This change should not regress that use-case, as
we are still accepting new events on the main thread after the callback.

Non-main threads were already unreliable after this call as we already
block normal dispatches by setting gXPCOMThreadsShutdown, and new
threads cannot be started for the background thread pool.

Comment 2

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

Attachment: Bug 1764119 - Part 2: Don't check gXPCOMThreadsShutdown in ThreadEventTarget::Dispatch, r=#xpcom-reviewers

We already have a mechanism for ending threads accepting new messages
when they are shut down, so this only allows tasks started from thread
shutdown tasks during xpcom shutdown to behave consistently.

We already didn't prevent dispatching to the background thread pool at
this time, so it should make little difference there as well, and may
just instead save us from deadlocks where code expects a dispatch to
succeed and it does not during actor shutdown.

Depends on D144591

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8ba6b0a8c6f
Part 1: Do final KillClearOnShutdown after XPCOM threads shutdown, r=xpcom-reviewers,kmag,jstutte
https://hg.mozilla.org/integration/autoland/rev/62743521627e
Part 2: Don't check gXPCOMThreadsShutdown in ThreadEventTarget::Dispatch, r=xpcom-reviewers,kmag,jstutte

Comment 4

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing build bustages. CLOSED TREE

Backout link : https://hg.mozilla.org/integration/autoland/rev/3dc7b514d38229365e7e05b598f4c2fc90c57637

Push with failures : https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=62743521627ecce518054580e6a79263baa76a3e&selectedTaskRun=Cixstli5RhqYAeaGFIn11w.0

Link to failure logs:
https://treeherder.mozilla.org/logviewer?job_id=376500985&repo=autoland&lineNumber=27617
https://treeherder.mozilla.org/logviewer?job_id=376500979&repo=autoland&lineNumber=32492
https://treeherder.mozilla.org/logviewer?job_id=376501006&repo=autoland&lineNumber=31075

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53c35279a526
Part 1: Do final KillClearOnShutdown after XPCOM threads shutdown, r=xpcom-reviewers,kmag,jstutte
https://hg.mozilla.org/integration/autoland/rev/4117dd5eaf86
Part 2: Don't check gXPCOMThreadsShutdown in ThreadEventTarget::Dispatch, r=xpcom-reviewers,kmag,jstutte

Comment 6

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/53c35279a526
https://hg.mozilla.org/mozilla-central/rev/4117dd5eaf86

Comment 7

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

Comment on attachment 9273674 [details]
Bug 1764119 - Part 2: Don't check gXPCOMThreadsShutdown in ThreadEventTarget::Dispatch, r=#xpcom-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Shutdown crashes
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Changes to shutdown are always somewhat risky due to the weird interactions between different states which only come up in the real world and not in testing, but this patch has been on Nightly for a bit now.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 8

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

Uplift request as requested by bug 1765393 comment 11

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9273673 [details]
Bug 1764119 - Part 1: Do final KillClearOnShutdown after XPCOM threads shutdown, r=#xpcom-reviewers

We're still pretty early in the Beta cycle and this addresses a topcrash. Agreed that it's worth taking. Approved for 101.0b4.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/312a63e7e7e6
https://hg.mozilla.org/releases/mozilla-beta/rev/91beaa1f7356