Bug 121970

Summary: [WK2] Does not remove page from WebProcessProxy's map upon WebProcess termination
Product: WebKit Reporter: Gustavo Noronha (kov) <gns>
Component: WebKit2Assignee: Gustavo Noronha (kov) <gns>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, clopez, hugo.lima, kling, ryuan.choi, sam, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch darin: review-

Description Gustavo Noronha (kov) 2013-09-26 12:25:06 PDT
[WK2] Does not remove page from WebProcessProxy's map on WebProcess
Comment 1 Gustavo Noronha (kov) 2013-09-26 12:32:54 PDT
Created attachment 212738 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2013-09-26 12:34:29 PDT
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.
Comment 3 Brian Holt 2013-09-30 10:24:11 PDT
Comment on attachment 212738 [details]
Patch

Informal review: looks good to me.
Comment 4 Anders Carlsson 2013-09-30 11:49:56 PDT
Comment on attachment 212738 [details]
Patch

I don’t think this is the right fix.
Comment 5 Gustavo Noronha (kov) 2013-09-30 16:47:29 PDT
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.
Comment 6 Gustavo Noronha (kov) 2013-10-02 12:33:29 PDT
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).
Comment 7 Gustavo Noronha (kov) 2013-10-07 18:20:19 PDT
Committed r157072: <http://trac.webkit.org/changeset/157072>
Comment 8 Gustavo Noronha (kov) 2013-10-07 18:46:01 PDT
Err, sorry, I mentioned this bug in the changelog entry and the script misunderstood my intention, this was not landed.
Comment 9 Zan Dobersek 2013-12-02 01:52:32 PST
I'm not able to reproduce the crash in WebKit2.TerminateTwice.

Gustavo, can you please check that the test passes for you as well?
Comment 10 Carlos Alberto Lopez Perez 2014-04-16 08:52:47 PDT
I'm also not longer able to reproduce this. I think we can unskip the test WebKit2.TerminateTwice
Comment 11 Darin Adler 2014-08-19 09:26:55 PDT
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.
Comment 12 Darin Adler 2014-08-19 09:27:53 PDT
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.
Comment 13 Alexey Proskuryakov 2014-08-19 10:25:26 PDT
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?
Comment 14 Gustavo Noronha (kov) 2014-08-28 04:52:22 PDT
Does not seem to be a problem after 1135996, I think we can go ahead and close it, thanks!