Bug 109305 – [WK2] Page reloading will crash UIProcess after WebProcess was killed

• Component: WebKit2
• Status: RESOLVED FIXED

[Adenilson Cavalcanti Silva]

Steps to reproduce:

a) Start a WK2 based browser
b) Kill the associated WebProcess
c) Reload the page

What happens:
A call to load a new page or to reload the current page will start by first inspecting the state of WebProcess. If it is not valid, WebPageProxy::reattachToWebProcess() is executed that later will call WebPageProxy::initializeWebPage().

The backtrace shows that in ::initializeWebPage the crash happens, at calling a method of the class object pointed by m_inspector (an instance of WebInspectorProxy). The issue is that when the WebProcess dies, WebPageProxy::processDidCrash() is executed and sets the pointer to null.

This patch adds a check for the pointer state before executing calls on it and moves the creation of WebInspector object from the reattachToWebProcess() to initializeWebPage().

[Adenilson Cavalcanti Silva]

Created attachment 187328 [details]
The backtrace

The backtrace.

[Adenilson Cavalcanti Silva]

Created attachment 187331 [details]
Move the place where the object is created, add a test for pointer state in initializeWebPage()

[Rafael Brandao]

Comment on attachment 187331 [details]
Move the place where the object is created, add a test for pointer state in initializeWebPage()

Are you sure ENABLE(INSPECTOR) and ENABLE(INSPECTOR_SERVER) are equivalent? Note you've moved something from one protector to the other.

[Adenilson Cavalcanti Silva]

Rafael

Probably they are different. But if you look at the code, the calls to WebInspectorProxy methods only happen on ENABLE(INSPECTOR_SERVER) blocks.

Those are the calls that can result in crashes if the pointer are set to null.

[Adenilson Cavalcanti Silva]

A heads up on the bug: I started to write a WK2 API to stress the scenario (crash/reload).

It is building/running on mac and EFL ports (GTK should be next).

[Adenilson Cavalcanti Silva]

For the record: the same bug will make EFL Minibrowser crash, if the WebProcess was killed.

[Adenilson Cavalcanti Silva]

Created attachment 187438 [details]
EFL backtrace

[Chris Dumez]

*** Bug 109407 has been marked as a duplicate of this bug. ***

[Chris Dumez]

I have just been made aware of this bug report. I already had a patch for this as well at Bug 109407.
My patch seems a bit simpler.

[Chris Dumez]

Comment on attachment 187331 [details]
Move the place where the object is created, add a test for pointer state in initializeWebPage()

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:436
> +        if (!m_inspector)

If a port has INSPECTOR enabled but INSPECTOR_SERVER disabled, it is likely going to cause problems as m_inspector is not going to be initialized. Please check my patch at Bug 109407.

[Sam Weinig]

