Bug 237917

The segfault is because a doble-call to the WebProcess::terminate() method in the WebProcess::shutdown() path. ``` void WebProcess::terminate() { #ifndef NDEBUG GCController::singleton().garbageCollectNow(); FontCache::singleton().invalidate(); MemoryCache::singleton().setDisabled(true); #endif m_webConnection->invalidate(); <<<<<<<< invalid access during the second invocation m_webConnection = nullptr; <<<<<<<<< set null in the first invocation platformTerminate(); AuxiliaryProcess::terminate(); } ``` Here the stack method calls: AuxiliaryProcess::shutDown(): ``` -> terminate() -> WebProcess::terminate() -> AuxiliaryProcess::terminate() -> AuxiliaryProcess::terminate() -> stopRunLoop() -> WebProcess::stopRunLoop() (from glib/WebProcessGLib.cpp) -> WebPage::close() -> WebProcess::singleton().removeWebPage(m_identifier) -> AuxiliaryProcess::enableTermination() -- m_terminationCounter: 0 -> WebProcess::terminate() ```

Created attachment 454750 [details] patch

With the proposed patch, the new stack could someth WebProcess::stopRunLoop() (from glib/WebProcessGLib.cpp) ``` -> WebPage::close() -> WebProcess::singleton().removeWebPage(m_identifier) -> WebProcess::removeWebPage() -> AuxiliaryProcess::enableTermination() -> WebProcess::terminate() <<<<<<<<<<< Called for each WebProcess associated to each WebPage ... -> WebPage::~WebPage() -> AuxiliaryProcess::stopRunLoop() -> platformStopRunLoop() ```

Created attachment 454756 [details] patch

So many #ifs. I wonder if we can make other ports use the same implementation of AuxiliaryProcess::didClose that we do, where we call stopRunLoop. I know Apple was previously opposed to similar approaches, but I really doubt one run loop iteration should have a major performance impact on process termination speed. That would allow considerably reducing #ifs, and should hopefully make it safer to modify this code without introducing platform-specific problems.

Comment on attachment 454756 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=454756&action=review Notice that this change, even if could be good for avoiding redundant calls to the RunLoop::main().stop(), is not mandatory since it doesn't harms the shutDown logic. > Source/WebKit/Shared/AuxiliaryProcess.cpp:216 > +#if !PLATFORM(GTK) && !PLATFORM(WPE) Notice that this change, even if could be good for avoiding redundant calls to the RunLoop::main().stop(), is not mandatory since it doesn't harms the shutDown logic.

