REGRESSION(r268097): Calling processDidTerminate delegate asynchronously is risky compatibility-wise

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Calling processDidTerminate delegate asynchronously like we did in r268097 is risky compatibility-wise. This caused breakage in at least 2 client applications. While this can be dealt with on the client side, it would be better to fix what r268097 was trying to fix without making the delegate call asynchronous. The reason calling the delegate asynchronously is risky is because some view state may have time to get reset by the time the client gets notified on the crash, potentially confusing the crash handling logic in the client.

[Chris Dumez]

Created attachment 420567 [details]
Patch

[Luming Yin]

<rdar://problem/74074861>

[Carlos Garcia Campos]

Comment on attachment 420567 [details]
Patch

Thanks!

[EWS]

commit-queue failed to commit attachment 420567 [details] to WebKit repository. To retry, please set cq+ flag again.

[EWS]

Committed r272990: <https://commits.webkit.org/r272990>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420567 [details].

[Michael Catanzaro]

Hi Chris, it looks like this didn't actually fix Epiphany. The problem is here in WebProcessProxy::processDidTerminateOrFailedToLaunch:

    for (auto& page : pages)
        page->resetStateAfterProcessTermination(reason);

    for (auto& provisionalPage : provisionalPages) {
        if (provisionalPage)
            provisionalPage->processDidTerminate();
    }

    for (auto& page : pages)
        page->dispatchProcessDidTerminate(reason);

WebPageProxy::resetStateAfterProcessTermination calls WebPageProxy::resetStateAfterProcessExited, which resets its m_pageLoadState, here:

    if (terminationReason != ProcessTerminationReason::NavigationSwap) {
        PageLoadState::Transaction transaction = m_pageLoadState.transaction();
        m_pageLoadState.reset(transaction);
    }

That clears the page's URI, and as you see above it happens just before WebProcessProxy calls WebPageProxy::dispatchProcessDidTerminate. To avoid this regression, we would need to ensure the page's URI doesn't get reset until *after* WebPageProxy::dispatchProcessDidTerminate.

Again, easy to workaround in the client, but would be nice if we didn't need to. How bad would it be to move the code that resets the PageLoadState to a later point?

[Chris Dumez]

(In reply to Michael Catanzaro from comment #6)
> Hi Chris, it looks like this didn't actually fix Epiphany. The problem is
> here in WebProcessProxy::processDidTerminateOrFailedToLaunch:
> 
>     for (auto& page : pages)
>         page->resetStateAfterProcessTermination(reason);
> 
>     for (auto& provisionalPage : provisionalPages) {
>         if (provisionalPage)
>             provisionalPage->processDidTerminate();
>     }
> 
>     for (auto& page : pages)
>         page->dispatchProcessDidTerminate(reason);
> 
> WebPageProxy::resetStateAfterProcessTermination calls
> WebPageProxy::resetStateAfterProcessExited, which resets its
> m_pageLoadState, here:
> 
>     if (terminationReason != ProcessTerminationReason::NavigationSwap) {
>         PageLoadState::Transaction transaction =
> m_pageLoadState.transaction();
>         m_pageLoadState.reset(transaction);
>     }
> 
> That clears the page's URI, and as you see above it happens just before
> WebProcessProxy calls WebPageProxy::dispatchProcessDidTerminate. To avoid
> this regression, we would need to ensure the page's URI doesn't get reset
> until *after* WebPageProxy::dispatchProcessDidTerminate.
> 
> Again, easy to workaround in the client, but would be nice if we didn't need
> to. How bad would it be to move the code that resets the PageLoadState to a
> later point?

I don't understand why this is a regression from my original change then though. We always called resetStateAfterProcessExited() *before* notifying the client.

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> (In reply to Michael Catanzaro from comment #6)
> > Hi Chris, it looks like this didn't actually fix Epiphany. The problem is
> > here in WebProcessProxy::processDidTerminateOrFailedToLaunch:
> > 
> >     for (auto& page : pages)
> >         page->resetStateAfterProcessTermination(reason);
> > 
> >     for (auto& provisionalPage : provisionalPages) {
> >         if (provisionalPage)
> >             provisionalPage->processDidTerminate();
> >     }
> > 
> >     for (auto& page : pages)
> >         page->dispatchProcessDidTerminate(reason);
> > 
> > WebPageProxy::resetStateAfterProcessTermination calls
> > WebPageProxy::resetStateAfterProcessExited, which resets its
> > m_pageLoadState, here:
> > 
> >     if (terminationReason != ProcessTerminationReason::NavigationSwap) {
> >         PageLoadState::Transaction transaction =
> > m_pageLoadState.transaction();
> >         m_pageLoadState.reset(transaction);
> >     }
> > 
> > That clears the page's URI, and as you see above it happens just before
> > WebProcessProxy calls WebPageProxy::dispatchProcessDidTerminate. To avoid
> > this regression, we would need to ensure the page's URI doesn't get reset
> > until *after* WebPageProxy::dispatchProcessDidTerminate.
> > 
> > Again, easy to workaround in the client, but would be nice if we didn't need
> > to. How bad would it be to move the code that resets the PageLoadState to a
> > later point?
> 
> I don't understand why this is a regression from my original change then
> though. We always called resetStateAfterProcessExited() *before* notifying
> the client.

Oh, I think it is because there was this in scope:
PageLoadState::Transaction transaction = m_pageLoadState.transaction();

So even though we cleared the URL before call the client, the change would not take effect until after the transaction gets destroyed, which was *after* we called the client.. So it seems I indeed changed behavior here :/

[Michael Catanzaro]

Chris posted a fix in bug #222809.