Skip to content

Commit

Permalink
Bug 1689953: Harmonize shutdown phase definitions across nsTerminator…
Browse files Browse the repository at this point in the history
… and AppShutdown r=dthayer,chutten

This patch wants to solve several quirks around the shutdown terminator.

 - Use the same shutdown phase definitions in AppShutdown and nsTerminator. This touches quite a few files.
 - Ensure that the terminator phase shift is handled before any shutdown observer notifications are sent and reduce its heartbeat duration.
 - Add missing phases to the shutdown telemetry.

Please note that this changes the unit of "tick" to 100ms rather than 1s.
As a side effect, we also remove the obsolete "shutdown-persist" context.

While the existing test coverage continues to prove the most important functions, we acknowledge the wish for better test coverage with [[ https://bugzilla.mozilla.org/show_bug.cgi?id=1693966 | bug 1693966 ]].

Differential Revision: https://phabricator.services.mozilla.com/D103626
  • Loading branch information
jensstutte committed Feb 26, 2021
1 parent 466e0ca commit 02aaf67
Show file tree
Hide file tree
Showing 42 changed files with 370 additions and 235 deletions.
2 changes: 1 addition & 1 deletion accessible/ipc/win/DocAccessibleChild.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ DocAccessibleChild::DocAccessibleChild(DocAccessible* aDoc, IProtocol* aManager)
MOZ_COUNT_CTOR_INHERITED(DocAccessibleChild, DocAccessibleChildBase);
if (!sPlatformChild) {
sPlatformChild = new PlatformChild();
ClearOnShutdown(&sPlatformChild, ShutdownPhase::Shutdown);
ClearOnShutdown(&sPlatformChild, ShutdownPhase::XPCOMShutdown);
}

SetManager(aManager);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ async function fake_profile_change() {
}, "cookie-db-closed");
Services.cookies
.QueryInterface(Ci.nsIObserver)
.observe(null, "profile-before-change", "shutdown-persist");
.observe(null, "profile-before-change", null);
});
await new Promise(resolve => {
Services.obs.addObserver(function waitForDBOpen() {
Expand Down
2 changes: 1 addition & 1 deletion dom/ipc/ContentChild.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ ContentChild::ContentChild()
// happens without requiring the observer service at this time.
if (!sShutdownCanary) {
sShutdownCanary = new ShutdownCanary();
ClearOnShutdown(&sShutdownCanary, ShutdownPhase::Shutdown);
ClearOnShutdown(&sShutdownCanary, ShutdownPhase::XPCOMShutdown);
}
}

Expand Down
2 changes: 1 addition & 1 deletion dom/ipc/ContentParent.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2839,7 +2839,7 @@ bool ContentParent::InitInternal(ProcessPriority aInitialPriority) {
// can't init the process without it, and since we're going to be canceling
// whatever load attempt that initiated this process creation anyway, just
// bail out now if shutdown has already started.
if (PastShutdownPhase(ShutdownPhase::Shutdown)) {
if (PastShutdownPhase(ShutdownPhase::XPCOMShutdown)) {
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion dom/media/MediaCache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,7 @@ RefPtr<MediaCache> MediaCache::GetMediaCache(int64_t aContentLength,
thread->Shutdown();
}
} sClearThread;
ClearOnShutdown(&sClearThread, ShutdownPhase::ShutdownThreads);
ClearOnShutdown(&sClearThread, ShutdownPhase::XPCOMShutdownThreads);
}

if (!sThread) {
Expand Down
6 changes: 4 additions & 2 deletions dom/media/doctor/DecoderDoctorLogger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,11 @@ bool DecoderDoctorLogger::EnsureLogIsEnabled() {
TaskCategory::Other,
NS_NewRunnableFunction("DDLogger shutdown setup", [] {
sDDLogShutdowner = MakeUnique<DDLogShutdowner>();
ClearOnShutdown(&sDDLogShutdowner, ShutdownPhase::Shutdown);
ClearOnShutdown(&sDDLogShutdowner,
ShutdownPhase::XPCOMShutdown);
sDDLogDeleter = MakeUnique<DDLogDeleter>();
ClearOnShutdown(&sDDLogDeleter, ShutdownPhase::ShutdownThreads);
ClearOnShutdown(&sDDLogDeleter,
ShutdownPhase::XPCOMShutdownThreads);
})));

// Nobody else should change the state when *we* are enabling logging.
Expand Down
2 changes: 1 addition & 1 deletion dom/media/mediacapabilities/MediaCapabilities.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ already_AddRefed<Promise> MediaCapabilities::DecodingInfo(
NS_NewRunnableFunction(
"MediaCapabilities::AllocPolicy:Video", []() {
ClearOnShutdown(&sVideoAllocPolicy,
ShutdownPhase::ShutdownThreads);
ShutdownPhase::XPCOMShutdownThreads);
}));
return new SingleAllocPolicy(TrackInfo::TrackType::kVideoTrack,
taskQueue);
Expand Down
6 changes: 4 additions & 2 deletions dom/media/platforms/AllocationPolicy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ NotNull<AllocPolicy*> GlobalAllocPolicy::Instance(TrackType aTrack) {
TaskCategory::Other,
NS_NewRunnableFunction(
"GlobalAllocPolicy::GlobalAllocPolicy:Audio", []() {
ClearOnShutdown(&sAudioPolicy, ShutdownPhase::ShutdownThreads);
ClearOnShutdown(&sAudioPolicy,
ShutdownPhase::XPCOMShutdownThreads);
}));
return new AllocPolicyImpl(MediaDecoderLimitDefault());
}();
Expand All @@ -108,7 +109,8 @@ NotNull<AllocPolicy*> GlobalAllocPolicy::Instance(TrackType aTrack) {
TaskCategory::Other,
NS_NewRunnableFunction(
"GlobalAllocPolicy::GlobalAllocPolicy:Audio", []() {
ClearOnShutdown(&sVideoPolicy, ShutdownPhase::ShutdownThreads);
ClearOnShutdown(&sVideoPolicy,
ShutdownPhase::XPCOMShutdownThreads);
}));
return new AllocPolicyImpl(MediaDecoderLimitDefault());
}();
Expand Down
2 changes: 1 addition & 1 deletion dom/media/webrtc/CubebDeviceEnumerator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ CubebDeviceEnumerator* CubebDeviceEnumerator::GetInstance() {
sInstance = new CubebDeviceEnumerator();
static bool clearOnShutdownSetup = []() -> bool {
auto setClearOnShutdown = []() -> void {
ClearOnShutdown(&sInstance, ShutdownPhase::ShutdownThreads);
ClearOnShutdown(&sInstance, ShutdownPhase::XPCOMShutdownThreads);
};
if (NS_IsMainThread()) {
setClearOnShutdown();
Expand Down
2 changes: 1 addition & 1 deletion dom/media/webrtc/transport/nr_socket_prsock.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ class SingletonThreadHolder final {
static StaticRefPtr<SingletonThreadHolder> sThread;

static void ClearSingletonOnShutdown() {
ClearOnShutdown(&sThread, ShutdownPhase::ShutdownLoaders);
ClearOnShutdown(&sThread, ShutdownPhase::XPCOMShutdownLoaders);
}
#endif

Expand Down
2 changes: 1 addition & 1 deletion dom/plugins/ipc/PluginProcessChild.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ void PluginProcessChild::CleanUp() {
NS_LogTerm();
#endif

mozilla::KillClearOnShutdown(ShutdownPhase::ShutdownFinal);
mozilla::KillClearOnShutdown(ShutdownPhase::XPCOMShutdownFinal);

AbstractThread::ShutdownMainThread();

Expand Down
2 changes: 1 addition & 1 deletion dom/storage/SessionStorageManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,7 @@ BackgroundSessionStorageManager* BackgroundSessionStorageManager::GetOrCreate(
return;
}
},
ShutdownPhase::Shutdown);
ShutdownPhase::XPCOMShutdown);
}));
}

