Bug 139736 – [iOS] WebKitTestRunner: Tests fail due to incorrect web view size

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

When I ran all the layout tests for iOS using WebKitTestRunner today, some tests intermittently fail because the size of the RenderView associated with the main web view is 1024 x 768 pixels instead of 800 x 600 pixels. For example, the test LayoutTests/tables/mozilla/dom/insertCellsExpand2.html failed with the following diff in one run of the full suite of layout tests:

[[
--- /Volumes/Data/WebKitDevGit/OpenSource/layout-test-results-rebase/tables/mozilla/dom/insertCellsExpand2-expected.txt
+++ /Volumes/Data/WebKitDevGit/OpenSource/layout-test-results-rebase/tables/mozilla/dom/insertCellsExpand2-actual.txt
@@ -1,9 +1,9 @@
-layer at (0,0) size 800x600
-  RenderView at (0,0) size 800x600
-layer at (0,0) size 800x600
-  RenderBlock {HTML} at (0,0) size 800x600
-    RenderBody {BODY} at (8,8) size 784x584
-      RenderBlock (anonymous) at (0,0) size 784x20
+layer at (0,0) size 1024x768
+  RenderView at (0,0) size 1024x768
+layer at (0,0) size 1024x768
+  RenderBlock {HTML} at (0,0) size 1024x768
+    RenderBody {BODY} at (8,8) size 1008x752
+      RenderBlock (anonymous) at (0,0) size 1008x20
         RenderText {#text} at (0,0) size 218x19
           text run at (0,0) width 218: "The 2 tables should look the same"
       RenderTable {TABLE} at (0,20) size 124x82 [bgcolor=#FFA500] [border: (1px outset #808080)]
@@ -38,7 +38,7 @@
             RenderTableCell {TD} at (92,54) size 28x24 [border: (1px inset #808080)] [r=2 c=3 rs=1 cs=1]
               RenderText {#text} at (2,2) size 24x19
                 text run at (2,2) width 24: "c34"
-      RenderBlock (anonymous) at (0,102) size 784x20
+      RenderBlock (anonymous) at (0,102) size 1008x20
         RenderBR {BR} at (0,0) size 0x19
       RenderTable {TABLE} at (0,122) size 124x82 [bgcolor=#FFA500] [border: (1px outset #808080)]
         RenderTableSection {TBODY} at (1,1) size 122x80
]]

The tests that fail with similar differences change between runs of the full suite of layout tests. Moreover, the tests that fail with such differences pass when run either individually or as part of running all the tests in some sub-directory of LayoutTests along its path. In particular, LayoutTests/tables/mozilla/dom/insertCellsExpand2.html passes when run individually or when run as part of running all the tests in LayoutTests/tables.

For completeness, I ran the full suite of layout tests using a command line of the form:

Tools/Scripts/run-webkit-tests --ios-sim --runtime com.apple.CoreSimulator.SimRuntime.iOS-[...] --device-type com.apple.CoreSimulator.SimDeviceType.iPhone-5s --no-sample-on-timeout --results-directory layout-test-results-rebase -2

[Daniel Bates]

I forgot to add that when I run the full test suite run-webkit-tests runs 24 instances of WebKitTestRunnerApp.apps in parallel. The test failure of LayoutTests/tables/mozilla/dom/insertCellsExpand2.html occurred during a run with a debug build of WebKit at r177430. I am using a Mac Pro with 2 x 2.66 GHz 6-Core Intel Xeon processors, 12 GB RAM and OS X Yosemite (14A378).

[Alexey Proskuryakov]

Some tests force a larger window size (imported svg tests I think?). Sounds like we fail to resize back when returning to normal tests.

run-webkit-tests --debug-rwt-logging shows the order of tests run on each worker.

[Radar WebKit Bug Importer]

<rdar://problem/19282196>

[David Kilzer (:ddkilzer)]

As a quick fix, can we "isolate" SVG tests to their own subset of parallel workers?  (Not sure if that's possible or not in run-webkit-tests.)

[Jon Honeycutt]

Created attachment 244497 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 244497 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/PageClientImplIOS.h:49
>      PageClientImpl(WKContentView *, WKWebView *);
> +    PageClientImpl(WKContentView *, WKView *);

I don't think we want to add more uses of WKView in WebKit2, since it's obsolete and will be removed. Isn't there some other way we can get the _didRelaunchProcess notification out for WTR?

[Jon Honeycutt]

(In reply to comment #6)
> Comment on attachment 244497 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244497&action=review
> 
> > Source/WebKit2/UIProcess/ios/PageClientImplIOS.h:49
> >      PageClientImpl(WKContentView *, WKWebView *);
> > +    PageClientImpl(WKContentView *, WKView *);
> 
> I don't think we want to add more uses of WKView in WebKit2, since it's
> obsolete and will be removed. Isn't there some other way we can get the
> _didRelaunchProcess notification out for WTR?

I discussed this with Ben, and he felt it was OK to add a pointer to the WKView to PageClient until we reach the point that we can remove WKView.

I couldn't come up with a better approach to fix this for WKView, but we could fix this by adopting WKWebView for WKTR, because it already handles this case. <rdar://problem/16947123> covers that.

[Simon Fraser (smfr)]

Comment on attachment 244497 [details]
Patch

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

> Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:344
> +- (void)_didRelaunchProcess
> +{
> +    // Update the WebView to our size rather than the default size it will have after being relaunched.
> +    [self _frameOrBoundsChanged];
> +}

But there's already -[WKView _didRelaunchProcess] in WKView.mm. Why can't you just add code there?

[Jon Honeycutt]

(In reply to comment #8)
> Comment on attachment 244497 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244497&action=review
> 
> > Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:344
> > +- (void)_didRelaunchProcess
> > +{
> > +    // Update the WebView to our size rather than the default size it will have after being relaunched.
> > +    [self _frameOrBoundsChanged];
> > +}
> 
> But there's already -[WKView _didRelaunchProcess] in WKView.mm. Why can't
> you just add code there?

That's the Mac WKView impl. It's not shared with iOS.

[Simon Fraser (smfr)]

(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 244497 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=244497&action=review
> > 
> > > Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:344
> > > +- (void)_didRelaunchProcess
> > > +{
> > > +    // Update the WebView to our size rather than the default size it will have after being relaunched.
> > > +    [self _frameOrBoundsChanged];
> > > +}
> > 
> > But there's already -[WKView _didRelaunchProcess] in WKView.mm. Why can't
> > you just add code there?
> 
> That's the Mac WKView impl. It's not shared with iOS.

Can we do something via the C SPI?

[Jon Honeycutt]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Comment on attachment 244497 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=244497&action=review
> > > 
> > > > Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:344
> > > > +- (void)_didRelaunchProcess
> > > > +{
> > > > +    // Update the WebView to our size rather than the default size it will have after being relaunched.
> > > > +    [self _frameOrBoundsChanged];
> > > > +}
> > > 
> > > But there's already -[WKView _didRelaunchProcess] in WKView.mm. Why can't
> > > you just add code there?
> > 
> > That's the Mac WKView impl. It's not shared with iOS.
> 
> Can we do something via the C SPI?

AFAICT, the C SPI only goes as far as notifying the PageClient, which doesn't currently have access to the WKView.

I did consider another hacky fix for this. WKTR knows that it's killing and relaunching the web process, so it can force the WebView to resize itself, which will send a message to the web process telling it the correct layout size. However, WKView has an early return in -setFrame: if the size of the view hasn't changed, so we would have to first resize the view to something other than 800x600 and then resize it again to 800x600 whenever WKTR decides to kill the web process.

Given that both fixes are temporary workarounds, that approach would have the benefit of being a smaller change.

[Sam Weinig]

(In reply to comment #11)

I am ok with this patch a temporary measure to get more tests passing in the short term. I am working on getting WKWebView to be something we can base WebKitTestRunner on, but I don't see why this should be blocked on that in the interim.

[Benjamin Poulain]

(In reply to comment #12)
> (In reply to comment #11)
> 
> I am ok with this patch a temporary measure to get more tests passing in the
> short term. I am working on getting WKWebView to be something we can base
> WebKitTestRunner on, but I don't see why this should be blocked on that in
> the interim.

+1

This is disgusting, but not running tests is worse.

[Benjamin Poulain]

Comment on attachment 244497 [details]
Patch

Ok, let's do it.

I r+ this as a workaround until WKWebView is the default for testing.

I hope I can outrun Simon if he tries to strangle me.

[Jon Honeycutt]

Landed in <http://trac.webkit.org/changeset/178870>.