Bug 7180 - Table cell's anonymous wrappers are left in the tree, impacting our layout
Summary: Table cell's anonymous wrappers are left in the tree, impacting our layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 420+
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords: HasReduction
: 75798 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-02-10 12:46 PST by mitz
Modified: 2012-02-17 11:03 PST (History)
9 users (show)

See Also:


Attachments
Testcase (665 bytes, text/html)
2006-02-10 12:47 PST, mitz
no flags Details
Proposed fix: remove our table cell wrappers during destroy() in a new function destroyAndRemoveAnonymousWrapper. (7.17 KB, patch)
2012-01-24 10:57 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Moved the clean-up to removal time per suggestion. (5.26 KB, patch)
2012-01-26 15:38 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Same change, fixed the test's bug description. (5.27 KB, patch)
2012-01-27 13:06 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch updated after Eric's comment. Not for review, while I investigate more the destroying. (5.17 KB, patch)
2012-01-31 11:04 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Switched the strategy to removing anonymous wrapper at detach time as this is easier to generalize. (7.01 KB, patch)
2012-01-31 16:55 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Added a fast-path when destroying the document. (7.26 KB, patch)
2012-02-16 12:19 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Improved our clean-up code after Dave's IRC comments. (6.80 KB, patch)
2012-02-16 12:57 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2006-02-10 12:46:52 PST
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.
Comment 1 mitz 2006-02-10 12:47:55 PST
Created attachment 6397 [details]
Testcase
Comment 2 Julien Chaffraix 2012-01-24 08:45:47 PST
*** Bug 75798 has been marked as a duplicate of this bug. ***
Comment 3 Julien Chaffraix 2012-01-24 08:48:23 PST
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.
Comment 4 Julien Chaffraix 2012-01-24 10:57:18 PST
Created attachment 123771 [details]
Proposed fix: remove our table cell wrappers during destroy() in a new function destroyAndRemoveAnonymousWrapper.
Comment 5 WebKit Review Bot 2012-01-24 13:50:19 PST
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
Comment 6 Julien Chaffraix 2012-01-24 17:42:25 PST
(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.
Comment 7 Adam Barth 2012-01-24 17:43:46 PST
> 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.
Comment 8 Dave Hyatt 2012-01-25 10:38:56 PST
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?
Comment 9 Julien Chaffraix 2012-01-26 15:38:38 PST
Created attachment 124197 [details]
Moved the clean-up to removal time per suggestion.
Comment 10 Julien Chaffraix 2012-01-27 13:06:10 PST
Created attachment 124357 [details]
Same change, fixed the test's bug description.
Comment 11 Eric Seidel (no email) 2012-01-30 15:59:50 PST
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.
Comment 12 Julien Chaffraix 2012-01-30 18:28:48 PST
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.
Comment 13 Julien Chaffraix 2012-01-31 11:04:04 PST
Created attachment 124776 [details]
Patch updated after Eric's comment. Not for review, while I investigate more the destroying.
Comment 14 Eric Seidel (no email) 2012-01-31 11:08:47 PST
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()?
Comment 15 Eric Seidel (no email) 2012-01-31 11:10:50 PST
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.)
Comment 16 Julien Chaffraix 2012-01-31 14:09:35 PST
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.
Comment 17 Eric Seidel (no email) 2012-01-31 14:13:36 PST
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.
Comment 18 Julien Chaffraix 2012-01-31 16:55:38 PST
Created attachment 124852 [details]
Switched the strategy to removing anonymous wrapper at detach time as this is easier to generalize.
Comment 19 Julien Chaffraix 2012-02-01 09:28:43 PST
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.
Comment 20 Eric Seidel (no email) 2012-02-07 15:17:53 PST
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.
Comment 21 Julien Chaffraix 2012-02-08 08:39:01 PST
(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.
Comment 22 Julien Chaffraix 2012-02-16 12:19:47 PST
Created attachment 127426 [details]
Added a fast-path when destroying the document.
Comment 23 Julien Chaffraix 2012-02-16 12:57:43 PST
Created attachment 127431 [details]
Improved our clean-up code after Dave's IRC comments.
Comment 24 Dave Hyatt 2012-02-17 10:22:37 PST
Comment on attachment 127431 [details]
Improved our clean-up code after Dave's IRC comments.

r=me
Comment 25 Julien Chaffraix 2012-02-17 10:28:14 PST
Comment on attachment 127431 [details]
Improved our clean-up code after Dave's IRC comments.

Thanks Dave!
Comment 26 WebKit Review Bot 2012-02-17 11:02:53 PST
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>
Comment 27 WebKit Review Bot 2012-02-17 11:03:00 PST
All reviewed patches have been landed.  Closing bug.