Expand Down
2 changes: 1 addition & 1 deletion dom/webauthn/U2FTokenManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class U2FPrefManager final : public nsIObserver {
PREF_WEBAUTHN_ANDROID_FIDO2_ENABLED);
Preferences::AddStrongObserver(gPrefManager,
PREF_WEBAUTHN_ALLOW_DIRECT_ATTESTATION);
ClearOnShutdown(&gPrefManager, ShutdownPhase::ShutdownThreads);
ClearOnShutdown(&gPrefManager, ShutdownPhase::XPCOMShutdownThreads);
}
return gPrefManager;
}
Expand Down
2 changes: 1 addition & 1 deletion gfx/vr/VRManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ VRManager::VRManager()
VRServiceHost::Init(mVRProcessEnabled);
mServiceHost = VRServiceHost::Get();
// We must shutdown before VRServiceHost, which is cleared
// on ShutdownPhase::ShutdownFinal, potentially before VRManager.
// on ShutdownPhase::XPCOMShutdownFinal, potentially before VRManager.
// We hold a reference to VRServiceHost to ensure it stays
// alive until we have shut down.
#else
Expand Down
3 changes: 2 additions & 1 deletion gfx/vr/VRServiceHost.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,8 @@ void VRServiceHost::PuppetReset() {
// If we're already into ShutdownFinal, the VRPuppetCommandBuffer instance
// will have been cleared, so don't try to access it after that point.
if (!mVRProcessEnabled &&
!(NS_IsMainThread() && PastShutdownPhase(ShutdownPhase::ShutdownFinal))) {
!(NS_IsMainThread() &&
PastShutdownPhase(ShutdownPhase::XPCOMShutdownFinal))) {
// Puppet is running in this process, tell it to reset directly.
VRPuppetCommandBuffer::Get().Reset();
}
Expand Down
2 changes: 1 addition & 1 deletion intl/locale/LocaleService.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ LocaleService* LocaleService::GetInstance() {
}
// DOM might use ICUUtils and LocaleService during UnbindFromTree by
// final cycle collection.
ClearOnShutdown(&sInstance, ShutdownPhase::ShutdownPostLastCycleCollection);
ClearOnShutdown(&sInstance, ShutdownPhase::CCPostLastCycleCollection);
}
return sInstance;
}
Expand Down
4 changes: 2 additions & 2 deletions intl/strres/nsStringBundle.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ nsresult nsStringBundleBase::ParseProperties(nsIPersistentProperties** aProps) {
nsresult nsStringBundle::LoadProperties() {
// Something such as Necko might use string bundle after ClearOnShutdown is
// called. LocaleService etc is already down, so we cannot get bundle data.
if (PastShutdownPhase(ShutdownPhase::Shutdown)) {
if (PastShutdownPhase(ShutdownPhase::XPCOMShutdown)) {
return NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
}

Expand Down Expand Up @@ -500,7 +500,7 @@ nsresult SharedStringBundle::LoadProperties() {
// our string bundles come from). Since shared string bundles won't be
// useful after shutdown has started anyway (and we almost certainly got
// here from a pre-load attempt in an idle task), just bail out.
if (PastShutdownPhase(ShutdownPhase::Shutdown)) {
if (PastShutdownPhase(ShutdownPhase::XPCOMShutdown)) {
return NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
}

Expand Down
2 changes: 1 addition & 1 deletion ipc/mscom/EnsureMTA.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ nsCOMPtr<nsIThread> EnsureMTA::GetMTAThread() {
BackgroundMTAData* bgData = new BackgroundMTAData();

auto setClearOnShutdown = [ptr = &sMTAData]() -> void {
ClearOnShutdown(ptr, ShutdownPhase::ShutdownThreads);
ClearOnShutdown(ptr, ShutdownPhase::XPCOMShutdownThreads);
};

if (NS_IsMainThread()) {
Expand Down
2 changes: 1 addition & 1 deletion ipc/mscom/Registration.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ void RegisterArrayData(const ArrayData* aArrayData, size_t aLength) {

if (!sArrayData) {
sArrayData = new Vector<std::pair<const ArrayData*, size_t>>();
ClearOnShutdown(&sArrayData, ShutdownPhase::ShutdownThreads);
ClearOnShutdown(&sArrayData, ShutdownPhase::XPCOMShutdownThreads);
}

MOZ_ALWAYS_TRUE(sArrayData->emplaceBack(std::make_pair(aArrayData, aLength)));
Expand Down
2 changes: 1 addition & 1 deletion js/xpconnect/loader/mozJSComponentLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1220,7 +1220,7 @@ nsresult mozJSComponentLoader::Import(JSContext* aCx,
!mInProgressImports.Get(info.Key(), &mod)) {
// We're trying to import a new JSM, but we're late in shutdown and this
// will likely not succeed and might even crash, so fail here.
if (PastShutdownPhase(ShutdownPhase::ShutdownFinal)) {
if (PastShutdownPhase(ShutdownPhase::XPCOMShutdownFinal)) {
return NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
}

Expand Down
3 changes: 1 addition & 2 deletions netwerk/protocol/http/nsHttpHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,7 @@ already_AddRefed<nsHttpHandler> nsHttpHandler::GetInstance() {
MOZ_ASSERT(NS_SUCCEEDED(rv));
// There is code that may be executed during the final cycle collection
// shutdown and still referencing gHttpHandler.
ClearOnShutdown(&gHttpHandler,
ShutdownPhase::ShutdownPostLastCycleCollection);
ClearOnShutdown(&gHttpHandler, ShutdownPhase::CCPostLastCycleCollection);
}
RefPtr<nsHttpHandler> httpHandler = gHttpHandler;
return httpHandler.forget();
Expand Down
4 changes: 2 additions & 2 deletions netwerk/test/unit/head_cookies.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ function do_close_profile(generator) {

// Close the db.
let service = Services.cookies.QueryInterface(Ci.nsIObserver);
service.observe(null, "profile-before-change", "shutdown-persist");
service.observe(null, "profile-before-change", null);
}

function _promise_observer(topic) {
Expand Down Expand Up @@ -131,7 +131,7 @@ function promise_close_profile() {

// Close the db.
let service = Services.cookies.QueryInterface(Ci.nsIObserver);
service.observe(null, "profile-before-change", "shutdown-persist");
service.observe(null, "profile-before-change", null);

return promise;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ static LinkedList<ChannelWrapper>& ChannelList() {
static UniquePtr<ChannelListHolder> sChannelList;
if (!sChannelList) {
sChannelList.reset(new ChannelListHolder());
ClearOnShutdown(&sChannelList, ShutdownPhase::Shutdown);
ClearOnShutdown(&sChannelList, ShutdownPhase::XPCOMShutdown);
}
return *sChannelList;
}
Expand Down
9 changes: 4 additions & 5 deletions toolkit/components/startup/nsAppStartup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -441,11 +441,10 @@ nsAppStartup::Quit(uint32_t aMode, int aExitCode, bool* aUserAllowedQuit) {

// No chance of the shutdown being cancelled from here on; tell people
// we're shutting down for sure while all services are still available.
if (obsService) {
bool isRestarting = mozilla::AppShutdown::IsRestarting();
obsService->NotifyObservers(nullptr, "quit-application",
isRestarting ? u"restart" : u"shutdown");
}
bool isRestarting = mozilla::AppShutdown::IsRestarting();
mozilla::AppShutdown::AdvanceShutdownPhase(
mozilla::ShutdownPhase::AppShutdownConfirmed,
isRestarting ? u"restart" : u"shutdown");

if (!mRunning) {
postedExitEvent = true;
Expand Down
45 changes: 43 additions & 2 deletions toolkit/components/telemetry/Histograms.json
Original file line number Diff line number Diff line change
Expand Up @@ -12632,34 +12632,75 @@
"kind": "exponential",
"high": 65,
"n_buckets": 10,
"bug_numbers": [1689953],
"alert_emails": ["[email protected], [email protected]"],
"description": "Duration of shutdown phase quit-application, as measured by the shutdown terminator, in seconds of activity"
},
"SHUTDOWN_PHASE_DURATION_TICKS_PROFILE_CHANGE_NET_TEARDOWN": {
"record_in_processes": ["main", "content"],
"products": ["firefox", "fennec", "thunderbird"],
"expires_in_version": "never",
"kind": "exponential",
"high": 65,
"n_buckets": 10,
"bug_numbers": [1689953],
"alert_emails": ["[email protected], [email protected]"],
"description": "Duration of shutdown phase profile-change-net-teardown, as measured by the shutdown terminator, in seconds of activity"
},
"SHUTDOWN_PHASE_DURATION_TICKS_PROFILE_CHANGE_TEARDOWN": {
"record_in_processes": ["main", "content"],
"products": ["firefox", "fennec", "thunderbird"],
"expires_in_version": "never",
"kind": "exponential",
"high": 65,
"n_buckets": 10,
"bug_numbers": [1689953],
"alert_emails": ["[email protected], [email protected]"],
"description": "Duration of shutdown phase profile-change-teardown, as measured by the shutdown terminator, in seconds of activity"
},
"SHUTDOWN_PHASE_DURATION_TICKS_PROFILE_BEFORE_CHANGE": {
"record_in_processes": ["main", "content"],
"products": ["firefox", "fennec", "thunderbird"],
"expires_in_version": "never",
"kind": "exponential",
"high": 65,
"n_buckets": 10,
"bug_numbers": [1689953],
"alert_emails": ["[email protected], [email protected]"],
"description": "Duration of shutdown phase profile-before-change, as measured by the shutdown terminator, in seconds of activity"
},
"SHUTDOWN_PHASE_DURATION_TICKS_PROFILE_BEFORE_CHANGE_QM": {
"record_in_processes": ["main", "content"],
"products": ["firefox", "fennec", "thunderbird"],
"expires_in_version": "never",
"kind": "exponential",
"high": 65,
"n_buckets": 10,
"bug_numbers": [1689953],
"alert_emails": ["[email protected], [email protected]"],
"description": "Duration of shutdown phase profile-before-change-qm, as measured by the shutdown terminator, in seconds of activity"
},
"SHUTDOWN_PHASE_DURATION_TICKS_XPCOM_WILL_SHUTDOWN": {
"record_in_processes": ["main", "content"],
"products": ["firefox", "fennec", "thunderbird"],
"expires_in_version": "never",
"kind": "exponential",
"high": 65,
"n_buckets": 10,
"bug_numbers": [1689953],
"alert_emails": ["[email protected], [email protected]"],
"description": "Duration of shutdown phase xpcom-will-shutdown, as measured by the shutdown terminator, in seconds of activity"
},
"SHUTDOWN_PHASE_DURATION_TICKS_PROFILE_BEFORE_CHANGE": {
"SHUTDOWN_PHASE_DURATION_TICKS_XPCOM_SHUTDOWN": {
"record_in_processes": ["main", "content"],
"products": ["firefox", "fennec", "thunderbird"],
"expires_in_version": "never",
"kind": "exponential",
"high": 65,
"n_buckets": 10,
"description": "Duration of shutdown phase profile-before-change, as measured by the shutdown terminator, in seconds of activity"
"bug_numbers": [1689953],
"alert_emails": ["[email protected], [email protected]"],
"description": "Duration of shutdown phase xpcom-shutdown, as measured by the shutdown terminator, in seconds of activity"
},
"TAP_TO_LOAD_ENABLED": {
"record_in_processes": ["main", "content"],
Expand Down
8 changes: 0 additions & 8 deletions toolkit/components/telemetry/histogram-allowlists.json
Original file line number Diff line number Diff line change
Expand Up @@ -316,10 +316,6 @@
"SERVICE_WORKER_WAS_SPAWNED",
"SHOULD_AUTO_DETECT_LANGUAGE",
"SHOULD_TRANSLATION_UI_APPEAR",
"SHUTDOWN_PHASE_DURATION_TICKS_PROFILE_BEFORE_CHANGE",
"SHUTDOWN_PHASE_DURATION_TICKS_PROFILE_CHANGE_TEARDOWN",
"SHUTDOWN_PHASE_DURATION_TICKS_QUIT_APPLICATION",
"SHUTDOWN_PHASE_DURATION_TICKS_XPCOM_WILL_SHUTDOWN",
"SLOW_ADDON_WARNING_RESPONSE_TIME",
"SLOW_ADDON_WARNING_STATES",
"STARTUP_CRASH_DETECTED",
Expand Down Expand Up @@ -792,10 +788,6 @@
"SERVICE_WORKER_WAS_SPAWNED",
"SHOULD_AUTO_DETECT_LANGUAGE",
"SHOULD_TRANSLATION_UI_APPEAR",
"SHUTDOWN_PHASE_DURATION_TICKS_PROFILE_BEFORE_CHANGE",
"SHUTDOWN_PHASE_DURATION_TICKS_PROFILE_CHANGE_TEARDOWN",
"SHUTDOWN_PHASE_DURATION_TICKS_QUIT_APPLICATION",
"SHUTDOWN_PHASE_DURATION_TICKS_XPCOM_WILL_SHUTDOWN",
"SLOW_ADDON_WARNING_RESPONSE_TIME",
"SLOW_ADDON_WARNING_STATES",
"SLOW_SCRIPT_NOTICE_COUNT",
Expand Down
5 changes: 5 additions & 0 deletions toolkit/components/terminator/TerminatorTelemetry.jsm
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,16 @@ function nsTerminatorTelemetry() {}

var HISTOGRAMS = {
"quit-application": "SHUTDOWN_PHASE_DURATION_TICKS_QUIT_APPLICATION",
"profile-change-net-teardown":
"SHUTDOWN_PHASE_DURATION_TICKS_PROFILE_CHANGE_NET_TEARDOWN",
"profile-change-teardown":
"SHUTDOWN_PHASE_DURATION_TICKS_PROFILE_CHANGE_TEARDOWN",
"profile-before-change":
"SHUTDOWN_PHASE_DURATION_TICKS_PROFILE_BEFORE_CHANGE",
"profile-before-change-qm":
"SHUTDOWN_PHASE_DURATION_TICKS_PROFILE_BEFORE_CHANGE_QM",
"xpcom-will-shutdown": "SHUTDOWN_PHASE_DURATION_TICKS_XPCOM_WILL_SHUTDOWN",
"xpcom-shutdown": "SHUTDOWN_PHASE_DURATION_TICKS_XPCOM_SHUTDOWN",
};

nsTerminatorTelemetry.prototype = {
Expand Down
Loading

0 comments on commit 02aaf67

Please sign in to comment.