(In reply to comment #9)
> I have just been made aware of this bug report. I already had a patch for this as well at Bug 109407.
> My patch seems a bit simpler.

The patch in that bug seems like the right fix.

[Alexey Proskuryakov]

Christophe, can you please post your patch for review here?

[Chris Dumez]

Created attachment 187617 [details]
Alternative patch

[Adenilson Cavalcanti Silva]

Created attachment 187630 [details]
Following approach suggested by Dumez, plus test.

[Chris Dumez]

Comment on attachment 187630 [details]
Following approach suggested by Dumez, plus test.

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

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:31
> +#include <stdio.h>

Is this really needed?

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:35
> +static bool done = false;

I think we only need one boolean.

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:40
> +    // Ok, this is the first load

iirc. Coding style says comments should end with a period.

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:43
> +        EXPECT_TRUE(done);

This check right after assigning does not seem useful.

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:48
> +    EXPECT_EQ(static_cast<uint32_t>(kWKFrameLoadStateFinished), WKFrameGetFrameLoadState(frame));

This check could be done in CrashReload test function.

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:50
> +    EXPECT_TRUE(loaded);

This check right after assigning does not seem useful.

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:60
> +    loaderClient.didFinishLoadForFrame = didFinishLoad;

Should we make sure processDidCrash is called as well?

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:67
> +    Util::sleep(3);

I don't understand why we need to sleep.

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:69
> +    Util::sleep(3);

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:70
> +    WKPageLoadURL(webView.page(), adoptWK(WKURLCreateWithUTF8CString("about:blank")).get());

Since we use the same WKUrl twice, maybe we could create it once and store it in a variable?

[Adenilson Cavalcanti Silva]

Christophe

Thanks for reviewing, I'm working in a new patch addressing the issues (plus adding support for GTK test building).

[Build Bot]

Comment on attachment 187630 [details]
Following approach suggested by Dumez, plus test.

Attachment 187630 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16504235

[Adenilson Cavalcanti Silva]

Created attachment 187686 [details]
Adding test to GTK buildsystem, addressing reviewer's comments

[Adenilson Cavalcanti Silva]

Looking into PlatformUtilities.h, I've found this:

// Runs a platform runloop until the 'done' is true. 
void run(bool* done);

So I think we will need two control variables instead of a single one.

[Adenilson Cavalcanti Silva]

Created attachment 187718 [details]
Now it should execute reload properly.

[Build Bot]

Comment on attachment 187718 [details]
Now it should execute reload properly.

Attachment 187718 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16486633

[Adenilson Cavalcanti Silva]

Created attachment 187759 [details]
trying to make xcode happy again.

[Build Bot]

Comment on attachment 187759 [details]
trying to make xcode happy again.

Attachment 187759 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16486734

[Adenilson Cavalcanti Silva]

Created attachment 187767 [details]
Xcode try again #001

[Adenilson Cavalcanti Silva]

Created attachment 187768 [details]
patch #01

[Benjamin Poulain]

Comment on attachment 187768 [details]
patch #01

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

Some comments:

> Source/WebKit2/ChangeLog:9
> +        Re-initialize pointer to WebInspectorProxy object before calling
> +        initializeWebPage().

You should also explain why you make the change, not only what the change is.

> Tools/ChangeLog:14
> +        * TestWebKitAPI/Tests/WebKit2/CrashReload.cpp: Added.

I'd argue the name CrashReload is a little too generic. It is hard to pinpoint what is tested in there.

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:1091
>  				2E7765CD16C4D80A00BA2BB1 /* mainIOS.mm in Sources */,
>  				2E7765CF16C4D81100BA2BB1 /* mainMac.mm in Sources */,
> +				8A3AF93B16C9ED2700D248C1 /* CrashReload.cpp in Sources */,

The build section should be sorted alphabetically

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:35
> +static bool first = false;
> +static bool second = false;

Those names are too generic.
First, could they be made local instead of being global?
Second, they should be named in a way take make their purpose obvious.

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:39
> +    // Ok, this is the first load.

Comments like this are not helping.

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:43
> +    if (!first) {
> +        first = true;
> +        return;
> +    }

Should't this also check second is false? If second is true here

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:45
> +    // Second time is the charm.

Comments like this are not helping, comments should explain the "Why" of the code.

> Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:74
> +    // Load a blank page and next kills it

Period at the end of the sentence.

[Adenilson Cavalcanti Silva]

Created attachment 187968 [details]
Addressing reviewers comments: Changelog, comments, variable names, test name, xcode files ordering.

[Benjamin Poulain]

Comment on attachment 187968 [details]
Addressing reviewers comments: Changelog, comments, variable names, test name, xcode files ordering.

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

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:-1087
> -				2E7765CD16C4D80A00BA2BB1 /* mainIOS.mm in Sources */,
> -				2E7765CF16C4D81100BA2BB1 /* mainMac.mm in Sources */,

Where is mainMac.mm?

[Adenilson Cavalcanti Silva]

Created attachment 187975 [details]
Should have the xcode changes right (hopefully)

[Benjamin Poulain]

Comment on attachment 187617 [details]
Alternative patch

Clearing this, patch with test available for landing.

[WebKit Review Bot]

Comment on attachment 187975 [details]
Should have the xcode changes right (hopefully)

Clearing flags on attachment: 187975

Committed r142704: <http://trac.webkit.org/changeset/142704>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Claudio Saavedra]

*** Bug 109670 has been marked as a duplicate of this bug. ***