Bug 107701 – Canvas and DOM go out of sync

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

This bug was reported in Chromium as Issue 169024, but it was reproduced in other browsers, so it's not a Chromium specific bug.

Go to the provided URL and drag the green square using the mouse. The red square will lag behind even though it should be in sync with the green square.
Removing the line "overflow: hidden;" from the "overlay" element solves the issue, so the "overflow: hidden;" line triggers this issue somehow.

Created attachment 184256 [details]
Patch

[WebKit Review Bot]

Comment on attachment 184256 [details]
Patch

Attachment 184256 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16073324

New failing tests:
svg/custom/text-xy-updates-SVGList.xhtml
svg/dynamic-updates/SVGUseElement-svgdom-href1-prop.html
svg/dynamic-updates/SVGUseElement-dom-href1-attr.html
svg/custom/circle-move-invalidation.svg
svg/custom/mask-invalidation.svg
fast/repaint/search-field-cancel.html
fast/repaint/moving-shadow-on-path.html
svg/transforms/animated-path-inside-transformed-html.xhtml
fast/repaint/inline-relative-positioned.html
svg/repaint/repaint-webkit-svg-shadow-container.html

I don't know whether or not it's the correct fix, but it does fix the issue. So, basically, here's the problem I found :

There are 2 different codepaths to doing a relayout :
1 ) If you have overflow:visible, then you may write anywhere in your parent's canvas, so your parent has to relayout too.
- Doing a relayout all the way up will trigger a relayout of the RenderView object and this will work because everything gets to have a relayout. So here, FrameView::scheduleRelayout() is called from RenderObject::scheduleRelayout(), which was called from RenderObject::markContainingBlocksForLayout().

2 ) In any other overflow mode, you clip your content to your own canvas, so no need for your parent to relayout, so we stop there. 
- So since we stop early in RenderObject::markContainingBlocksForLayout(), when we get to RenderObject::scheduleRelayout(), we'll actually call FrameView::scheduleRelayoutOfSubtree(). The fix I wrote is inside that function. It seems it wasn't recursively marking contained objects for relayout when m_layoutSchedulingEnabled is true, for some reason.

If the fix isn't the appropriate fix, please explain how the m_layoutSchedulingEnabled logic should work.

Thanks

[Dimitri Glazkov (Google)]

Comment on attachment 184256 [details]
Patch

One of the things that might be great is your fiddle converted into a layout test.

Also, the fix looks wrong -- you're simply invalidating "the whole thing". But I defer to paint experts to guide you here :)

(In reply to comment #4)
> (From update of attachment 184256 [details])
> One of the things that might be great is your fiddle converted into a layout test.

I have a layout test locally, I'll add it as soon as I know that it validates the "proper" fix.

> Also, the fix looks wrong -- you're simply invalidating "the whole thing". But I defer to paint experts to guide you here :)

I'm not sure I follow you about invalidating "the whole thing". The path where m_layoutSchedulingEnabled is true is the only path not calling "markContainingBlocksForLayout". Does that mean that when m_layoutSchedulingEnabled is false, we always invalidate "the whole thing" ?

Also, at the point where this function is called, m_layoutRoot points to the current object (in the reference URL, it would be pointing to the bottom right canvas of the page), which is the root of the subtree and we do want to invalidate the subtree, I believe, since we're about to render it.

[James Robinson]

Comment on attachment 184256 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2258
> +        m_layoutRoot->markContainingBlocksForLayout(false);

this basically disables the subtree relayout optimization.

The idea of the subtree layout optimization is it's applied when the only layout we need to do is within the subtree.  In your test case, likely one of the following is true:

1.) The subtree is incorrectly rooted (too low in the tree)
2.) The subtree optimization shouldn't be used in this instance
3.) The subtree optimization is correct, but something else is awry

I would wager it's 3.  This call will cover up many sins by causing us to relayout and reinvalidation pretty much the entire world every frame.

