Bug 79823

Summary: [Chromium] Every compositing layout test with render surfaces is flaky
Product: WebKit Reporter: Adam Klein <adamk>
Component: Tools / TestsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, bashi, enne, haraken, jamesr, jannywatson1999, morrita, senorblanco, shawnsingh, suchiabram, trchen, ukai, vangelis
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch enne: review+

Description Adam Klein 2012-02-28 12:40:37 PST
The following layout test is failing flakily on all platforms:

compositing/repaint/opacity-between-absolute.html

See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=compositing%2Frepaint%2Fopacity-between-absolute.html

From the dashboard, http://trac.webkit.org/changeset/108886/ looks like a possibility.  Since that change, the test has been failing consistently on Linux dbg, and most of the time on Windows.
Comment 1 Adam Klein 2012-02-28 16:00:38 PST
Also seeing similar flakiness (similarly correlated with 108886) in compositing/reflections/masked-reflection-on-composited.html:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=compositing%2Freflections%2Fmasked-reflection-on-composited.html
Comment 2 Adam Klein 2012-02-28 16:03:35 PST
Committed r109164: <http://trac.webkit.org/changeset/109164>
Comment 3 Adam Barth 2012-03-13 15:04:14 PDT
New, seemingly related failures:

compositing/geometry/fixed-position.html
compositing/reflections/compositing-change-inside-reflection.html
compositing/reflections/reflection-on-composited.html
compositing/webgl/webgl-reflection.html
platform/chromium/compositing/render-surface-alpha-blending.html
Comment 4 Adam Barth 2012-03-13 15:13:38 PDT
Committed r110621: <http://trac.webkit.org/changeset/110621>
Comment 5 Adam Barth 2012-03-13 15:13:54 PDT
Not fixed...
Comment 6 Adam Barth 2012-03-13 15:35:25 PDT
jamesr: I think there's something wrong with the compositor dropping images.  Here's another example:

compositing/masks/direct-image-mask.html

http://build.chromium.org/f/chromium/layout_test_results/Webkit_Linux_32/results/layout-test-results/compositing/masks/direct-image-mask-diff.png

It's some kind of flaky problem, which makes it hard to track down.  Have their been any changes related to masking and/or reflections in the compositor recently?
Comment 7 Adam Barth 2012-03-13 15:35:44 PDT
+enne for additional thoughts.
Comment 8 Adrienne Walker 2012-03-13 15:57:57 PDT
(In reply to comment #7)
> +enne for additional thoughts.

I'd be kind of surprised if r108886 broke things in a flaky manner, but I don't have any better suggestions or insight here, sorry.
Comment 9 Tien-Ren Chen 2012-03-13 16:15:23 PDT
I probably see why r108886 can cause a problem with reflection. The LayerChromium is shared between its owner GraphicsLayer and also the reflection GraphicsLayer (as replica). The impl-side clone shouldn't really use an OwnPtr for the replica layer.

I think I can come up with a fix reasonably soon. Just don't know why the tests are just flaky instead of failing constantly.
Comment 10 Tien-Ren Chen 2012-03-13 16:57:30 PDT
(In reply to comment #9)
> I probably see why r108886 can cause a problem with reflection. The LayerChromium is shared between its owner GraphicsLayer and also the reflection GraphicsLayer (as replica). The impl-side clone shouldn't really use an OwnPtr for the replica layer.
> 
> I think I can come up with a fix reasonably soon. Just don't know why the tests are just flaky instead of failing constantly.

NVM, it was a wrong guess. The replica layers don't seem to be shared.
Comment 11 Hajime Morrita 2012-03-13 22:59:05 PDT
*** Bug 81080 has been marked as a duplicate of this bug. ***
Comment 12 James Robinson 2012-03-14 11:23:19 PDT
Taking a look - this is really bad :/
Comment 13 Stephen White 2012-03-14 13:07:40 PDT
Some other layout tests with similar symptoms:

https://bugs.webkit.org/show_bug.cgi?id=79572
https://bugs.webkit.org/show_bug.cgi?id=80563
http://crbug.com/114777

Those bugs all seem to have RenderSurface in common, which  I think that's also used for reflections and masks, which are some of the failures reported on this bug.  Could be related to RenderSurface in some way?
Comment 14 Stephen White 2012-03-14 13:17:50 PDT
(In reply to comment #13)
> Some other layout tests with similar symptoms:
> 
> https://bugs.webkit.org/show_bug.cgi?id=79572
> https://bugs.webkit.org/show_bug.cgi?id=80563
> http://crbug.com/114777
> 
> Those bugs all seem to have RenderSurface in common, which  I think that's also used for reflections and masks, which are some of the failures reported on this bug.  Could be related to RenderSurface in some way?

By RenderSurface I meant CCRenderSurface, of course.  (And by <really poor grammar> I meant <really excellent grammar>.)
Comment 15 James Robinson 2012-03-14 13:21:29 PDT
Yeah, there's a bug with caching a CCRenderSurface pointer in LayerRendererChromium. Patch on the way.
Comment 16 James Robinson 2012-03-14 13:22:40 PDT
*** Bug 80563 has been marked as a duplicate of this bug. ***
Comment 17 James Robinson 2012-03-14 13:25:17 PDT
*** Bug 74731 has been marked as a duplicate of this bug. ***
Comment 18 James Robinson 2012-03-14 13:27:07 PDT
*** Bug 80007 has been marked as a duplicate of this bug. ***
Comment 19 James Robinson 2012-03-14 13:27:28 PDT
*** Bug 79572 has been marked as a duplicate of this bug. ***
Comment 20 James Robinson 2012-03-14 13:30:23 PDT
Created attachment 131910 [details]
Patch
Comment 21 James Robinson 2012-03-14 13:31:27 PDT
*** Bug 74050 has been marked as a duplicate of this bug. ***
Comment 22 Adrienne Walker 2012-03-14 13:35:16 PDT
Comment on attachment 131910 [details]
Patch

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

R=me.  Nice catch.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-395
> -    // The GL viewport covers the entire visible area, including the scrollbars.
> -    GLC(m_context.get(), m_context->viewport(0, 0, viewportWidth(), viewportHeight()));
> -    m_windowMatrix = screenMatrix(0, 0, viewportWidth(), viewportHeight());
> -

Good riddance to this too.
Comment 23 James Robinson 2012-03-14 13:52:53 PDT
Committed r110741: <http://trac.webkit.org/changeset/110741>
Comment 24 James Robinson 2012-03-14 14:54:15 PDT
*** Bug 79647 has been marked as a duplicate of this bug. ***
Comment 25 Stephen White 2012-03-15 07:47:14 PDT
Thanks!  I had it on my list to look into this after my gardening shift, but I got sidelined by other things.  Fix is much appreciated!