1811195 - Investigate possible shutdown bail outs on AppShutdownConfirmed during browser process creation requests

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, task, P3)

Description

[Jens Stutte [:jstutte]]

There seems to be a time window between AppShutdownConfirmed and AppShutdown where we already notified existing processes that they will shutdown but continue to ask for new processes from events still in flight (and explicitly enforced to be processed before we advance the phase).

Bug 1810666 avoids that those requests will try to recycle existing processes that have been already slated for removal, but that means we will now try to create new processes that are immediately torn down again. Simply anticipating the existing checks inside BeginSubprocessLaunch and CreateBrowser to AppShutdownConfirmed however seemed to create some fallout that needs investigation.

We might want to verify if we can better add some shutdown check at a higher level of the stack, like DocShell creation or ChangeRemoteness execution.

Comment 1

[Jens Stutte [:jstutte]]

AsyncFrameInit might also be interesting here.

Comment 2

[Jens Stutte [:jstutte]]

Attachment: Bug 1811195 - Bail out on AppShutdownConfirmed in nsFrameLoader::TryRemoteBrowser. r?smaug

Comment 3

[Jens Stutte [:jstutte]]

Attachment: Bug 1811195 - Bail out on AppShutdownConfirmed in DocumentLoadListener::OnStartRequest. r?smaug

Depends on D167394

Comment 4

[Jens Stutte [:jstutte]]

Attachment: Bug 1811195 - Cleanup late process creation checks. r?smaug

Depends on D167395

Comment 5

[Jens Stutte [:jstutte]]

Looking at the call paths to BeginSubprocessLaunch we should probably have regular checks also at:

• SharedWorkerManager::MaybeCreateRemoteWorker and BackgroundParentImpl::AllocPRemoteWorkerControllerParent probably better catched inside RemoteWorkerController::Create
• ContentParent::RecvCloneDocumentTreeInto
• nsFrameLoader::EnsureRemoteBrowser and/or some of its callers
• the PreallocatedProcessManager should stop to serve processes, too, which has been already addressed by bug 1795964.

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

These checks can probably go away (leaving just the assert in AssertAlive):

• ContentParent::BeginSubprocessLaunch
• ContentParent::CreateBrowser

Remains the question if ContentParent::GetNewOrUsedLaunchingBrowserProcess should bail out, too, or just rely on the callers (and the AssertAlive). Note that there is a shortcut that hands out a used process there, too, and bug 1811746 shows this happens in the wild.

Comment 6

[Jens Stutte [:jstutte]]

(In reply to Jens Stutte [:jstutte] from comment #5)

Remains the question if ContentParent::GetNewOrUsedLaunchingBrowserProcess should bail out, too, or just rely on the callers (and the AssertAlive). Note that there is a shortcut that hands out a used process there, too, and bug 1811746 shows this happens in the wild.

I think we should have an explicit assert for !IsInOrBeyond(AppShutdownConfirmed) here, as AssertAlive just checks for !mIsSignaledImpendingShutdown which may or may not have happened (actually it happens on AppShutdownConfirmed for all existing processes, but the order of how shutdown blockers are executed might be random).

Comment 7

[Jens Stutte [:jstutte]]

Attachment: Bug 1811195 - Bail out on AppShutdownConfirmed in SharedWorkerManager::MaybeCreateRemoteWorker and BackgroundParentImpl::AllocPRemoteWorkerControllerParent r?#dom-worker-reviewers

Depends on D167395

Comment 8

[Jens Stutte [:jstutte]]

Attachment: Bug 1811195 - Bail out on AppShutdownConfirmed in ContentParent::RecvCloneDocumentTreeInto r?smaug

Depends on D167565

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b112db2a47e5
Bail out on AppShutdownConfirmed in nsFrameLoader::TryRemoteBrowser. r=smaug
https://hg.mozilla.org/integration/autoland/rev/df20721143f6
Bail out on AppShutdownConfirmed in DocumentLoadListener::OnStartRequest. r=smaug,necko-reviewers,nika,valentin
https://hg.mozilla.org/integration/autoland/rev/3a8153d6d830
Bail out on AppShutdownConfirmed in SharedWorkerManager::MaybeCreateRemoteWorker and BackgroundParentImpl::AllocPRemoteWorkerControllerParent r=dom-worker-reviewers,smaug
https://hg.mozilla.org/integration/autoland/rev/f396ec232f8d
Bail out on AppShutdownConfirmed in ContentParent::RecvCloneDocumentTreeInto r=smaug
https://hg.mozilla.org/integration/autoland/rev/fa5549b8b07f
Cleanup late process creation checks. r=smaug

Comment 10

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/b112db2a47e5
https://hg.mozilla.org/mozilla-central/rev/df20721143f6
https://hg.mozilla.org/mozilla-central/rev/3a8153d6d830
https://hg.mozilla.org/mozilla-central/rev/f396ec232f8d
https://hg.mozilla.org/mozilla-central/rev/fa5549b8b07f