Bug 7180 – Table cell's anonymous wrappers are left in the tree, impacting our layout

• Component: Tables
• Status: RESOLVED FIXED

Changing a table cell's position property to absolute replaces it with an anonymous cell containing a block. When the position is changed back to static, the block changes back to a cell, but the anonymous cell is left behind, so now there's an extra cell.

You can reproduce this in the testcase by changing the orange cell's position property from static to absolute a few times using the radio buttons.

Filing as P3 since this doesn't come from a real website.

Created attachment 6397 [details]
Testcase

[Julien Chaffraix]

*** Bug 75798 has been marked as a duplicate of this bug. ***

[Julien Chaffraix]

jQuery is impacted by this issue so bumping the priority to P2.

The issue is related to bug 52123: we leave anonymous cells wrapper in the tree that confuses our layout algorithm.

[Julien Chaffraix]

Created attachment 123771 [details]
Proposed fix: remove our table cell wrappers during destroy() in a new function destroyAndRemoveAnonymousWrapper.

[WebKit Review Bot]

Comment on attachment 123771 [details]
Proposed fix: remove our table cell wrappers during destroy() in a new function destroyAndRemoveAnonymousWrapper.

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

New failing tests:
media/audio-garbage-collect.html

[Julien Chaffraix]

(In reply to comment #5)
> (From update of attachment 123771 [details])
> Attachment 123771 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11294069
> 
> New failing tests:
> media/audio-garbage-collect.html

The EWS is confused here. This test is flaky and more importantly does not involve any table so could not be impacted by the change.

[Adam Barth]

> The EWS is confused here. This test is flaky and more importantly does not involve any table so could not be impacted by the change.

Yeah, I've marked the test as flaky.  Sorry for the noise.

[Dave Hyatt]

Comment on attachment 123771 [details]
Proposed fix: remove our table cell wrappers during destroy() in a new function destroyAndRemoveAnonymousWrapper.

Maybe it would be better to do this at removal time rather than destroy time?

[Julien Chaffraix]

Created attachment 124197 [details]
Moved the clean-up to removal time per suggestion.

[Julien Chaffraix]

Created attachment 124357 [details]
Same change, fixed the test's bug description.

[Eric Seidel (no email)]

Comment on attachment 124357 [details]
Same change, fixed the test's bug description.

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

> Source/WebCore/rendering/RenderTableCell.cpp:1099
> +    if (beingDestroyed() || documentBeingDestroyed())

When are we not being destroyed when our document is?  Do we need to check for other forms of destruction?  Does destroy() assert if you call it recursively (to catch people missing this check?)

> Source/WebCore/rendering/RenderTableCell.cpp:1104
> +        destroy();

is destory() the right way to remove ourselves from teh tree?  (intead of say parent()->removeChild(this)?) 

Why is the isBeforeOrAfterContent check needed?  Are there other types of anonymous blocks which we need to whitelist here?  This and the "documentBeingDestoryed()" check above seem like error-prone whitelists.

[Julien Chaffraix]

Comment on attachment 124357 [details]
Same change, fixed the test's bug description.

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

Eric, regarding your question about anonymous boxes removal, there is some code in RenderBlock::removeChild to merge / remove some anonymous child in some cases. However it is fairly limited and some testing showed that we leave some anonymous boxes in the tree.

>> Source/WebCore/rendering/RenderTableCell.cpp:1099
>> +    if (beingDestroyed() || documentBeingDestroyed())
> 
> When are we not being destroyed when our document is?  Do we need to check for other forms of destruction?  Does destroy() assert if you call it recursively (to catch people missing this check?)

> When are we not being destroyed when our document is?

I don't think there is any such case. If our document is destroyed, destroy() should have been called on |this| as we are going up the RenderTree (from child to parent in removeChild).

> Do we need to check for other forms of destruction?

I don't think there are other forms. Whenever a node is destroyed, willBeDestroyed will be called and it sets beingDestroyed() as RenderTableCells are RenderBlock.

> Does destroy() assert if you call it recursively (to catch people missing this check?)

Unfortunately it doesn't as beingDestroyed() is specific to RenderBlock. However any memory checker will start screaming.

>> Source/WebCore/rendering/RenderTableCell.cpp:1104
>> +        destroy();
> 
> is destory() the right way to remove ourselves from teh tree?  (intead of say parent()->removeChild(this)?) 
> 
> Why is the isBeforeOrAfterContent check needed?  Are there other types of anonymous blocks which we need to whitelist here?  This and the "documentBeingDestoryed()" check above seem like error-prone whitelists.

> is destory() the right way to remove ourselves from teh tree?  (intead of say parent()->removeChild(this)?)

destroy() will call parent()->removeChild() in RenderObject::willBeDestroyed. The difference is that destroy() also takes care of free'ing the memory and cleaning our tree. The right way to remove a node doesn't look very clear as sometimes we just remove(), other times we destroy()...

> Why is the isBeforeOrAfterContent check needed?

I was looking for the comment that prompted me not to poke into generated content but couldn't find it again. Looking again, generated content has special rules for removal but don't warrant a special case.

[Julien Chaffraix]

Created attachment 124776 [details]
Patch updated after Eric's comment. Not for review, while I investigate more the destroying.

[Eric Seidel (no email)]

Comment on attachment 124776 [details]
Patch updated after Eric's comment. Not for review, while I investigate more the destroying.

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

> Source/WebCore/rendering/RenderTableCell.cpp:1103
> +    // Clean up anonymous wrapper as it can confuse our layout algorithm.
> +    if (isAnonymous() && !firstChild())

This is specifically a check to see if we're an anonymous wrapper which is now empty.  It might be clearer to split this into a separate function and move this down onto RenderObject/RenderBoxModelObject?  Named something like destroyIfAnonymousAndAbandoned()?

[Eric Seidel (no email)]

Comment on attachment 124776 [details]
Patch updated after Eric's comment. Not for review, while I investigate more the destroying.

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

> Source/WebCore/rendering/RenderTableCell.cpp:1104
> +        destroy();

I assume that destroy() will properly clear the Node::m_renderer reference if any?  If so, how does that work for anonymous blocks in general?  Do both the anonymous wrapper and the real renderer point back to the node?  Which one is responsible for clearing the node's pointer to the renderer on death?  (I know these are basic rendering tree questions, but they will inform these sorts of refactorings so we should make sure we understand them perfectly.)

[Julien Chaffraix]

Comment on attachment 124776 [details]
Patch updated after Eric's comment. Not for review, while I investigate more the destroying.

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

>> Source/WebCore/rendering/RenderTableCell.cpp:1103
>> +    if (isAnonymous() && !firstChild())
> 
> This is specifically a check to see if we're an anonymous wrapper which is now empty.  It might be clearer to split this into a separate function and move this down onto RenderObject/RenderBoxModelObject?  Named something like destroyIfAnonymousAndAbandoned()?

Yes, that's exactly what this check is. It's possible to add a destroyIfAnonymousAndAbandoned() as this could become a rather common task in the render tree if we decide to really start removing anonymous parent agressively.

I realize more and more that I artificially put the scope at the RenderTableCell level but the scope could be a lot broader. We just have to be super careful as we are free'ing |this|.

>> Source/WebCore/rendering/RenderTableCell.cpp:1104
>> +        destroy();
> 
> I assume that destroy() will properly clear the Node::m_renderer reference if any?  If so, how does that work for anonymous blocks in general?  Do both the anonymous wrapper and the real renderer point back to the node?  Which one is responsible for clearing the node's pointer to the renderer on death?  (I know these are basic rendering tree questions, but they will inform these sorts of refactorings so we should make sure we understand them perfectly.)

There is no need to clear the Node::m_renderer as we are an anonymous RenderObject (which means that we have no associated DOM object). For anonymous wrappers, RenderObject::node() points to the Document.

The one clearing the node's pointer to the renderer is the DOM element itself - as part of detach() - as it is the one controlling the associated RenderObject's lifetime.

[Eric Seidel (no email)]

OK.  So I think its interesting to put this in a more general place, and make it a more general "delete me if it's time to do so" kind of method.

We don't have to move it onto Node, etc. yet, but we should at least design the helper with the idea that it might be moved there later.  The goal here being to make it simple to get this all right, and hard to get it wrong in future code.

Thanks again for looking at this.  I'm happy to r+ any patch along the lines of what we've discussed so far.  This isn't a very involved change.

[Julien Chaffraix]

Created attachment 124852 [details]
Switched the strategy to removing anonymous wrapper at detach time as this is easier to generalize.

[Julien Chaffraix]

Comment on attachment 124852 [details]
Switched the strategy to removing anonymous wrapper at detach time as this is easier to generalize.

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

> Source/WebCore/rendering/RenderObject.cpp:2254
> +    bool parentShouldDie = false;

Btw, this name - while cool - is not great. I think isParentLeftOverAnonymousWrapper would be a better fit. I will change it before landing the patch.

[Eric Seidel (no email)]

Comment on attachment 124852 [details]
Switched the strategy to removing anonymous wrapper at detach time as this is easier to generalize.

I wonder if this would slow down render tree destruction.  You should try and hunt down Hyatt in #webkit for his opinion on this patch.

[Julien Chaffraix]

(In reply to comment #20)
> (From update of attachment 124852 [details])
> I wonder if this would slow down render tree destruction. 

This is definitely a possibility especially at document destruction as we are trying to clean up the tree one RenderObject at a time. We could alleviate that by adding a fast path in this case.

[Julien Chaffraix]

Created attachment 127426 [details]
Added a fast-path when destroying the document.

[Julien Chaffraix]

Created attachment 127431 [details]
Improved our clean-up code after Dave's IRC comments.

[Dave Hyatt]

Comment on attachment 127431 [details]
Improved our clean-up code after Dave's IRC comments.

r=me

[Julien Chaffraix]

Comment on attachment 127431 [details]
Improved our clean-up code after Dave's IRC comments.

Thanks Dave!

[WebKit Review Bot]

Comment on attachment 127431 [details]
Improved our clean-up code after Dave's IRC comments.

Clearing flags on attachment: 127431

Committed r108098: <http://trac.webkit.org/changeset/108098>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.