(In reply to Michael Catanzaro from comment #5) > So many #ifs. > > I wonder if we can make other ports use the same implementation of > AuxiliaryProcess::didClose that we do, where we call stopRunLoop. I know > Apple was previously opposed to similar approaches, but I really doubt one > run loop iteration should have a major performance impact on process > termination speed. That would allow considerably reducing #ifs, and should > hopefully make it safer to modify this code without introducing > platform-specific problems. AFAIK, the major concern to move this to the WebProcess.cpp is the loop stop is too slow in some ports. This is the reason why didClose() is implemented with `_exit(EXIT_SUCCESS)`. I don't think that situation changed a lot since the last time this was discussed.

BTW, a simple safe-guard like this also fix the crash: ``` void WebProcess::terminate() { #ifndef NDEBUG GCController::singleton().garbageCollectNow(); FontCache::singleton().invalidate(); MemoryCache::singleton().setDisabled(true); #endif if (m_webConnection) <<<<<<<<<<<<<<< safe-guarded access { m_webConnection->invalidate(); <<<<<<<< invalid access during the second invocation m_webConnection = nullptr; <<<<<<<<< set null in the first invocation } platformTerminate(); AuxiliaryProcess::terminate(); } ``` ; however I don't think the recursive logic followed by the terminate() could be considered strictly correct: ``` -> terminate() -> WebProcess::terminate() -> AuxiliaryProcess::terminate() -> AuxiliaryProcess::terminate() -> stopRunLoop() -> WebProcess::stopRunLoop() (from glib/WebProcessGLib.cpp) -> WebPage::close() -> WebProcess::singleton().removeWebPage(m_identifier) -> AuxiliaryProcess::enableTermination() -- m_terminationCounter: 0 -> WebProcess::terminate() ```

Checking m_webConnection was also my first idea, but returning early instead. However, I would like to understand how this can happen, because I would expect the page map to be empty when shutdown is called.

``` XXX ( thread &1844441728 pid &001) -- WebPageProxy::commitProvisionalPage() -- m_process->removeWebPage() XXX ( thread &1844441728 pid &001) -- WebProcessProxy::removeWebPage() XXX ( thread &1844441728 pid &001) -- WebProcessProxy::maybeShutDown() XXX ( thread &1844441728 pid &001) -- WebProcessProxy::canTerminateAuxiliaryProcess() [1] XXX WebProcessProxy::canTerminateAuxiliaryProcess: (pageCount=0, provisionalPageCount=0, m_suspendedPageCount=0, m_isInProcessCache=0, m_shutdownPreventingScopeCounter=0) XXX WebProcessProxy::canTerminateAuxiliaryProcess: true XXX ( thread &1844441728 pid &001) -- WebProcessProxy::maybeShutDown() -> shutDown() XXX ( thread &1844441728 pid &001) -- WebProcessProxy::shutDown() XXX ( thread &1887639264 pid &111) -- AuxiliaryProcess::shutDown() -> terminate() -- m_terminationCounter: 1 XXX ( thread &1887639264 pid &111) -- WebProcess::terminate() -- BEGIN -- this (&0x6fff6000) XXX ( thread &1887639264 pid &111) -- WebProcess::terminate() -- 1 -- this (&0x6fff6000) XXX ( thread &1887639264 pid &111) -- WebProcess::terminate() -- 1.1 -- this (&0x6fff6000) XXX ( thread &1887639264 pid &111) -- WebProcess::terminate() -- 2 -- this (&0x6fff6000) XXX ( thread &1887639264 pid &111) -- WebProcess::terminate() -- 3 -- this (&0x6fff6000) XXX ( thread &1887639264 pid &111) -- glib/WebProcessGLib.cpp -- WebProcess::stopRunLoop() [2] XXX ( thread &1887639264 pid &111) -- WebPage::close() --> This will lead in a 2nd terminate() czll ``` notice that: * in [1] pageCount is WebProcessProxy.m_pageMap.size() -> 0. * in [2] WebPage::close() is called because the m_WebProcess.pageMap is not empty At that point it looks like there is a desynchrony between the WebProcessState state and the WebProcess state

(In reply to Carlos Garcia Campos from comment #9) > Checking m_webConnection was also my first idea, but returning early > instead. However, I would like to understand how this can happen, because I > would expect the page map to be empty when shutdown is called. Notice that a similar decision was taken in the WebProcessProxy: ``` void WebProcessProxy::shutDown() { ... if (m_webConnection) { m_webConnection->invalidate(); m_webConnection = nullptr; } ```

Coming back to the comment:10 This change could prevent the m_pageMap being not empty by calling for the page close() before the page being removed from the WebProcessProxy map: ``` diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp index a7e19d46..745559a3 100644 --- a/Source/WebKit/UIProcess/WebPageProxy.cpp +++ b/Source/WebKit/UIProcess/WebPageProxy.cpp @@ -3607,6 +3615,11 @@ void WebPageProxy::commitProvisionalPage(FrameIdentifier frameID, FrameInfoData& m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_webPageID); auto* navigation = navigationState().navigation(m_provisionalPage->navigationID()); bool didSuspendPreviousPage = navigation && !m_provisionalPage->shouldClosePreviousPageAfterCommit() ? suspendCurrentPageIfPossible(*navigation, mainFrameIDInPreviousProcess, m_provisionalPage->processSwapRequestedByClient(), shouldDelayClosingUntilFirstLayerFlush) : false; + + RunLoop::current().dispatch([destinationID = messageSenderDestinationID(), protectedProcess = m_process.copyRef(), preventProcessShutdownScope = m_process->shutdownPreventingScope()] { + protectedProcess->send(Messages::WebPage::Close(), destinationID); + }); + m_process->removeWebPage(*this, m_websiteDataStore.ptr() == &m_provisionalPage->process().websiteDataStore() ? WebProcessProxy::EndsUsingDataStore::No : WebProcessProxy::EndsUsingDataStore::Yes); // There is no way we'll be able to return to the page in the previous page so close it. ``` Resulting in this workflow: ``` XXX ( layerFlush thread &1887233760) -- CoordinatedGraphicsLayer::~CoordinatedGraphicsLayer() -- END XXX ( ??? thread &1844441728 pid &1) -- WebProcessProxy::WebProcessProxy() -- m_shutdownPreventingScopeCounter XXX ( ??? thread &1844441728 pid &1) -- WebPageProxy::commitProvisionalPage() -- m_process->removeWebPage() XXX ( ??? thread &1844441728 pid &1) -- WebProcessProxy::removeWebPage() XXX ( ??? thread &1844441728 pid &1) -- WebProcessProxy::maybeShutDown() XXX ( ??? thread &1844441728 pid &1) -- WebProcessProxy::canTerminateAuxiliaryProcess() XXX WebProcessProxy::canTerminateAuxiliaryProcess: (pageCount=0, provisionalPageCount=0, m_suspendedPageCount=0, m_isInProcessCache=0, m_shutdownPreventingScopeCounter=1) XXX ( ??? thread &1844441728 pid &1) -- WebProcessProxy::addExistingWebPage() XXX ( ??? thread &1886918368 pid &168) -- WebPage::close() <<<<<<<<<<< [1] XXX ( ??? thread &1844441728 pid &1) -- WebProcessProxy::removeProvisionalPageProxy() XXX ( ??? thread &1844441728 pid &1) -- WebProcessProxy::maybeShutDown() XXX ( ??? thread &1844441728 pid &1) -- WebProcessProxy::canTerminateAuxiliaryProcess() XXX WebProcessProxy::canTerminateAuxiliaryProcess: (pageCount=1, provisionalPageCount=0, m_suspendedPageCount=0, m_isInProcessCache=0, m_shutdownPreventingScopeCounter=0) ... XXX ( ??? thread &1886918368 pid &168) -- WebPage::close() -> WebProcess::singleton().removeWebPage(m_identifier) XXX (? t:&1886918368 p:&:168) -- WebProcess::removeWebPage() -- BEGIN -- this (&0x6fef6000) XXX ( ??? thread &1886918368 pid &168) -- AuxiliaryProcess::enableTermination() -- m_terminationCounter: 0 XXX (? t:&1886918368 p:&:168) -- AuxiliaryProcess::terminationTimerFired()() (&0x6fef6000) XXX (? t:&1886918368 p:&:168) -- WebProcess::shouldTerminate() -- BEGIN -- this (&0x6fef6000) XXX ( ??? thread &1844441728 pid &1) -- WebProcessProxy::WebProcessProxy() -- m_shutdownPreventingScopeCounter XXX ( ??? thread &1844441728 pid &1) -- WebProcessProxy::maybeShutDown() XXX ( ??? thread &1844441728 pid &1) -- WebProcessProxy::canTerminateAuxiliaryProcess() XXX WebProcessProxy::canTerminateAuxiliaryProcess: (pageCount=0, provisionalPageCount=0, m_suspendedPageCount=0, m_isInProcessCache=0, m_shutdownPreventingScopeCounter=0) XXX WebProcessProxy::canTerminateAuxiliaryProcess: true XXX ( ??? thread &1844441728 pid &1) -- WebProcessProxy::maybeShutDown() -> shutDown() XXX ( ??? thread &1844441728 pid &1) -- WebProcessProxy::shutDown() XXX (? t:&1886918368 p:&:168) -- WebProcess::shouldTerminate() -- END -- true -- this (&0x6fef6000) XXX (? t:&1886918368 p:&:168) -- WebProcess::terminate() -- BEGIN -- this (&0x6fef6000) XXX ( ??? thread &1886918368 pid &168) -- glib/WebProcessGLib.cpp -- WebProcess::stopRunLoop() (no WebPage.close() because it was alrady called in [1] before the shutdown) ... ``` ; apparently more logically right.

Created attachment 454958 [details] Patch

(In reply to Carlos Garcia Campos from comment #13) > Created attachment 454958 [details] > Patch Yes, this patch also works. Moreover, this avoids shouldTerminate() being called and this again the shutdown() one more time. As, Carlos explained to me, in case of shutdown, we want to inconditionally terminate the WebProcess. We dont want ask to the UI-process if we should do it because it is the UI-process itself who is shutting down the WebProcess. This patch is clear r+ for me. Thanks Carlos.

Comment on attachment 454958 [details] Patch I like that you managed this in a non-intrusive and cross-platform way. Needs owner approval, though.

Committed r291475 (248591@trunk): <https://commits.webkit.org/248591@trunk>