Bug 133351 - Viewport percentage tests that resize the viewport are flaky
Summary: Viewport percentage tests that resize the viewport are flaky
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-28 11:24 PDT by Bem Jones-Bey
Modified: 2014-05-31 23:25 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.00 KB, patch)
2014-05-28 11:33 PDT, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch with onresize event handler (2.28 KB, patch)
2014-05-29 09:04 PDT, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (59.84 KB, patch)
2014-05-31 16:47 PDT, Darin Adler
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bem Jones-Bey 2014-05-28 11:24:28 PDT
Viewport percentage tests that resize the viewport are flaky
Comment 1 Bem Jones-Bey 2014-05-28 11:33:12 PDT
Created attachment 232207 [details]
Patch
Comment 2 Martin Hock 2014-05-28 14:28:59 PDT
Committed r169434: <http://trac.webkit.org/changeset/169434>
Comment 3 Mark Lam 2014-05-28 14:30:54 PDT
(In reply to comment #2)
> Committed r169434: <http://trac.webkit.org/changeset/169434>

The test was marked as flaky in r169434.  Please revert the TestExpectation once this issue is resolved.
Comment 4 Darin Adler 2014-05-28 15:40:56 PDT
Comment on attachment 232207 [details]
Patch

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

> LayoutTests/css3/viewport-percentage-lengths/resources/resize-test.js:22
> +        // FIXME: It would be nice to verify that the window resized to the
> +        // given dimensions. Unfortunately, it looks like the window doesn't
> +        // always resize to the specified dimensions, so we can't do the
> +        // straightforward test, as it makes tests written using this script
> +        // flaky.

I think the issue here is a race condition. The problem is using a timeout/0. We should fix by waiting for the resize to happen.
Comment 5 Benjamin Poulain 2014-05-28 15:57:17 PDT
Comment on attachment 232207 [details]
Patch

I agree with Darin. We have a hard condition that the state must be correct when we send the resize event. The test should wait for the resize.
Comment 6 Bem Jones-Bey 2014-05-28 17:54:05 PDT
(In reply to comment #5)
> (From update of attachment 232207 [details])
> I agree with Darin. We have a hard condition that the state must be correct when we send the resize event. The test should wait for the resize.

When I originally wrote the test, I tried to use a window.onresize handler, and I had trouble getting it to work. Is that the correct thing to use here, or is there a better event to check for the resize to be finished?
Comment 7 Bem Jones-Bey 2014-05-29 09:04:30 PDT
Created attachment 232250 [details]
Patch with onresize event handler

So I tried again with the resize event handler, and am running into the same problems I ran into before: when I load it in my build of Safari, it runs, but if I reload the page, it doesn't run until I manually resize the window a little to kick it off. If I run it using RWT (WTR, not DRT), the resize handler is never triggered, and the test just times out. Am I doing something wrong with the way I am setting up the resize handler, or is there a bug in the interaction between resize handlers and programatically causing a resize?
Comment 8 Darin Adler 2014-05-30 20:58:37 PDT
(In reply to comment #7)
> So I tried again with the resize event handler, and am running into the same problems I ran into before: when I load it in my build of Safari, it runs, but if I reload the page, it doesn't run until I manually resize the window a little to kick it off. If I run it using RWT (WTR, not DRT), the resize handler is never triggered, and the test just times out. Am I doing something wrong with the way I am setting up the resize handler, or is there a bug in the interaction between resize handlers and programatically causing a resize?

Yes, we will have to debug to find out why there’s no resize event under WebKit2. I can’t tell just from your description, but we can set breakpoints to find out why it’s failing.
Comment 9 Darin Adler 2014-05-30 21:06:34 PDT
(In reply to comment #7)
> when I load it in my build of Safari, it runs, but if I reload the page, it doesn't run until I manually resize the window a little to kick it off.

I tried making a simple test, and I do not see this problem in stock Safari 7.0.4. Waiting for my build of TOT WebKit to see if I can reproduce the problem with the latest, or with WebKitTestRunner.
Comment 10 Darin Adler 2014-05-30 21:07:39 PDT
Oops, wait, my test was with the "timeout 0" version, not the resize event version!
Comment 11 Darin Adler 2014-05-30 21:12:36 PDT
(In reply to comment #8)
> if I reload the page, it doesn't run until I manually resize the window a little to kick it off

That’s because once we run the test the window is 800x600. So when we run the test again, the code sets the size to 800x600. That is not going to send a resize event, because the window is already that size!

Same problem under the test runner. The window’s default size is 800x600, and so setting it to that size again will not trigger a resize event.

One simple way to fix this is to “warm up” the test by resizing relative to the starting size, perhaps 10 pixels larger in each dimension, and then continue with the rest of the test.
Comment 12 Darin Adler 2014-05-30 21:26:52 PDT
After fixing the test to warm up properly, the resize events are firing, but under old Safari (not latest WebKit), I am still seeing flakiness; some kind of race condition where at the time of the resize event outerWidth and outerHeight sometimes have not yet been updated to the new size.
Comment 13 Darin Adler 2014-05-31 09:00:22 PDT
Those failures only happened with the older WebKit. With TOT WebKit everything seems to work fine.
Comment 14 Darin Adler 2014-05-31 11:59:10 PDT
If you look closely at the expected results, you’ll see that the resizing hasn’t been working in DumpRenderTree. The test is being run repeatedly at the same size. I’ve been working on this for a few hours today. There are many bugs here.
Comment 15 Darin Adler 2014-05-31 16:47:24 PDT
Created attachment 232325 [details]
Patch
Comment 16 Darin Adler 2014-05-31 17:01:44 PDT
Committed r169505: <http://trac.webkit.org/changeset/169505>
Comment 17 Alexey Proskuryakov 2014-05-31 23:05:56 PDT
svg/filters/filter-hidden-content.svg started to fail every time on all WebKit2 bots after this change:

http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK2%20(Tests)/r169508%20(6071)/svg/filters/filter-hidden-content-diff.txt
Comment 18 Alexey Proskuryakov 2014-05-31 23:11:01 PDT
Actually, two more tests are failing (it's difficult to see because WebKit2 testers still retry failures, hiding many of those that depend on previous tests):

fast/dynamic/window-resize-scrollbars-test.html
fast/dom/rtl-scroll-to-leftmost-and-resize.html
Comment 19 Darin Adler 2014-05-31 23:20:42 PDT
(In reply to comment #17)
> svg/filters/filter-hidden-content.svg started to fail every time on all WebKit2 bots after this change:

I noticed, and my change in <http://trac.webkit.org/changeset/169509> was motivated by that failure.
Comment 20 Darin Adler 2014-05-31 23:22:51 PDT
(In reply to comment #18)
> fast/dynamic/window-resize-scrollbars-test.html

http://trac.webkit.org/changeset/169508

> fast/dom/rtl-scroll-to-leftmost-and-resize.html

An expected failure as mentioned in the change log of http://trac.webkit.org/changeset/169505

These tests were “succeeding” before, but they weren’t really testing anything because they weren’t doing any resizing. We could skip them but I think it’s fine to run them as expected failures. Actually fixing the bugs they are testing for could be a big project. The RTL scroll bug is something that has never worked in any Mac port at least; I think it was fixed only for Chromium. The scrollbars bug has never worked on WebKit2, but the test wasn’t revealing that before because the test wasn’t doing any resizing.
Comment 21 Darin Adler 2014-05-31 23:25:19 PDT
(In reply to comment #19)
> (In reply to comment #17)
> > svg/filters/filter-hidden-content.svg started to fail every time on all WebKit2 bots after this change:
> 
> I noticed, and my change in <http://trac.webkit.org/changeset/169509> was motivated by that failure.

This reflects a problem in WebKitTestRunner. Might be hard to fix. I believe it’s a race condition where WebKitTestRunner resizes the view for a test and then runs the test but there is no guarantee the resizing has completed before the test is run.

What’s frustrating is that the thing that’s causing “failure” is not really part of the test at all. Just a layer that’s sized to the size of the view, which is incorrectly configured.