Bug 121970 – [WK2] Does not remove page from WebProcessProxy's map upon WebProcess termination

• Component: WebKit2
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

[WK2] Does not remove page from WebProcessProxy's map on WebProcess

[Gustavo Noronha (kov)]

Created attachment 212738 [details]
Patch

[Gustavo Noronha (kov)]

A couple comments: I couldn't figure out why Mac doesn't crash - I guess the page proxy is not invalidated before it's destroyed? The test that is crashing requiring this fix is TestWebKit2's WebKit2.WillSendSubmitEvent, but the test that leads to the state that causes the crash is WebKit2.TerminateTwice.

[Brian Holt]

Comment on attachment 212738 [details]
Patch

Informal review: looks good to me.

[Anders Carlsson]

Comment on attachment 212738 [details]
Patch

I don’t think this is the right fix.

[Gustavo Noronha (kov)]

Thanks for the review! I'll try to get a better understanding of the problem.

Let me explain how I got to this, maybe someone can provide some insight: the crash happens in the test that comes right after WebKit2.TerminateTwice, the second terminate in that test happens while the process is still starting up and in that case things are not getting cleaned up properly it seems.

When the test that comes after terminate twice starts the web process and is going to start a new connection the WebProcessProxy tells all pages in its map that the connection will be opened. The page that exists when that second terminate happened is still in the map even though it's been destroyed becase WebPageProxy::resetStateAfterProcessExited() is called before WebPageProxy::close(). The former invalidates the page by setting m_isValid to false but does not tell the WebProcessProxy. When we get to the latter (which does remove the page from the process proxy) we get the early return - http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/WebPageProxy.cpp#L543.

FWIW, I split the run-gtk-tests change to bug 122135 since it's independent and generally useful without this fix.

[Gustavo Noronha (kov)]

Created attachment 213185 [details]
Patch

How about something like this? The interaction between m_isClosed and isValid() seems pretty complex, I cannot see how the page would get unregistered from the map regardless of the case, though, if the web process has gone away at least once (it is not just in the not done starting case it turns out).

[Gustavo Noronha (kov)]

Committed r157072: <http://trac.webkit.org/changeset/157072>

[Gustavo Noronha (kov)]

Err, sorry, I mentioned this bug in the changelog entry and the script misunderstood my intention, this was not landed.

[Zan Dobersek]

I'm not able to reproduce the crash in WebKit2.TerminateTwice.

Gustavo, can you please check that the test passes for you as well?

[Carlos Alberto Lopez Perez]

I'm also not longer able to reproduce this. I think we can unskip the test WebKit2.TerminateTwice

[Darin Adler]

Comment on attachment 213185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213185&action=review

> Source/WebKit2/UIProcess/WebPageProxy.cpp:364
> +        // A page may not be closed but be invalid because the WebProcess has exited; in that
> +        // case close() will simply return, leaving the page registered on WebProcessProxy's map,
> +        // so we remove it here.
> +        if (!isValid())
> +            m_process->removeWebPage(m_pageID);
> +        else
> +            close();

The right place to fix this is in WebPageProxy::resetStateAfterProcessExited, not here.

[Darin Adler]

Comment on attachment 213185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213185&action=review

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:364
>> +            close();
> 
> The right place to fix this is in WebPageProxy::resetStateAfterProcessExited, not here.

And that’s what the original patch did. I am unclear on why this patch exists. Leaving this patch as review- for now.

[Alexey Proskuryakov]

I don't see why the page should be removed from WebProcessProxy's map upon process termination. The page is still associated with the terminated process at this point.

Anyway, is this still an issue after bug 135996?

[Gustavo Noronha (kov)]

Does not seem to be a problem after 1135996, I think we can go ahead and close it, thanks!