I think you should try to back out to the narrowest point that's misbehaving.  Perhaps something isn't marked as needing layout that should, or perhaps everything is being laid out correctly but something isn't generating correct invalidations (aka repaints in the RenderObject world)

Ok, at this point, I'm stuck, I need the input of someone more knowledgeable than me on the compositing tree.

It seems that the compositing tree is incompatible with partial (subtree) layout updates. Here's why :

When we call WebCore::RenderLayerModelObject::styleDidChange(), here's what happens :

1)
WebCore::RenderLayerModelObject::styleDidChange
WebCore::RenderObject::styleDidChange
WebCore::RenderObject::setNeedsPositionedMovementLayout
WebCore::RenderObject::markContainingBlocksForLayout
WebCore::RenderObject::scheduleRelayout 
WebCore::FrameView::scheduleRelayoutOfSubtree

2)
WebCore::RenderLayerModelObject::styleDidChange (same call as stack trace 1)
WebCore::RenderLayer::styleChanged
WebCore::RenderLayerCompositor::updateLayerCompositingState
WebCore::RenderLayerBacking::updateGraphicsLayerConfiguration
...
-> Eventually, we read a position value here and set the layer position

Unfortunately, the layer position is not updated yet. It will be in the next FrameView::layout() call when, after updating the position value by calling layout on the subtree root, we'll call "updateLayerPositionsAfterLayout()". That call has to happen before the we call updateLayerCompositingState(), otherwise, the layer position will be wrong.

I also get a weird behavior when I'm debugging a page that shows this bug. When I refresh it multiple times, I get different results, as if the timers were sometimes getting triggered in a different order.

I don't want to implement a solution where I'd call layout(), then updateLayerCompositingState(), then layout(), then updateLayerCompositingState() again, since that would be obviously sub-optimal.

I'll wait for someone to chime in and give me some sort of explanation about the expected relationship between the layout operation and the compositing tree before I continue my investigation.

Created attachment 185271 [details]
Patch

Ok, I tried this new patch to fix the issue. The original fix was breaking the subtree optimization, as James pointed out, so I tried to find something less aggressive, but the current fix may still be too much for what we need, I'll let the reviewers decide.

In this version of the fix, only accelerated compositing is affected and it is only affected when a layout call using the subtree optimization causes a layer to move. In that specific case, the position of all layers gets updated so that we don't miss any update before computing the compositing tree. It is slightly redundant, because the subtree will get its position updated twice, potentially, but it's probably better than to update the whole tree more frequently since we use the result of the 1st call to updateLayerPositionsAfterLayout() in order to know if a top layer call is necessary.

[Simon Fraser (smfr)]

(In reply to comment #7)
> Ok, at this point, I'm stuck, I need the input of someone more knowledgeable than me on the compositing tree.
> 
> It seems that the compositing tree is incompatible with partial (subtree) layout updates. Here's why :
> 
> When we call WebCore::RenderLayerModelObject::styleDidChange(), here's what happens :
> 
> 1)
> WebCore::RenderLayerModelObject::styleDidChange
> WebCore::RenderObject::styleDidChange
> WebCore::RenderObject::setNeedsPositionedMovementLayout
> WebCore::RenderObject::markContainingBlocksForLayout
> WebCore::RenderObject::scheduleRelayout 
> WebCore::FrameView::scheduleRelayoutOfSubtree
> 
> 2)
> WebCore::RenderLayerModelObject::styleDidChange (same call as stack trace 1)
> WebCore::RenderLayer::styleChanged
> WebCore::RenderLayerCompositor::updateLayerCompositingState
> WebCore::RenderLayerBacking::updateGraphicsLayerConfiguration
> ...
> -> Eventually, we read a position value here and set the layer position

This part sounds wrong; if we're inside styleChanged (and there's a pending layout), then we can't rely on any geometry information (since layout might change it). The code does do this right now, but what should happen is a subsequent compositing layer update after the next layout should fix it.  Is that happening?

