Bug 109842 – [WK2] Write a test to simulate crashed WebProcess followed by Window resize

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Adenilson Cavalcanti Silva]

When WebProcess is killed or has crashed, WebPageProxy will set some of its data members to null (e.g. DrawingAreaProxy, WebFrameProxies, etc).

Some of those elements can later be accessed by the port code, when, for example, there is a Window resize event.

The objective here is to create a WK2 API test to simulate the following scenario: WebProcess crash followed by a Window resize.

Assuming that the port is well behaved, it will test those WebPageProxy members state before making calls into them.

[Adenilson Cavalcanti Silva]

Created attachment 188372 [details]
Missing XCode buildsystem changes

Not to commit, I'm looking for comments if the approach is sane.

[Benjamin Poulain]

Comment on attachment 188372 [details]
Missing XCode buildsystem changes

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

The test is great.

Try to be more careful with your comments.
Think about how someone else will read this code in 2 years when the test fails. You need to maximize the important information, while reducing the noise and redundant information.

> Tools/ChangeLog:14
> +        * TestWebKitAPI/GNUmakefile.am:
> +        * TestWebKitAPI/PlatformEfl.cmake:
> +        * TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp: Added.

What about the xcodeproject ?

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:36
> +static bool resizeAfterCrash = false;
> +static bool done = false;
> +static PlatformWebView *webView = 0;

WebKit style: *webView.

"done" is too generic in this case. Because when "done == true", the test is not finished!

I would prefer if you put those 3 in a struct and pass it as clientInfo.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:40
> +    // Loading a blank page worked, next we will kill WebProcess

This comment is true only in the if(). It should be in the brackets.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:43
> +        // Set it, otherwise the loop will get stuck and we won't move to the
> +        // next step.

No need to cut the line. You can even get rid of this comment.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:48
> +    // If the UIProcess was able to resize (without a bounded WebProcess),

"bounded WebProcess"?

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:56
> +    // TODO: use a smart/shared pointer here
> +    delete webView;

Use a OwnPtr in your Struct.

Don't leave a TODO in a test :)
We also don't use TODO in WebKit, but FIXME.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:68
> +
> +    // Could be any size, it is just a matter of trigging an event
> +    // that will force the port to re-act (and if it is well behaved,
> +    // it will test WebPageProxy data members state before accessing them).
> +    // Maybe a safer value would be current window size + n
> +    // (where n = any value).
> +    webView->resizeTo(800, 800);

This comment is too long. Here, you could state:
1) This is the part being tested: resize could cause the UIProcess to crash. The test succeed if we do not crash.
2) The reason of the crash: The view was accessing the dead web process.

For the size, I would not bother and just call resizeTo twice:
    webView->resizeTo(100, 200);
    webView->resizeTo(300, 400);
That was you are sure whatever the size was, resize was not skipped.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:69
> +    resizeAfterCrash = false;

It should just be initialize to false when creating the clientInfo struct.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:93
> +    // Let's try load a page and see what happens.
> +    WKPageLoadURL(webView->page(), url.get());
> +    Util::run(&resizeAfterCrash);

It is great you have this part.

[Adenilson Cavalcanti Silva]

Created attachment 188610 [details]
Implementing reviewer's suggestion (except OwnPtr) and Xcode build

Using OwnPtr (or auto_ptr) with a static/global variable will result in SEGFAULT at test exit.

[Build Bot]

Comment on attachment 188610 [details]
Implementing reviewer's suggestion (except OwnPtr) and Xcode build

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

[Adenilson Cavalcanti Silva]

Created attachment 188614 [details]
Xcode: the mission!

[Chris Dumez]

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

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:36
> +static PlatformWebView *webView = 0;

You could pass it as clientInfo to the callbacks. This way it wouldn't need to be global and you could use OwnPtr.

[Chris Dumez]

(In reply to comment #6)
> View in context: https://bugs.webkit.org/attachment.cgi?id=188610&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:36
> > +static PlatformWebView *webView = 0;
> 
> You could pass it as clientInfo to the callbacks. This way it wouldn't need to be global and you could use OwnPtr.

I just noticed that Benjamin actually suggested that already:
> I would prefer if you put those 3 in a struct and pass it as clientInfo.

[Adenilson Cavalcanti Silva]

Created attachment 188616 [details]
Try #02

[Adenilson Cavalcanti Silva]

Created attachment 188622 [details]
Using clientInfo to pass PlatformWebview to didCrash callback

[Build Bot]

Comment on attachment 188622 [details]
Using clientInfo to pass PlatformWebview to didCrash callback

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

[WebKit Review Bot]

Attachment 188622 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/GNUmakefile.am', u'Tools/TestWebKitAPI/PlatformEfl.cmake', u'Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj', u'Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp']" exit_code: 1
Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:53:  Declaration has space between type name and * in PlatformWebView *webView  [whitespace/declaration] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adenilson Cavalcanti Silva]

Created attachment 188645 [details]
Data in a struct, hopefully XCode should be happy

[Benjamin Poulain]

Comment on attachment 188645 [details]
Data in a struct, hopefully XCode should be happy

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

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:38
> +    : webView(context)
> +    , firstLoad(false)
> +    , resizeAfterCrash(false)

This would be indented 4 more spaces to the right in WebKit style.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:50
> +        // Loading a blank page worked, next we will kill WebProcess

Period at the end of the sentence.

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:69
> +    WKPageReload(page);

What cases does this cover?

[Adenilson Cavalcanti Silva]

Created attachment 188659 [details]
Missing point.

Removing WKPageReload() will make re-spawn of WebProcess fail (tested in linux/efl and mac ports), thus making the test hang and timeout.

This will be further investigated in issue #109980.

[Benjamin Poulain]

Comment on attachment 188659 [details]
Missing point.

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

> Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp:39
> +    : webView(context)
> +    , firstLoad(false)
> +    , resizeAfterCrash(false)
> +    { }

Should be: 
    TestStatesData(WKContextRef context)
        : webView(context)
        , firstLoad(false)
        , resizeAfterCrash(false)
    {
    }

[Adenilson Cavalcanti Silva]

Created attachment 188662 [details]
Constructor curly braces spacing fix

[Benjamin Poulain]

Committed r143067: <http://trac.webkit.org/changeset/143067>

[Benjamin Poulain]

Comment on attachment 188662 [details]
Constructor curly braces spacing fix

Clearing flags, this is landed.