1750635 - AppShutdown::IsShuttingDown() doesn't work in xpcshell tests

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Florian Quèze [:florian]]

After some debugging for bug 1745511, I noticed that AppShutdown::IsShuttingDown() already returns false in xpcshell tests.

AppShutdown::IsShuttingDown() returns the value of sIsShuttingDown which is set by AppShutdown::OnShutdownConfirmed, called by nsAppStartup::Quit.

The xpcshell test shutdown process calls Services.startup.advanceShutdownPhase multiple times instead of calling nsAppStartup::Quit: https://searchfox.org/mozilla-central/rev/72c7cef167829b6f1e24cae216fa261934c455fc/testing/xpcshell/head.js#705-716

Testing AppShutdown::GetCurrentShutdownPhase() != ShutdownPhase::NotInShutdown instead of AppShutdown::IsShuttingDown() works fine.

So possible fixes:

• make xpcshell tests call nsAppStartup::Quit. Not sure why it isn't already the case; maybe that depends on things that xpcshell doesn't initialize?
• remove the sIsShuttingDown static variable, and change the implementation of AppShutdown::IsShuttingDown to return AppShutdown::GetCurrentShutdownPhase() != ShutdownPhase::NotInShutdown.

Comment 1

[Jens Stutte [:jstutte]]

I would recommend the second strategy as I do not think we can call nsAppStartup::Quit from xpcshell. It tries to close windows we never opened and such. We might be able to call just AppShutdown::OnShutdownConfirmed without too much harm. But it fiddles with profiles, and I am not sure what that would do in xpcshell, too.

sIsShuttingDown has only very few direct uses. We have the convenience function AppShutdown::IsInOrBeyond that could be used inside AppShutdown::IsShuttingDown testing for AppShutdownConfirmed.

There is obviously much room for improvement here in general, some of the functions in AppShutdown have non-obvious side effects. See bug 1697745 you already linked here.

Comment 2

[Jens Stutte [:jstutte]]

Looking at where AppShutdown::IsShuttingDown is used there are not too many places. We could also just remove that function and replace it by the equivalent AppShutdown::IsInOrBeyond(ShutdownPhase::AppShutdownConfirmed). That makes it explicit at what phase we are looking here (which might even not always be the desired one and could give a push to rethink it for each usage).

Comment 3

[Jens Stutte [:jstutte]]

Attachment: Bug 1750635: Substitute AppShutdown:IsShuttingDown with equivalent AppShutdown::IsInOrBeyond. r?#ipc-reviewers!,florian

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08d2dc5327f7
Substitute AppShutdown:IsShuttingDown with equivalent AppShutdown::IsInOrBeyond. r=florian,xpcom-reviewers,nika

Comment 5

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/08d2dc5327f7