[Simon Fraser (smfr)]

Comment on attachment 185271 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1256
> +    if (subtree && positionChanged) {
> +        RenderObject* topRoot = document->renderer();
> +        if (topRoot) {
> +            RenderLayer* topLayer = topRoot->enclosingLayer();
> +            topLayer->updateLayerPositionsAfterLayout(rootRenderer(this)->layer(),
> +                updateLayerPositionFlags(topLayer, false, m_doFullRepaint));
> +        }
> +    }

This doesnt' seem compositing-related. We rely on RenderLayer positions for invalidation etc too.

[Simon Fraser (smfr)]

Ah, the bug here is that
  layer = root->enclosingLayer();
in FrameView::layout() doesn't go high enough up the tree in this case; it needs to find the enclosing stacking container.

[Simon Fraser (smfr)]

So that should be

layer = root->enclosingLayer()-> enclosingStackingContainer();

Created attachment 185509 [details]
Patch

I tried to use Simon's solution, but it did not fix the issue. In the cases I've tested, unless we start at the document's root's layer, the bug is still present. The proposed solution works in the cases I've tested with and, unless calling updateLayerPositionsAfterLayout() on the top layer is a terribly time consuming operation, I think/hope this fix could be acceptable, because I currently have no alternative solution :) I'll investigate more if this doesn't work for you.

[Build Bot]

Comment on attachment 185509 [details]
Patch

Attachment 185509 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16218587

New failing tests:
fast/canvas/canvas-overflow-hidden-animation.html

[Simon Fraser (smfr)]

Comment on attachment 185509 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1197
> -        layer = root->enclosingLayer();
> +        layer = document->renderer()->enclosingLayer();

This is wrong.

You want layer = root->enclosingLayer()->enclosingStackingContainer().

> LayoutTests/ChangeLog:11
> +        * fast/canvas/canvas-overflow-hidden-animation.html: Added.

This test is not automatic or self-testing. It needs to detect the incorrect condition without the patch (either with some self-check, or by comparing against a reference).

Comment on attachment 185509 [details]
Patch

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

>> Source/WebCore/page/FrameView.cpp:1197
>> +        layer = document->renderer()->enclosingLayer();
> 
> This is wrong.
> 
> You want layer = root->enclosingLayer()->enclosingStackingContainer().

I agree that this is not what we discussed yesterday, but as I mentioned in the comments, "layer = root->enclosingLayer()->enclosingStackingContainer()" does not fix the issue.

[Simon Fraser (smfr)]

Hm, interesting. We should figure out why it does not work.

[Simon Fraser (smfr)]

