Bug 101219 - Layout Test compositing/repaint/invalidations-on-composited-layers.html is failing/flaky
Summary: Layout Test compositing/repaint/invalidations-on-composited-layers.html is fa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-05 07:13 PST by Stephen White
Modified: 2012-11-12 12:52 PST (History)
8 users (show)

See Also:


Attachments
Patch (19.29 KB, patch)
2012-11-07 07:28 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (5.20 KB, patch)
2012-11-12 06:39 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (5.20 KB, patch)
2012-11-12 11:31 PST, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2012-11-05 07:13:10 PST
The following layout test is flaky on all platforms

compositing/repaint/invalidations-on-composited-layers.html

Probable cause:

Nothing obvious; WebKit (r133458) and Chrome (r165894) changes around the start of flakiness seem innocuous.
Comment 1 Stephen White 2012-11-05 07:19:20 PST
CC'ing vollick, since this test was introduced at http://trac.webkit.org/changeset/133332, and perhaps has been just intermittently flaky since then.
Comment 2 vollick 2012-11-07 07:28:02 PST
Created attachment 172796 [details]
Patch

The test is flaky because I'd forgotten to force a commit. This is normally done
via testRunner.display(), but that's problematic for a couple of reasons. First,
we're trying to stop using display altogether in favour of text-based repaint
testing. It's also a problem because testRunner.display() has unfortunate side
effects in addition to forcing a composite. For example, on mac it starts
repaint tracking and resets our stored repaint rects, making our test useless.

What we really need is a new method, testRunner.forceComposite(). This should
eventually replace testRunner.display() in all composited tests (since that was
always the real intent of using display there).

This CL adds the forceComposite method and uses it in the flaky test.
Comment 3 EFL EWS Bot 2012-11-07 07:39:41 PST
Comment on attachment 172796 [details]
Patch

Attachment 172796 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14757408
Comment 4 Simon Fraser (smfr) 2012-11-07 08:56:22 PST
Comment on attachment 172796 [details]
Patch

Is Chromium the only platform that requires special behavior here?
Can you just force the compositing update before dumping repaint rects (and layer tree)? I'd prefer this be something "automatic", rather than something that test authors have to think about, particularly if a test that passes on one platform is going to fail on another.
Comment 5 Build Bot 2012-11-07 09:32:54 PST
Comment on attachment 172796 [details]
Patch

Attachment 172796 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14763253
Comment 6 Simon Fraser (smfr) 2012-11-07 11:27:47 PST
Comment on attachment 172796 [details]
Patch

r- for now.
Comment 7 vollick 2012-11-12 06:39:26 PST
Created attachment 173638 [details]
Patch

It turns out that it's not a composite that I'm after here. testRunner.display()
causes a layout on chromium, and it's that layout that affects the lists of
invalidation rects. This layout can be forced simply by forcing a style recalc
in the usual way -- it just needs to happen before we start repaint tracking.

So although it would be nice to add a testRunner.forceComposite, it's not
required to fix the flake we're seeing here.
Comment 8 James Robinson 2012-11-12 09:44:00 PST
Comment on attachment 173638 [details]
Patch

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

> LayoutTests/compositing/repaint/invalidations-on-composited-layers.html:48
> +          var dummy = child.offsetTop;

just "document.body.offsetTop;" as a statement is sufficient, you don't have to declare a var
Comment 9 vollick 2012-11-12 11:31:13 PST
Created attachment 173678 [details]
Patch

(In reply to comment #8)
> (From update of attachment 173638 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173638&action=review
>
> > LayoutTests/compositing/repaint/invalidations-on-composited-layers.html:48
> > +          var dummy = child.offsetTop;
>
> just "document.body.offsetTop;" as a statement is sufficient, you don't have to declare a var

Thanks, that's much nicer.
Comment 10 James Robinson 2012-11-12 11:32:49 PST
Comment on attachment 173678 [details]
Patch

R=me although I wonder if this should be implicit as part of starting/stopping tracking.
Comment 11 vollick 2012-11-12 11:47:43 PST
(In reply to comment #10)
> (From update of attachment 173678 [details])
> R=me although I wonder if this should be implicit as part of starting/stopping tracking.

I was also thinking about automatically forcing a style recalc at start/stop and just before dumping the layer tree as text, but I was worried that there may be cases where we intentionally want to avoid forcing a style update. Maybe that's not a big concern though? I'll close this for now, but I'd be happy to open a bug if we decide to make the style recalcs implicit.
Comment 12 WebKit Review Bot 2012-11-12 12:52:00 PST
Comment on attachment 173678 [details]
Patch

Clearing flags on attachment: 173678

Committed r134287: <http://trac.webkit.org/changeset/134287>
Comment 13 WebKit Review Bot 2012-11-12 12:52:06 PST
All reviewed patches have been landed.  Closing bug.