[Webkit-unassigned] [Bug 110743] [WK2] WebPageProxy loadURL() won't work when called just after terminateProcess()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=110743





--- Comment #15 from Adenilson Cavalcanti Silva <savagobr at yahoo.com>  2013-04-09 11:22:39 PST ---
Benjamin

Thanks for reviewing this patch. I will try to address the comments bellow.

> I think the changelog should be clearer. You are fixing a very specific >issue. You first state what was the bug, then explain why it happened, then >you put the text explaining how you fixed the problem.
> 

Concerning the CL... agreed. The fix is one thing (i.e. load fails after call to terminate) and the points of failure are 2:
a) Cleaning up of WebPageProxy
b) Reattach executing *before* complete WebProcessProxy cleanup.

I will upload a new version of this patch with a hopefully better explanation in the CL. 

> > Source/WebKit2/UIProcess/WebPageProxy.cpp:1571
> > +    m_process->userRequestedTerminate();
> > +    processTeardown();
> 
> Why are those called in this order?
> 

Executing WebProcess termination must be done *before* any cleanup of WebPageProxy. If you think about it, in normal crashes, this is exactly what has happened already.

This ensures that reattach/respawn of WebProcess will succeed, since when we return to WebPageProxy, WebProcessProxy is in a well defined state (and not in the middle of its own cleanup).

> 
> When does that happen?
>     1) terminateProcess()
>     2) processTeardown()
>     3) processDidCrash()

The previous behavior was to execute processDidCrash(), called from WebProcessProxy.

With this patch, using the new member function 'userRequestedTerminate', WebProcessProxy will *not* callback any method into WebPageProxy. This includes processDidCrash().

So, the execution flow for user requested termination was:

WebPageProxy................ WebProcessProxy
terminateProcess() --------->  processTerminate()
processDidCrash() <----------  DidClose()


*Now* the execution flow for this case is:

WebPageProxy................ WebProcessProxy
terminateProcess() --------->  userRequestedTerminate() //kill everything
processTearDown()

For normal crashes (i.e. not user requested termination), the execution flow hasn't changed. WebProcessProxy will call processDidCrash() into WebPageProxy which will then notify the client/port of the crash. 

Another thing that this patch changes is the ordering of steps for normal crashes. Before it was: notify the client/port of a crash and next try to do the cleanup.

Now it is: first do the cleanup, next notify the client/port of a crash. This helps to ensure that WebPageProxy is in a defined state if someone calls its member functions (e.g. loadURL()). In those cases, since the cleanup hasn't executed yet, the test for WebProcess state would fail (and reattach is never executed).

> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:3810
> > +    // After removing the message receivers and cleaning up resources, we
> > +    // notify the client/port that a crash happened. This ensures
> > +    // that we are in a defined state and calls to loadURL() or reload()
> > +    // should work.
> 
> I don't think this comment is necessary.
> 

Ack.


> > Source/WebKit2/UIProcess/WebPageProxy.h:604
> > +    void processTeardown();
> 
> This should be private.
> 

Agreed.

> > Source/WebKit2/UIProcess/WebProcessProxy.h:119
> > +    void userRequestedTerminate();
> > +
> 
> The name should not have "user", as it is a callback for any programmatic termination. Not only the one triggered by the user.
> 

Isn't this executed when the user/client code call WKPageTerminate()?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.