1811746 - Crash in [@ mozilla::dom::ContentParent::AssertAlive]

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P2)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/2227b679-4683-40ee-95b8-7cdb70230121

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(!mIsSignaledImpendingShutdown)

Top 10 frames of crashing thread:

0  xul.dll  mozilla::dom::ContentParent::AssertAlive  dom/ipc/ContentParent.cpp:1961
1  xul.dll  mozilla::dom::ContentParent::GetNewOrUsedLaunchingBrowserProcess  dom/ipc/ContentParent.cpp:1063
2  xul.dll  mozilla::dom::CanonicalBrowsingContext::ChangeRemoteness  docshell/base/CanonicalBrowsingContext.cpp:2072
3  xul.dll  mozilla::net::DocumentLoadListener::TriggerProcessSwitch  netwerk/ipc/DocumentLoadListener.cpp:1979
4  xul.dll  mozilla::net::DocumentLoadListener::MaybeTriggerProcessSwitch  netwerk/ipc/DocumentLoadListener.cpp:1907
4  xul.dll  mozilla::net::DocumentLoadListener::OnStartRequest  netwerk/ipc/DocumentLoadListener.cpp:2503
5  xul.dll  mozilla::net::ParentChannelListener::OnStartRequest  netwerk/protocol/http/ParentChannelListener.cpp:86
6  xul.dll  nsDocumentOpenInfo::OnStartRequest  uriloader/base/nsURILoader.cpp:167
7  xul.dll  mozilla::net::ParentProcessDocumentOpenInfo::OnDocumentStartRequest  netwerk/ipc/DocumentLoadListener.cpp:297
7  xul.dll  mozilla::net::ParentProcessDocumentOpenInfo::OnStartRequest  netwerk/ipc/DocumentLoadListener.cpp:345

The earliest affected buildid is 20230120212103. Only one crash report has a comment that says:

restart after NIGHTLY update

Comment 1

[Jens Stutte [:jstutte]]

These crashes come through DocumentLoadListener::OnStartRequest and will hopefully be addressed by bug 1811195. At least I am pretty sure that there should be no other edge case left than parent shutdown here. But I'll not dupe it until we'll see it go away.

There remains also the question, where are good places to put all the IsInOrBeyond checks. We currently have them at:

• ContentParent::BeginSubprocessLaunch but only in or beyond AppShutdown
• ContentParent::CreateBrowser but only in or beyond AppShutdown

and bug 1811195 wants to add them at

• DocumentLoadListener::OnStartRequest which will probably help here
• AsyncFrameInit::Run which can always be in flight when we want to start shutdown

which would make the first two kind of obsolete. But I overlooked that ContentParent::GetNewOrUsedLaunchingBrowserProcess has a shortcut that hands out a used process, too. I think that we should:

• have a check/assert/diagnostic assert and bail out at the beginning of ContentParent::GetNewOrUsedLaunchingBrowserProcess
• have regular bail outs (without assert) at higher level places (like bug 1811195 proposed)
• have only (diagnostic) asserts inside the internal launching functions (via AssertAlive)

Edit: Actually looking at call paths to BeginSubprocessLaunch we should probably have regular checks also at:

• SharedWorkerManager::MaybeCreateRemoteWorker and BackgroundParentImpl::AllocPRemoteWorkerControllerParent
• ContentParent::RecvCloneDocumentTreeInto
• nsFrameLoader::EnsureRemoteBrowser and/or some of its callers
• thePreallocatedProcessManager`should stop to serve processes, too, which has been already addressed by bug 1795964.

I assume that XRE_SendTestShellCommand and nsIXULRuntime::EnsureContentProcess ahould not check anything but just let the launching code assert at lower levels in case.

Edit 2: I moved this aspect to bug 1811195 comment 5 for further analysis.

Comment 2

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

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 desktop browser crashes on nightly

:janv, could you consider increasing the severity of this top-crash bug?

For more information, please visit auto_nag documentation.

Comment 3

[Jens Stutte [:jstutte]]

It seems we can find our process still subscribed to the given BrowsingContextGroup despite having started our content process' shutdown. IIUC, we always will ContentParent::MarkAsDead and thus ContentParent::RemoveFromList where we unsubscribe the process from all mGroups at the same time when we have done SignalImpendingShutdownToContentJS, so this is not really expected, unless there is some misalignment in our book-keeping for BrowsingContextGroups between what the ContentParent knows about it and who else might reference them.

I assume we could just check for !contentParent->IsShuttingDown() here to paint over this here but it feels wrong that those BrowsingContextGroup still refer to an apparently dying process?

Comment 4

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

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

For more information, please visit auto_nag documentation.

Comment 5

[Jens Stutte [:jstutte]]

So the volume went down since bug 1814603 landed and the remaining crashes happen in a different place than comment 3 assumed:

0 	XUL 	mozilla::dom::ContentParent::AssertAlive() 	dom/ipc/ContentParent.cpp:1993 	context
1 	XUL 	mozilla::dom::ContentParent::AddKeepAlive() 	dom/ipc/ContentParent.cpp:2434 	cfi
2 	XUL 	mozilla::dom::CanonicalBrowsingContext::ChangeRemoteness(mozilla::dom::NavigationIsolationOptions const&, unsigned long long) 	docshell/base/CanonicalBrowsingContext.cpp:2063 	cfi
3 	XUL 	mozilla::net::DocumentLoadListener::TriggerProcessSwitch(mozilla::dom::CanonicalBrowsingContext*, mozilla::dom::NavigationIsolationOptions const&, bool) 	netwerk/ipc/DocumentLoadListener.cpp:1994 	cfi
4 	XUL 	mozilla::net::DocumentLoadListener::MaybeTriggerProcessSwitch(bool*) 	netwerk/ipc/DocumentLoadListener.cpp:1922 	inlined
4 	XUL 	mozilla::net::DocumentLoadListener::OnStartRequest(nsIRequest*) 	netwerk/ipc/DocumentLoadListener.cpp:2525 	cfi
5 	XUL 	mozilla::net::ParentChannelListener::OnStartRequest(nsIRequest*) 	netwerk/protocol/http/ParentChannelListener.cpp:86 	cfi
6 	XUL 	nsDocumentOpenInfo::OnStartRequest(nsIRequest*) 	uriloader/base/nsURILoader.cpp:167 	cfi

I think we should just do the same IsShuttingDown check as here also for this content parent ?

Comment 6

[Jens Stutte [:jstutte]]

Attachment: Bug 1811746 - Check embedderBrowser process if IsShuttingDown before re-use. r?smaug

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df3288d7931f
Check embedderBrowser process if IsShuttingDown before re-use. r=smaug

Comment 8

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/df3288d7931f

Comment 10

[Jens Stutte [:jstutte]]

It seems the recent (small) spike started with bug 1728331 landing.