1797688 - Check if we can substitute gXPCOMThreadsShutDown with AppShutdown::IsInOrBeyond(ShutdownPhase::?)

Status: RESOLVED FIXED

(Core :: XPCOM, task, P3)

Description

[Jens Stutte [:jstutte]]

gXPCOMThreadsShutDown is set only after we fired the XPCOMShutdownThreads notification. Places that check for it might thus assume that it is still safe to dispatch events to other, non-main-thread threads while the observer is firing.

Conceptually it seems better to check for AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMShutdownThreads) in those places.

We then can hopefully make gXPCOMThreadsShutDown become a debug only variable used only for the assertion in ThreadEventTarget::Dispatch, see comment 9.

Comment 1

[Jens Stutte [:jstutte]]

BlobURL - ReleasingTimerHolder::Create

The comment there seems to say that we only need to be able to dispatch events to the main thread here, and indeed IIUC SchedulerGroup::Dispatch dispatches to the main thread only. This would probably translate more to a AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMShutdownFinal). But there is some weirdness there, we guard those holders by an async shutdown blocker for XPCOMWillShutdown which will probably fail for later phases anyways?

ReleasingTimerHolder::Run will self->CancelTimerAndRevokeURI(); if the async shutdown blocker is not present anymore. But we still will RevokeUri() synchronously in this case. So dispatching the ReleasingTimerHolder later than XPCOMWillShutdown might actually make sense.

Comment 2

[Jens Stutte [:jstutte]]

gfxFontInfoLoader::StartLoader

It feels wrong to start that loader such late, but substituting the assert with AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMShutdownThreads) should not hurt. But furthermore if we do the DelayedStartCallback we should not just assert but bail out also in release, as this seems very possible to happen.

Comment 3

[Jens Stutte [:jstutte]]

HTMLSlotElement::EnqueueSlotChangeEvent

I see no obvious reason for AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMShutdownThreads) to make things worse here. Try will tell.

Comment 4

[Jens Stutte [:jstutte]]

IDecodingTask::NotifyProgress

This check feels very late, so I see no obvious reason for AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMShutdownThreads) to make things worse here. Try will tell.

Comment 5

[Jens Stutte [:jstutte]]

ProgressTracker

D67635 added this check. TBH I think there should be more checks, in particular every time mEventTarget is actually used to dispatch something. Needs some reasoning.

Comment 6

[Jens Stutte [:jstutte]]

SurfaceCacheImpl::ReleaseImageOnMainThread

The two checks there ask for the main thread to be still active. We can probably substitute with AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMShutdownFinal).

Comment 7

[Jens Stutte [:jstutte]]

IdleSchedulerParent

There seems to be a ping-pong between the current thread and the background thread. IMHO this can only benefit from checking AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMShutdownThreads) instead and probably the check should be done also before the other dispatch in that function?

Comment 8

[Jens Stutte [:jstutte]]

IdlePeriodState::GetLocalIdleDeadline

The comment right before probably says that AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMShutdownThreads) would be actually necessary here to ensure immediate processing already while the notification fired.

Comment 9

[Jens Stutte [:jstutte]]

ThreadEventTarget::Dispatch

This is probably the most fragile assertion to be changed in this context. I think asserting already for AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMShutdownThreads) would be the right call here, but I'd expect some fallout from that change. Though I believe that any fallout from this must be corrected to happen in an earlier phase, as we have no guarantees about the order with which the threads are shut down during XPCOMShutdownThreads, IIUC. Try will tell.

So a quick check teaches me that we cannot anticipate this assertion. Threads need to communicate with other threads for their shutdown. Looking again at ShutdownXPCOM:

    mozilla::AppShutdown::AdvanceShutdownPhase(mozilla::ShutdownPhase::XPCOMShutdownThreads);
    gXPCOMThreadsShutDown = true;
    NS_ProcessPendingEvents(thread);

    // Shutdown the timer thread and all timers that might still be alive
    nsTimerImpl::Shutdown();
    NS_ProcessPendingEvents(thread);

    // Shutdown all remaining threads. 
    nsThreadManager::get().ShutdownNonMainThreads();

    RefPtr<nsObserverService> observerService;
    CallGetService("@mozilla.org/observer-service;1",
                   (nsObserverService**)getter_AddRefs(observerService));
    if (observerService) {
      observerService->Shutdown();
    }

    AppShutdown::AdvanceShutdownPhase(ShutdownPhase::XPCOMShutdownFinal);

gXPCOMThreadsShutDown wants to explicitly capture the moment when the XPCOMShutdownThreads observer notification has been processed. Then there is the nsTimerImpl::Shutdown(); and the nsThreadManager::get().ShutdownNonMainThreads();, so the assert is meant to be active only there. This has already been discussed in its patch. It might be worth to capture some of that discussion as a comment next to the assert to avoid future circling back.

Comment 10

[Jens Stutte [:jstutte]]

Threads need to communicate with other threads for their shutdown.

At least to some extent. I am just wondering if we could find some rules, which threads are supposed to talk with which in the XPCOMShutdownThreads phase. Something on the lines of thread hierarchy:

