Bug 59159 – [chromium] Ensure compositing layers are up to date before entering doComposite

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Ensure compositing layers are up to date before entering doComposite

[James Robinson]

Created attachment 90639 [details]
Patch

[James Robinson]

My testcase for this is extremely flaky and timing-depending, but the bad case is roughly that at the start of WebViewImpl::composite() we have a composited RenderLayer inside an iframe with a dirty z order list.  When we call paintContents() on that layer, it lazily updates the z order list and re-evaluates the compositing requirements on that RenderLayerCompositor which can change the GraphicsLayer tree.  This patch avoids the issue by explicitly updating compositing layers for every FrameView (which is 1:1 with RenderLayerCompositor) in the patch, syncing the compositing state for all FrameViews, and then not pumping the event loop until after all the paintContents() calls complete.  I've also added some assertions to make it easier to detect this case in debug builds.

[Kenneth Russell]

Comment on attachment 90639 [details]
Patch

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

Looks good to me.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:64
> +#ifndef NDEBUG
> +bool LayerRendererChromium::s_inPaintContents = false;
> +#endif

Barf!

[James Robinson]

(In reply to comment #3)
> (From update of attachment 90639 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90639&action=review
> 
> Looks good to me.
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:64
> > +#ifndef NDEBUG
> > +bool LayerRendererChromium::s_inPaintContents = false;
> > +#endif
> 
> Barf!

Do you know of another way to do this?

[Kenneth Russell]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 90639 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=90639&action=review
> > 
> > Looks good to me.
> > 
> > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:64
> > > +#ifndef NDEBUG
> > > +bool LayerRendererChromium::s_inPaintContents = false;
> > > +#endif
> > 
> > Barf!
> 
> Do you know of another way to do this?

Minimally I'd scope the variable inside the LayerRendererChromium instance. But if the debugger makes it difficult to examine it in that case, don't worry about it.

[James Robinson]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 90639 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=90639&action=review
> > > 
> > > Looks good to me.
> > > 
> > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:64
> > > > +#ifndef NDEBUG
> > > > +bool LayerRendererChromium::s_inPaintContents = false;
> > > > +#endif
> > > 
> > > Barf!
> > 
> > Do you know of another way to do this?
> 
> Minimally I'd scope the variable inside the LayerRendererChromium instance. But if the debugger makes it difficult to examine it in that case, don't worry about it.

LayerChromiums are constructed without a LayerRendererChromium pointer, so that doesn't trivially work.

[James Robinson]

Committed r85112: <http://trac.webkit.org/changeset/85112>

[WebKit Review Bot]

http://trac.webkit.org/changeset/85112 might have broken Leopard Intel Debug (Build) and SnowLeopard Intel Release (Build)

[James Robinson]

Reverted r85112 for reason:

Broke mac compile

Committed r85116: <http://trac.webkit.org/changeset/85116>

[James Robinson]

FrameView.h is included from files in the WebKit project on Mac, so adding a header to FrameView.h requires adding that header to the forwarding headers.

Given that CompositingUpdateAfterLayoutOrStyleChange and CompositingUpdateOnPaintingOrHitTest in practice do exactly the same thing I think I'll just not add the parameter to FrameView::updateCompositingLayers()

[James Robinson]

Created attachment 91377 [details]
Patch

[James Robinson]

Take two - this patch only modifies chromium-specific code.

[WebKit Review Bot]

http://trac.webkit.org/changeset/85116 might have broken SnowLeopard Intel Release (Tests)

[Kenneth Russell]

Comment on attachment 91377 [details]
Patch

Looks fine to me.

[James Robinson]

Comment on attachment 91377 [details]
Patch

Clearing flags on attachment: 91377

Committed r85136: <http://trac.webkit.org/changeset/85136>

[James Robinson]

All reviewed patches have been landed.  Closing bug.