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.
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.