• A thread can dispatch messages to all children it spawned (and wants to join for shutdown)
• A thread can dispatch messages to its parent

Not sure if we would have the information to reliably check this at hand, though.

Comment 11

[Jens Stutte [:jstutte]]

Attachment: Bug 1797688 - Part 1: Remove unnecessary check for gXPCOMThreadsShutDown in the ReleasingTimerHolder. r?#dom-storage-reviewers

Comment 12

[Jens Stutte [:jstutte]]

Attachment: Bug 1797688 - Part 2: Bail out from gfxFontInfoLoader::StartLoader InOrBeyond(AppShutdown). r?jfkthame,lsalzman

Depends on D160619

Comment 13

[Jens Stutte [:jstutte]]

Attachment: Bug 1797688 - Part 3: Bail out from HTMLSlotElement::EnqueueSlotChangeEvent InOrBeyond(XPCOMShutdownThreads). r?#xpcom-reviewers

Depends on D160620

Comment 14

[Jens Stutte [:jstutte]]

Attachment: Bug 1797688 - Part 4: Bail out from IDecodingTask::NotifyProgress and NotifyDecodeComplete InOrBeyond(XPCOMShutdownThreads). r?#xpcom-reviewers

Depends on D160621

Comment 15

[Jens Stutte [:jstutte]]

Attachment: Bug 1797688 - Part 5: Substitute gXPCOMThreadsShutDown with InOrBeyond(XPCOMShutdownThreads) in ProgressTracker::RemoveObserver. r?#xpcom-reviewers,tnikkel

Depends on D160622

Comment 16

[Jens Stutte [:jstutte]]

Attachment: Bug 1797688 - Part 6: Substitute gXPCOMThreadsShutDown with InOrBeyond(XPCOMShutdownFinal) in SurfaceCacheImpl. r?#xpcom-reviewers

Depends on D160623

Comment 17

[Jens Stutte [:jstutte]]

Attachment: Bug 1797688 - Part 7: Substitute gXPCOMThreadsShutDown with InOrBeyond checks in IdleSchedulerParent. r?#xpcom-reviewers

Depends on D160624

Comment 18

[Jens Stutte [:jstutte]]

Attachment: Bug 1797688 - Part 8: Substitute gXPCOMThreadsShutDown with InOrBeyond(XPCOMShutdownThreads) in IdlePeriodState::GetLocalIdleDeadline. r?#xpcom-reviewers

Depends on D160625

Comment 19

[Jens Stutte [:jstutte]]

Attachment: Bug 1797688 - Part 9: Make gXPCOMThreadsShutDown DEBUG only. r?#xpcom-reviewers

Depends on D160626

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7bf0fa2a9cda
Part 1: Remove unnecessary check for gXPCOMThreadsShutDown in the ReleasingTimerHolder. r=dom-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/03d862aee42e
Part 2: Bail out from gfxFontInfoLoader::StartLoader InOrBeyond(AppShutdown). r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/ba4d2d11fcfb
Part 3: Bail out from HTMLSlotElement::EnqueueSlotChangeEvent InOrBeyond(XPCOMShutdownThreads). r=xpcom-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/237e6b424953
Part 4: Bail out from IDecodingTask::NotifyProgress and NotifyDecodeComplete InOrBeyond(XPCOMShutdownThreads). r=xpcom-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/f918feb70c60
Part 5: Substitute gXPCOMThreadsShutDown with InOrBeyond(XPCOMShutdownThreads) in ProgressTracker::RemoveObserver. r=xpcom-reviewers,mccr8,tnikkel
https://hg.mozilla.org/integration/autoland/rev/549bc91d3d11
Part 6: Substitute gXPCOMThreadsShutDown with InOrBeyond(XPCOMShutdownFinal) in SurfaceCacheImpl. r=xpcom-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/10dbe4a98b50
Part 7: Substitute gXPCOMThreadsShutDown with InOrBeyond checks in IdleSchedulerParent. r=xpcom-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/7e83216a5cb5
Part 8: Substitute gXPCOMThreadsShutDown with InOrBeyond(XPCOMShutdownThreads) in IdlePeriodState::GetLocalIdleDeadline. r=xpcom-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/9be2b7c981dd
Part 9: Make gXPCOMThreadsShutDown DEBUG only. r=xpcom-reviewers,mccr8

Comment 21

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/7bf0fa2a9cda
https://hg.mozilla.org/mozilla-central/rev/03d862aee42e
https://hg.mozilla.org/mozilla-central/rev/ba4d2d11fcfb
https://hg.mozilla.org/mozilla-central/rev/237e6b424953
https://hg.mozilla.org/mozilla-central/rev/f918feb70c60
https://hg.mozilla.org/mozilla-central/rev/549bc91d3d11
https://hg.mozilla.org/mozilla-central/rev/10dbe4a98b50
https://hg.mozilla.org/mozilla-central/rev/7e83216a5cb5
https://hg.mozilla.org/mozilla-central/rev/9be2b7c981dd