[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


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #196945|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #13 from Benjamin Poulain <benjamin at webkit.org>  2013-04-08 15:09:38 PST ---
(From update of attachment 196945)
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()?

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