[webkit-reviews] review denied: [Bug 110743] [WK2] WebPageProxy loadURL() won't work when called just after terminateProcess() : [Attachment 196945] Fixed failing unit test

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Adenilson Cavalcanti Silva
<savagobr at yahoo.com>'s request for review:
Bug 110743: [WK2] WebPageProxy loadURL() won't work when called just after
terminateProcess()
https://bugs.webkit.org/show_bug.cgi?id=110743

Attachment 196945: Fixed failing unit test
https://bugs.webkit.org/attachment.cgi?id=196945&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=196945&action=review


> Source/WebKit2/ChangeLog:14
> +	   WebPageProxy should do its resources clean up as soon as executing
the call to terminate.
> +
> +	   Its state will be undefined if depending on the execution of
processDidCrash(), thus this
> +	   patch moves the cleanup code to a shared private function that is
used for both the cases
> +	   i.e. user termination and real crash. WebProcess shutdown is done
using a new method that
> +	   ensures that all cleanup was done before returning. Finally, for
user requested termination,
> +	   clients are no longer notified of crash.

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.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1571
> +    m_process->userRequestedTerminate();
> +    processTeardown();

Why are those called in this order?

You should first clean up the state, then invoke any callback.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3803
> +    if (!m_isValid)
> +	   return;

When does that happen?
    1) terminateProcess()
    2) processTeardown()
    3) processDidCrash()
?
Because I would think you cannot get processDidCrash() after processTeardown() 
(because of m_process->removeMessageReceiver).

Can this be tested?

> 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.

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

This should be private.

> 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.

Why not terminateProcess()?