Investigate possible shutdown bail outs on AppShutdownConfirmed during browser process creation requests
Categories
(Core :: DOM: Content Processes, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox111 | --- | fixed |
People
(Reporter: jstutte, Assigned: jstutte)
References
Details
Attachments
(5 files)
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.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 1•2 years ago
|
||
AsyncFrameInit
might also be interesting here.
Assignee | ||
Comment 2•2 years ago
|
||
Updated•2 years ago
|
Assignee | ||
Comment 3•2 years ago
|
||
Depends on D167394
Assignee | ||
Comment 4•2 years ago
|
||
Depends on D167395
Assignee | ||
Comment 5•2 years ago
•
|
||
Looking at the call paths to BeginSubprocessLaunch
we should probably have regular checks also at:
probably better catched insideSharedWorkerManager::MaybeCreateRemoteWorker
andBackgroundParentImpl::AllocPRemoteWorkerControllerParent
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
):
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.
Assignee | ||
Comment 6•2 years ago
•
|
||
(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 theAssertAlive
). 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).
Updated•2 years ago
|
Assignee | ||
Comment 7•2 years ago
|
||
Depends on D167395
Assignee | ||
Comment 8•2 years ago
|
||
Depends on D167565
Updated•2 years ago
|
Updated•2 years ago
|
Comment 10•2 years ago
|
||
bugherder |
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
Description
•