Bug 109842 - [WK2] Write a test to simulate crashed WebProcess followed by Window resize
Summary: [WK2] Write a test to simulate crashed WebProcess followed by Window resize
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 109216 109980
  Show dependency treegraph
 
Reported: 2013-02-14 09:39 PST by Adenilson Cavalcanti Silva
Modified: 2013-02-15 16:29 PST (History)
11 users (show)

See Also:


Attachments
Missing XCode buildsystem changes (6.79 KB, patch)
2013-02-14 09:46 PST, Adenilson Cavalcanti Silva
benjamin: review-
Details | Formatted Diff | Diff
Implementing reviewer's suggestion (except OwnPtr) and Xcode build (10.61 KB, patch)
2013-02-15 11:50 PST, Adenilson Cavalcanti Silva
buildbot: commit-queue-
Details | Formatted Diff | Diff
Xcode: the mission! (64 bytes, patch)
2013-02-15 12:03 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Try #02 (10.62 KB, patch)
2013-02-15 12:12 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Using clientInfo to pass PlatformWebview to didCrash callback (10.60 KB, patch)
2013-02-15 12:44 PST, Adenilson Cavalcanti Silva
buildbot: commit-queue-
Details | Formatted Diff | Diff
Data in a struct, hopefully XCode should be happy (10.97 KB, patch)
2013-02-15 14:37 PST, Adenilson Cavalcanti Silva
benjamin: review+
benjamin: commit-queue-
Details | Formatted Diff | Diff
Missing point. (10.97 KB, patch)
2013-02-15 16:00 PST, Adenilson Cavalcanti Silva
benjamin: review+
Details | Formatted Diff | Diff
Constructor curly braces spacing fix (10.97 KB, patch)
2013-02-15 16:20 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-14 09:39:43 PST
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.
Comment 1 Adenilson Cavalcanti Silva 2013-02-14 09:46:47 PST
Created attachment 188372 [details]
Missing XCode buildsystem changes

Not to commit, I'm looking for comments if the approach is sane.
Comment 2 Benjamin Poulain 2013-02-14 23:23:30 PST
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.
Comment 3 Adenilson Cavalcanti Silva 2013-02-15 11:50:27 PST
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.
Comment 4 Build Bot 2013-02-15 11:54:39 PST
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
Comment 5 Adenilson Cavalcanti Silva 2013-02-15 12:03:57 PST
Created attachment 188614 [details]
Xcode: the mission!
Comment 6 Chris Dumez 2013-02-15 12:05:59 PST
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.
Comment 7 Chris Dumez 2013-02-15 12:09:53 PST
(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.
Comment 8 Adenilson Cavalcanti Silva 2013-02-15 12:12:27 PST
Created attachment 188616 [details]
Try #02
Comment 9 Adenilson Cavalcanti Silva 2013-02-15 12:44:31 PST
Created attachment 188622 [details]
Using clientInfo to pass PlatformWebview to didCrash callback
Comment 10 Build Bot 2013-02-15 13:11:16 PST
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
Comment 11 WebKit Review Bot 2013-02-15 13:41:50 PST
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.
Comment 12 Adenilson Cavalcanti Silva 2013-02-15 14:37:47 PST
Created attachment 188645 [details]
Data in a struct, hopefully XCode should be happy
Comment 13 Benjamin Poulain 2013-02-15 14:46:39 PST
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?
Comment 14 Adenilson Cavalcanti Silva 2013-02-15 16:00:27 PST
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.
Comment 15 Benjamin Poulain 2013-02-15 16:03:55 PST
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)
    {
    }
Comment 16 Adenilson Cavalcanti Silva 2013-02-15 16:20:42 PST
Created attachment 188662 [details]
Constructor curly braces spacing fix
Comment 17 Benjamin Poulain 2013-02-15 16:23:53 PST
Committed r143067: <http://trac.webkit.org/changeset/143067>
Comment 18 Benjamin Poulain 2013-02-15 16:29:25 PST
Comment on attachment 188662 [details]
Constructor curly braces spacing fix

Clearing flags, this is landed.