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`](https://searchfox.org/mozilla-central/rev/270f0ed2146821e239232728adac41a3de41131b/dom/ipc/ContentParent.cpp#2594) but only in or beyond `AppShutdown` - [`ContentParent::CreateBrowser`](https://searchfox.org/mozilla-central/rev/270f0ed2146821e239232728adac41a3de41131b/dom/ipc/ContentParent.cpp#1469) 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](https://searchfox.org/mozilla-central/rev/270f0ed2146821e239232728adac41a3de41131b/dom/ipc/ContentParent.cpp#1056-1067), 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`)
Bug 1811746 Comment 1 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
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`](https://searchfox.org/mozilla-central/rev/270f0ed2146821e239232728adac41a3de41131b/dom/ipc/ContentParent.cpp#2594) but only in or beyond `AppShutdown` - [`ContentParent::CreateBrowser`](https://searchfox.org/mozilla-central/rev/270f0ed2146821e239232728adac41a3de41131b/dom/ipc/ContentParent.cpp#1469) 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](https://searchfox.org/mozilla-central/rev/270f0ed2146821e239232728adac41a3de41131b/dom/ipc/ContentParent.cpp#1056-1067), 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`](https://searchfox.org/mozilla-central/query/default?q=calls-to%3A%27mozilla%3A%3Adom%3A%3AContentParent%3A%3ABeginSubprocessLaunch%27+depth%3A5) we should probably have regular checks also at: - `SharedWorkerManager::MaybeCreateRemoteWorker` and `BackgroundParentImpl::AllocPRemoteWorkerControllerParent`` - `ContentParent::RecvCloneDocumentTreeInto` - `nsFrameLoader::EnsureRemoteBrowser` and/or [some of its callers](https://searchfox.org/mozilla-central/query/default?q=calls-to%3A%27nsFrameLoader%3A%3AEnsureRemoteBrowser%27%20depth%3A4) - `the `PreallocatedProcessManager`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.
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`](https://searchfox.org/mozilla-central/rev/270f0ed2146821e239232728adac41a3de41131b/dom/ipc/ContentParent.cpp#2594) but only in or beyond `AppShutdown` - [`ContentParent::CreateBrowser`](https://searchfox.org/mozilla-central/rev/270f0ed2146821e239232728adac41a3de41131b/dom/ipc/ContentParent.cpp#1469) 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](https://searchfox.org/mozilla-central/rev/270f0ed2146821e239232728adac41a3de41131b/dom/ipc/ContentParent.cpp#1056-1067), 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`](https://searchfox.org/mozilla-central/query/default?q=calls-to%3A%27mozilla%3A%3Adom%3A%3AContentParent%3A%3ABeginSubprocessLaunch%27+depth%3A5) we should probably have regular checks also at: - `SharedWorkerManager::MaybeCreateRemoteWorker` and `BackgroundParentImpl::AllocPRemoteWorkerControllerParent` - `ContentParent::RecvCloneDocumentTreeInto` - `nsFrameLoader::EnsureRemoteBrowser` and/or [some of its callers](https://searchfox.org/mozilla-central/query/default?q=calls-to%3A%27nsFrameLoader%3A%3AEnsureRemoteBrowser%27%20depth%3A4) - `the `PreallocatedProcessManager`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.
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`](https://searchfox.org/mozilla-central/rev/270f0ed2146821e239232728adac41a3de41131b/dom/ipc/ContentParent.cpp#2594) but only in or beyond `AppShutdown` - [`ContentParent::CreateBrowser`](https://searchfox.org/mozilla-central/rev/270f0ed2146821e239232728adac41a3de41131b/dom/ipc/ContentParent.cpp#1469) 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](https://searchfox.org/mozilla-central/rev/270f0ed2146821e239232728adac41a3de41131b/dom/ipc/ContentParent.cpp#1056-1067), 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`](https://searchfox.org/mozilla-central/query/default?q=calls-to%3A%27mozilla%3A%3Adom%3A%3AContentParent%3A%3ABeginSubprocessLaunch%27+depth%3A5) we should probably have regular checks also at: - `SharedWorkerManager::MaybeCreateRemoteWorker` and `BackgroundParentImpl::AllocPRemoteWorkerControllerParent` - `ContentParent::RecvCloneDocumentTreeInto` - `nsFrameLoader::EnsureRemoteBrowser` and/or [some of its callers](https://searchfox.org/mozilla-central/query/default?q=calls-to%3A%27nsFrameLoader%3A%3AEnsureRemoteBrowser%27%20depth%3A4) - `the `PreallocatedProcessManager`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.