Bug 72630 – [Chromium] Calls to paintContentsIfDirty() and updateCompositorResources() should be balanced.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Reveman]

Layer property changes during paintContent() can leave the layer in an invalid state as paintContentsIfDirty() has been called without a matching updateCompositorResources() call. Removing conditionals around these calls ensure they are balanced.

This change can result in unnecessary uploads but that can be properly solved using this change: https://bugs.webkit.org/show_bug.cgi?id=72192

[David Reveman]

Created attachment 115620 [details]
Patch

[Adrienne Walker]

Comment on attachment 115620 [details]
Patch

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

I wish there were some better way to assert that this never happened in practice.  Do you think it'd be a good idea to store the compositor frame that is being painted in TiledLayerChromium::prepareToUpdate and assert/early-out if the frame number is different during TiledLayerChromium::updateCompositorResources?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-339
> -    if (layer->drawsContent())
> -        layer->paintContentsIfDirty();

Can you leave the drawsContent() check both here and in updateCompositorResources? Otherwise, you're going to trip asserts like the ASSERT(drawsContent()) in VideoLayerChromium::updateCompositorResources.

[David Reveman]

Created attachment 115642 [details]
Patch

[David Reveman]

(In reply to comment #2)
> (From update of attachment 115620 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115620&action=review
> 
> I wish there were some better way to assert that this never happened in practice.  Do you think it'd be a good idea to store the compositor frame that is being painted in TiledLayerChromium::prepareToUpdate and assert/early-out if the frame number is different during TiledLayerChromium::updateCompositorResources?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-339
> > -    if (layer->drawsContent())
> > -        layer->paintContentsIfDirty();
> 
> Can you leave the drawsContent() check both here and in updateCompositorResources? Otherwise, you're going to trip asserts like the ASSERT(drawsContent()) in VideoLayerChromium::updateCompositorResources.

Realized this just after uploading the previous patch. I think my new patch handles this properly. We need to make sure that if one of these calls are made the other needs to be made as well. It will be easy to avoid both calls when 72192 has landed.

[James Robinson]

Comment on attachment 115642 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:338
>  static void paintContentsIfDirty(LayerChromium* layer)
>  {
> -    if (layer->drawsContent())
> -        layer->paintContentsIfDirty();
> +    layer->paintContentsIfDirty();

this helper isn't useful any more, can you just inline it?

[James Robinson]

Does https://bugs.webkit.org/show_bug.cgi?id=71038 simplify this?

[David Reveman]

Created attachment 115671 [details]
Patch

[David Reveman]

(In reply to comment #5)
> (From update of attachment 115642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115642&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:338
> >  static void paintContentsIfDirty(LayerChromium* layer)
> >  {
> > -    if (layer->drawsContent())
> > -        layer->paintContentsIfDirty();
> > +    layer->paintContentsIfDirty();
> 
> this helper isn't useful any more, can you just inline it?

Fixed.

[David Reveman]

(In reply to comment #6)
> Does https://bugs.webkit.org/show_bug.cgi?id=71038 simplify this?

That change is great as it makes this fix less likely to cause a performance regression.

[James Robinson]

Comment on attachment 115671 [details]
Patch

Looks great, R=me. Could you let Enne check it over before landing?

[Adrienne Walker]

Comment on attachment 115671 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 115671 [details]
Patch

Clearing flags on attachment: 115671

Committed r100702: <http://trac.webkit.org/changeset/100702>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.