Bug 109305 - [WK2] Page reloading will crash UIProcess after WebProcess was killed
Summary: [WK2] Page reloading will crash UIProcess after WebProcess was killed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Nobody
URL:
Keywords:
: 109407 109670 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-02-08 08:49 PST by Adenilson Cavalcanti Silva
Modified: 2013-02-13 10:49 PST (History)
12 users (show)

See Also:


Attachments
The backtrace (2.82 KB, text/plain)
2013-02-08 08:52 PST, Adenilson Cavalcanti Silva
no flags Details
Move the place where the object is created, add a test for pointer state in initializeWebPage() (2.22 KB, patch)
2013-02-08 08:59 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
EFL backtrace (4.83 KB, text/plain)
2013-02-09 10:57 PST, Adenilson Cavalcanti Silva
no flags Details
Alternative patch (1.87 KB, patch)
2013-02-11 11:08 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Following approach suggested by Dumez, plus test. (10.36 KB, patch)
2013-02-11 11:52 PST, Adenilson Cavalcanti Silva
buildbot: commit-queue-
Details | Formatted Diff | Diff
Adding test to GTK buildsystem, addressing reviewer's comments (11.35 KB, patch)
2013-02-11 14:53 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Now it should execute reload properly. (11.51 KB, patch)
2013-02-11 16:36 PST, Adenilson Cavalcanti Silva
buildbot: commit-queue-
Details | Formatted Diff | Diff
trying to make xcode happy again. (11.94 KB, patch)
2013-02-11 19:35 PST, Adenilson Cavalcanti Silva
buildbot: commit-queue-
Details | Formatted Diff | Diff
Xcode try again #001 (11.44 KB, patch)
2013-02-11 21:27 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
patch #01 (11.44 KB, patch)
2013-02-11 21:29 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Addressing reviewers comments: Changelog, comments, variable names, test name, xcode files ordering. (13.45 KB, patch)
2013-02-12 17:09 PST, Adenilson Cavalcanti Silva
benjamin: review+
benjamin: commit-queue-
Details | Formatted Diff | Diff
Should have the xcode changes right (hopefully) (12.53 KB, patch)
2013-02-12 17:31 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adenilson Cavalcanti Silva 2013-02-08 08:49:55 PST
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().
Comment 1 Adenilson Cavalcanti Silva 2013-02-08 08:52:22 PST
Created attachment 187328 [details]
The backtrace

The backtrace.
Comment 2 Adenilson Cavalcanti Silva 2013-02-08 08:59:42 PST
Created attachment 187331 [details]
Move the place where the object is created, add a test for pointer state in initializeWebPage()
Comment 3 Rafael Brandao 2013-02-08 13:43:48 PST
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.
Comment 4 Adenilson Cavalcanti Silva 2013-02-08 15:54:01 PST
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.
Comment 5 Adenilson Cavalcanti Silva 2013-02-09 10:55:59 PST
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).
Comment 6 Adenilson Cavalcanti Silva 2013-02-09 10:56:45 PST
For the record: the same bug will make EFL Minibrowser crash, if the WebProcess was killed.
Comment 7 Adenilson Cavalcanti Silva 2013-02-09 10:57:18 PST
Created attachment 187438 [details]
EFL backtrace
Comment 8 Chris Dumez 2013-02-11 10:30:46 PST
*** Bug 109407 has been marked as a duplicate of this bug. ***
Comment 9 Chris Dumez 2013-02-11 10:31:58 PST
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.
Comment 10 Chris Dumez 2013-02-11 10:33:24 PST
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.
Comment 11 Sam Weinig 2013-02-11 10:33:45 PST
(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.
Comment 12 Alexey Proskuryakov 2013-02-11 11:06:22 PST
Christophe, can you please post your patch for review here?
Comment 13 Chris Dumez 2013-02-11 11:08:04 PST
Created attachment 187617 [details]
Alternative patch
Comment 14 Adenilson Cavalcanti Silva 2013-02-11 11:52:50 PST
Created attachment 187630 [details]
Following approach suggested by Dumez, plus test.
Comment 15 Chris Dumez 2013-02-11 13:07:35 PST
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?
Comment 16 Adenilson Cavalcanti Silva 2013-02-11 13:23:29 PST
Christophe

Thanks for reviewing, I'm working in a new patch addressing the issues (plus adding support for GTK test building).
Comment 17 Build Bot 2013-02-11 14:05:27 PST
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
Comment 18 Adenilson Cavalcanti Silva 2013-02-11 14:53:53 PST
Created attachment 187686 [details]
Adding test to GTK buildsystem, addressing reviewer's comments
Comment 19 Adenilson Cavalcanti Silva 2013-02-11 16:26:56 PST
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.
Comment 20 Adenilson Cavalcanti Silva 2013-02-11 16:36:52 PST
Created attachment 187718 [details]
Now it should execute reload properly.
Comment 21 Build Bot 2013-02-11 18:32:03 PST
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
Comment 22 Adenilson Cavalcanti Silva 2013-02-11 19:35:10 PST
Created attachment 187759 [details]
trying to make xcode happy again.
Comment 23 Build Bot 2013-02-11 21:03:37 PST
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
Comment 24 Adenilson Cavalcanti Silva 2013-02-11 21:27:40 PST
Created attachment 187767 [details]
Xcode try again #001
Comment 25 Adenilson Cavalcanti Silva 2013-02-11 21:29:48 PST
Created attachment 187768 [details]
patch #01
Comment 26 Benjamin Poulain 2013-02-11 23:56:31 PST
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.
Comment 27 Adenilson Cavalcanti Silva 2013-02-12 17:09:28 PST
Created attachment 187968 [details]
Addressing reviewers comments: Changelog, comments, variable names, test name, xcode files ordering.
Comment 28 Benjamin Poulain 2013-02-12 17:24:35 PST
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?
Comment 29 Adenilson Cavalcanti Silva 2013-02-12 17:31:07 PST
Created attachment 187975 [details]
Should have the xcode changes right (hopefully)
Comment 30 Benjamin Poulain 2013-02-12 18:25:20 PST
Comment on attachment 187617 [details]
Alternative patch

Clearing this, patch with test available for landing.
Comment 31 WebKit Review Bot 2013-02-12 18:40:45 PST
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>
Comment 32 WebKit Review Bot 2013-02-12 18:40:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 33 Claudio Saavedra 2013-02-13 10:49:57 PST
*** Bug 109670 has been marked as a duplicate of this bug. ***