Summary: | [WK2] Write a test to simulate crashed WebProcess followed by Window resize | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adenilson Cavalcanti Silva <savagobr> | ||||||||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | andersca, benjamin, buildbot, cdumez, gyuyoung.kim, hausmann, rakuco, rniwa, savagobr, webkit.review.bot, zan | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 109216, 109980 | ||||||||||||||||||||
Attachments: |
|
Description
Adenilson Cavalcanti Silva
2013-02-14 09:39:43 PST
Created attachment 188372 [details]
Missing XCode buildsystem changes
Not to commit, I'm looking for comments if the approach is sane.
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. 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 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 Created attachment 188614 [details]
Xcode: the mission!
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. (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. Created attachment 188616 [details]
Try #02
Created attachment 188622 [details]
Using clientInfo to pass PlatformWebview to didCrash callback
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 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.
Created attachment 188645 [details]
Data in a struct, hopefully XCode should be happy
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? 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 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) { } Created attachment 188662 [details]
Constructor curly braces spacing fix
Committed r143067: <http://trac.webkit.org/changeset/143067> Comment on attachment 188662 [details]
Constructor curly braces spacing fix
Clearing flags, this is landed.
|