(In reply to comment #15)
> I tried to use Simon's solution, but it did not fix the issue. In the cases I've tested, unless we start at the document's root's layer, the bug is still present. The proposed solution works in the cases I've tested with and, unless calling updateLayerPositionsAfterLayout() on the top layer is a terribly time consuming operation, I think/hope this fix could be acceptable, because I currently have no alternative solution :) I'll investigate more if this doesn't work for you.

You should use showLayerTree() in the debugger to see what's different between the layer tree between an update from the root, and an update from the enclosingStackingContainer.

Using showLayerTree() shows that both trees (the good one and the bad one) are identical after calling updateCompositingLayersAfterLayout(). There must be something (like a redraw or an invalidation) triggered only by calling updateCompositingLayersAfterLayout() on the top layer.

Created attachment 185551 [details]
Patch

So I have a new proposal for the fix. I was expecting this new code, or something equivalent, to be done within RenderLayerCompositor::updateCompositingLayers() somewhere, but it seems like there's no guarantee of that happening, so I added it directly in RenderLayer::updateLayerPositionsAfterLayout() since similar code is executed within RenderLayer::updateLayerPositions()

[Simon Fraser (smfr)]

Comment on attachment 185551 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:386
> +#if USE(ACCELERATED_COMPOSITING)
> +    if ((flags & UpdateCompositingLayers) && rootLayer && (rootLayer != this) && rootLayer->isComposited()) {
> +        RenderLayerBacking::UpdateAfterLayoutFlags updateFlags = RenderLayerBacking::CompositingChildrenOnly;
> +        if (flags & NeedsFullRepaintInBacking)
> +            updateFlags |= RenderLayerBacking::NeedsFullRepaint;
> +        if (flags & IsCompositingUpdateRoot)
> +            updateFlags |= RenderLayerBacking::IsUpdateRoot;
> +        rootLayer->backing()->updateAfterLayout(updateFlags);
> +    }

What does this do that the later updateCompositingLayersAfterLayout() call in FrameView::layout() does not?

Your changelog needs to convince me that you understand the underlying cause for this bug. This feels like wallpapering over the cracks.

Created attachment 185769 [details]
Patch

Comment on attachment 185769 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:380
> +        if (RenderLayer* compLayer = enclosingCompositingLayer()) {

@smfr: You were right, the enclosingCompositingLayer() function returns the proper layer, no need to use the root layer directly. They were probably the same in my test cases, which is why these cases required the root layer (also, I didn't know about the enclosingCompositingLayer() function), but this code also fixes the issue.

[Build Bot]

Comment on attachment 185769 [details]
Patch

Attachment 185769 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16251416

New failing tests:
fast/canvas/canvas-overflow-hidden-animation.html

[Simon Fraser (smfr)]

Comment on attachment 185769 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:390
> +#if USE(ACCELERATED_COMPOSITING)
> +    if ((flags & UpdateCompositingLayers) && (rootLayer != this)) {
> +        if (RenderLayer* compLayer = enclosingCompositingLayer()) {
> +            RenderLayerBacking::UpdateAfterLayoutFlags updateFlags =
> +                RenderLayerBacking::CompositingChildrenOnly;
> +            if (flags & NeedsFullRepaintInBacking)
> +                updateFlags |= RenderLayerBacking::NeedsFullRepaint;
> +            if (flags & IsCompositingUpdateRoot)
> +                updateFlags |= RenderLayerBacking::IsUpdateRoot;
> +            compLayer->backing()->updateAfterLayout(updateFlags);
> +        }
> +    }
> +#endif

Does it not work just to start the updateLayerPositions() at the enclosingCompositingLayer()? I think add new updateAfterLayout() calls here is going to result in extra work in many more cases.

(In reply to comment #28)
> (From update of attachment 185769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185769&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:390
> > +#if USE(ACCELERATED_COMPOSITING)
> > +    if ((flags & UpdateCompositingLayers) && (rootLayer != this)) {
> > +        if (RenderLayer* compLayer = enclosingCompositingLayer()) {
> > +            RenderLayerBacking::UpdateAfterLayoutFlags updateFlags =
> > +                RenderLayerBacking::CompositingChildrenOnly;
> > +            if (flags & NeedsFullRepaintInBacking)
> > +                updateFlags |= RenderLayerBacking::NeedsFullRepaint;
> > +            if (flags & IsCompositingUpdateRoot)
> > +                updateFlags |= RenderLayerBacking::IsUpdateRoot;
> > +            compLayer->backing()->updateAfterLayout(updateFlags);
> > +        }
> > +    }
> > +#endif
> 
> Does it not work just to start the updateLayerPositions() at the enclosingCompositingLayer()? I think add new updateAfterLayout() calls here is going to result in extra work in many more cases.

Sure, it should work, but since this added code is called within updateLayerPositions(), around line 463, I thought it would be less costly to just call these lines rather than the whole function. I can easily change it to call the whole function, but I don't fully understand how that would affect performance positively.

[WebKit Review Bot]

Comment on attachment 185769 [details]
Patch

Attachment 185769 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16273326

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-overflow-hidden-animation.html

I redid my tests after seeing the failures and I made a mistake while doing my last tests. The current patch (using enclosingCompositingLayer()) doesn't work. Only the 4th patch, using the root layer, works, so it seems I'm back to patch number 4 as the latest working solution.

[WebKit Review Bot]

Comment on attachment 185769 [details]
Patch

Attachment 185769 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16269354

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-overflow-hidden-animation.html

Created attachment 185846 [details]
Patch

This new patch only does the missing geometry update for the current compositor, only for composited children and nothing else, so it may be a bit more efficient then the previous patch, and this also fixes the issue.

Created attachment 188179 [details]
Patch

I found a way to fix the bug using the enclosingStackingContainer().

The issue was that, even if we would use the enclosingStackingContainer() as the root of the update inside RenderLayer, if that container isn't itself composited, it wouldn't trigger any updates in the accelerated compositing section, which is why the original solution using enclosingStackingContainer() failed.

We do need to update from the enclosingStackingContainer() because only a stacking container can update the Z Order lists inside RenderLayerCompositor::updateCompositingDescendantGeometry() and these need to be updated, even on a subtree relayout.

The current fix should hopefully be less costly than the original proposition, but still do the proper updates.

[Simon Fraser (smfr)]

Comment on attachment 188179 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:465
>          if (flags & IsUpdateRoot) {
>              updateGraphicsLayerGeometry();
>              layerCompositor->updateRootLayerPosition();
> +            RenderLayer* stackingContainer = m_owningLayer->enclosingStackingContainer();
> +            if (!layerCompositor->compositingLayersNeedRebuild() && stackingContainer && (stackingContainer != m_owningLayer))
> +                layerCompositor->updateCompositingDescendantGeometry(stackingContainer, stackingContainer, flags & CompositingChildrenOnly);

So for the root layer, this will do both the updateCompositingDescendantGeometry() a few lines above, and then do another update from the SC ancestor?

> So for the root layer, this will do both the updateCompositingDescendantGeometry() a few lines above, and then do another update from the SC ancestor?

I don't think so, it won't do anything if stackingContainer == m_owningLayer, so if m_owningLayer is root, then its stacking container should either be itself or NULL (I don't know if NULL is possible for the root layer).

(In reply to comment #38)
> > So for the root layer, this will do both the updateCompositingDescendantGeometry() a few lines above, and then do another update from the SC ancestor?
> 
> I don't think so, it won't do anything if stackingContainer == m_owningLayer, so if m_owningLayer is root, then its stacking container should either be itself or NULL (I don't know if NULL is possible for the root layer).

But if you meant root as in m_owningLayer which is not the root layer, then yes, what you said is correct. Note that this is already the case as this function is called recursively from leaves to root, so updateCompositingDescendantGeometry() is already called in that fashion (children first, parents afterwards).

What I'm describing is from the line :

backing()->updateAfterLayout(updateFlags);

(approximately line 464 in file RenderLayer.cpp)

Friendly ping to reviewers. There's been no activity on this cl for more than a week, now. It's only 3 lines of code :)

[Eric Seidel (no email)]

jamesr or smfr are your best reviewers.  I would need someone to bring me up to speed on current RenderLayer thinking before reviewing.

[Simon Fraser (smfr)]

Comment on attachment 188179 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayerBacking.cpp:465
>> +                layerCompositor->updateCompositingDescendantGeometry(stackingContainer, stackingContainer, flags & CompositingChildrenOnly);
> 
> So for the root layer, this will do both the updateCompositingDescendantGeometry() a few lines above, and then do another update from the SC ancestor?

Actually I think this is fine.

[WebKit Review Bot]

Comment on attachment 188179 [details]
Patch

Clearing flags on attachment: 188179

Committed r144674: <http://trac.webkit.org/changeset/144674>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.