Removing the last child of an anonymous table or table section leaves the anonymous wrapper

• Component: Tables
• Status: NEW ---

[Levi Weintraub]

Created attachment 141513 [details]
test case

When an anonymous RenderTableSection is added to a RenderTable then stripped of all its children, it continues to return that is has 1 column due to http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderTableSection.cpp#L1273

This causes a crash/assert in FixedTableLayout::layout when values from m_width are used, but the vector is empty.

Adding code to RenderTableSection::removeChild to also remove itself in the event that its anonymous and all its children are removed solves the bug without regressing, but I'm unsure if it's the best solution.  I'll upload my patch after this for comment.

[Levi Weintraub]

Created attachment 141514 [details]
Fixes the patch

[Levi Weintraub]

(In reply to comment #1)
> Created an attachment (id=141514) [details]
> Fixes the patch

Fixes the *crash* even ;)

[Julien Chaffraix]

Comment on attachment 141514 [details]
Fixes the patch

That's not the way to go sir!
There is a function called RenderObject::destroyAndCleanupAnonymousWrappers() which should be extended to handle this case. Currently there is a specific check against table-cell but it is artificial. Loosen it and you have your fix (on top of fixing one CSS test case IIRC).

[Levi Weintraub]

(In reply to comment #3)
> (From update of attachment 141514 [details])
> That's not the way to go sir!
> There is a function called RenderObject::destroyAndCleanupAnonymousWrappers() which should be extended to handle this case. Currently there is a specific check against table-cell but it is artificial. Loosen it and you have your fix (on top of fixing one CSS test case IIRC).

Woohoo! Proper fix FTW!

[Julien Chaffraix]

See bug 52123 for the CSS tests are failing due to us not cleaning up our anonymous wrappers.

[Levi Weintraub]

Created attachment 141533 [details]
Patch

[Levi Weintraub]

(In reply to comment #6)
> Created an attachment (id=141533) [details]
> Patch

Ping :)

[Julien Chaffraix]

Comment on attachment 141533 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:2342
>      // Currently we only remove anonymous cells' wrapper but we should remove all unneeded
>      // wrappers. See http://webkit.org/b/52123 as an example where this is needed.

Please update the comment as this is not accurate anymore.

> Source/WebCore/rendering/RenderObject.cpp:2343
> +    if (parent->isTableCell() || parent->isTableRow() || parent->isTableSection())

I would use isTablePart() here instead of starting to special case only parts of the table. This means we should include more coverage but I would expect those elements to display some issues too.

Also something I would like tested is a generated (anonymous) table or table part. In your example, it would be something like:
.a:after { display: table-row; }

> LayoutTests/fast/table/crash-anonymous-tablesection-not-removed.html:4
> +This tests that removing the children of an anonymous table section doesn't trigger a crash on relayout. Passes if there's no crash.

Please include the bug id and description here so that it appears in the output.

[Robert Hogan]

Closing the barn door? :)

[Abhishek Arya]

(In reply to comment #9)
> Closing the barn door? :)

Why is this marked security ? The crash stack is speaking null ptr crash,

[Robert Hogan]

(In reply to comment #10)
> (In reply to comment #9)
> > Closing the barn door? :)
> 
> Why is this marked security ? The crash stack is speaking null ptr crash,

Is it too late to pretend that I wasn't aware of the difference? I thought not.

[Levi Weintraub]

Created attachment 142049 [details]
Patch

[Levi Weintraub]

(In reply to comment #8)
> (From update of attachment 141533 [details])
> Also something I would like tested is a generated (anonymous) table or table part. In your example, it would be something like:
> .a:after { display: table-row; }

Though it doesn't cause crashes, we don't execute RenderObject::destroyAndCleanupAnonymousWrappers for generated content, since this method is triggered by Node::detach.

[Julien Chaffraix]

Comment on attachment 142049 [details]
Patch

The change is super cool, I would love to see some testing for the <col> and <caption> cases.

[Levi Weintraub]

(In reply to comment #14)
> (From update of attachment 142049 [details])
> The change is super cool, I would love to see some testing for the <col> and <caption> cases.

To test caption, one creates a caption box not parented in a table and an anonymous table box is created. However the fix attached to this bug that you recommended uses isTablePart, which includes table cells, columns, captions, rows, and sections, but *not* tables themselves. I suspect we want to remove tables as well. Agreed?

Columns *would* have the same issue, since the CSS2 spec specifies they should also be wrapped in an anonymous table, but we seem to have a separate bug where we currently will parent a RenderTableCol directly in the body's renderer.

[Julien Chaffraix]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 142049 [details] [details])
> > The change is super cool, I would love to see some testing for the <col> and <caption> cases.
> 
> To test caption, one creates a caption box not parented in a table and an anonymous table box is created. However the fix attached to this bug that you recommended uses isTablePart, which includes table cells, columns, captions, rows, and sections, but *not* tables themselves. I suspect we want to remove tables as well. Agreed?

Good point, I wasn't going to ask you to go this far but I don't mind if you do indeed.

> Columns *would* have the same issue, since the CSS2 spec specifies they should also be wrapped in an anonymous table, but we seem to have a separate bug where we currently will parent a RenderTableCol directly in the body's renderer.

OK, you can file a follow-up bug about that as I think this is a bug per CSS 2.1.

[Levi Weintraub]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (From update of attachment 142049 [details] [details] [details])
> > > The change is super cool, I would love to see some testing for the <col> and <caption> cases.
> > 
> > To test caption, one creates a caption box not parented in a table and an anonymous table box is created. However the fix attached to this bug that you recommended uses isTablePart, which includes table cells, columns, captions, rows, and sections, but *not* tables themselves. I suspect we want to remove tables as well. Agreed?
> 
> Good point, I wasn't going to ask you to go this far but I don't mind if you do indeed.
> 
> > Columns *would* have the same issue, since the CSS2 spec specifies they should also be wrapped in an anonymous table, but we seem to have a separate bug where we currently will parent a RenderTableCol directly in the body's renderer.
> 
> OK, you can file a follow-up bug about that as I think this is a bug per CSS 2.1.

I'm starting with that so I can include it in the test for this one.

[Levi Weintraub]

Created attachment 143408 [details]
Patch

[WebKit Review Bot]

Attachment 143408 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/qt/test_expectations.txt:19:  inspector/styles/override-screen-size.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/qt/test_expectations.txt:61:  ietestcenter/css3/valuesandunits/units-000.htm is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/qt/test_expectations.txt:73:  fast/borders/border-antialiasing.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/qt/test_expectations.txt:77:  tables/mozilla/bugs/bug10296-1.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/qt/test_expectations.txt:83:  editing/inserting/typing-space-to-trigger-smart-link.html is also in a Skipped file.  [test/expectations] [5]
WARNING: skia test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/skia/skia_test_expectations.txt' does not exist
WARNING: chromium test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/webkit/tools/layout_tests/test_expectations.txt' does not exist
LayoutTests/platform/mac/test_expectations.txt:161:  fast/inline/continuation-outlines-with-layers.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/mac/test_expectations.txt:170:  tables/mozilla_expected_failures/collapsing_borders/bug41262-5.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/mac/test_expectations.txt:219:  ietestcenter/css3/valuesandunits/units-000.htm is also in a Skipped file.  [test/expectations] [5]
Total errors found: 8 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Chaffraix]

Comment on attachment 143408 [details]
Patch

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

> LayoutTests/fast/table/anonymous-table-wrapper-cleanup.html:2
> +<html>

The text makes the test platform-specific which makes Bob sad.

Some nifty tricks:
* change your HTML tag to be <html style="font: 10px Ahem; -webkit-font-smoothing: none;">, possibly make the text white to avoid any visual output as this is of no concern to us.
* make all your text non-visible visually using a comment, a <title>, a meta-tag or display: none.

In this case, I would go for the option one as the instruction would be in the text output.

Also it would be nice to add a non-DRT popup as this test is very tied to DRT.

[Levi Weintraub]

Comment on attachment 143408 [details]
Patch

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

>> LayoutTests/fast/table/anonymous-table-wrapper-cleanup.html:2
>> +<html>
> 
> The text makes the test platform-specific which makes Bob sad.
> 
> Some nifty tricks:
> * change your HTML tag to be <html style="font: 10px Ahem; -webkit-font-smoothing: none;">, possibly make the text white to avoid any visual output as this is of no concern to us.
> * make all your text non-visible visually using a comment, a <title>, a meta-tag or display: none.
> 
> In this case, I would go for the option one as the instruction would be in the text output.
> 
> Also it would be nice to add a non-DRT popup as this test is very tied to DRT.

What we *really* need is a way to dump the DRT output without Image results :( Ahem will do...

I don't believe we can do a non-drt popup, as there (at least as far as I know) is no way to query for anonymous renderers from the DOM! It'd be very strange if there were!

[Julien Chaffraix]

Comment on attachment 143408 [details]
Patch

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

>>> LayoutTests/fast/table/anonymous-table-wrapper-cleanup.html:2
>>> +<html>
>> 
>> The text makes the test platform-specific which makes Bob sad.
>> 
>> Some nifty tricks:
>> * change your HTML tag to be <html style="font: 10px Ahem; -webkit-font-smoothing: none;">, possibly make the text white to avoid any visual output as this is of no concern to us.
>> * make all your text non-visible visually using a comment, a <title>, a meta-tag or display: none.
>> 
>> In this case, I would go for the option one as the instruction would be in the text output.
>> 
>> Also it would be nice to add a non-DRT popup as this test is very tied to DRT.
> 
> What we *really* need is a way to dump the DRT output without Image results :( Ahem will do...
> 
> I don't believe we can do a non-drt popup, as there (at least as far as I know) is no way to query for anonymous renderers from the DOM! It'd be very strange if there were!

True but that wouldn't solve your problem here. The TEXT dump would still be platform-specific so you would have to use Ahem anyway.

By non-drt popup, I meant:

if (!window.layoutTestController)
    alert("This test is meant to be run under DRT. There is no way to test it manually as you need access to a RenderTree dump.");

Sorry for being unclear.

[Levi Weintraub]

(In reply to comment #22)
> > What we *really* need is a way to dump the DRT output without Image results :( Ahem will do...
> > 
> > I don't believe we can do a non-drt popup, as there (at least as far as I know) is no way to query for anonymous renderers from the DOM! It'd be very strange if there were!
> 
> True but that wouldn't solve your problem here. The TEXT dump would still be platform-specific so you would have to use Ahem anyway.

Yes, but I'm much happier with just platform-specific text files than *also* image results we don't care for!

> if (!window.layoutTestController)
>     alert("This test is meant to be run under DRT. There is no way to test it manually as you need access to a RenderTree dump.");
> 
> Sorry for being unclear.

That clears it up! Thanks, sir :)

I'll re-upload momentarily.

[Levi Weintraub]

Created attachment 143426 [details]
Patch

[WebKit Review Bot]

Attachment 143426 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/qt/test_expectations.txt:19:  inspector/styles/override-screen-size.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/qt/test_expectations.txt:61:  ietestcenter/css3/valuesandunits/units-000.htm is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/qt/test_expectations.txt:73:  fast/borders/border-antialiasing.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/qt/test_expectations.txt:77:  tables/mozilla/bugs/bug10296-1.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/qt/test_expectations.txt:83:  editing/inserting/typing-space-to-trigger-smart-link.html is also in a Skipped file.  [test/expectations] [5]
WARNING: skia test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/skia/skia_test_expectations.txt' does not exist
WARNING: chromium test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/webkit/tools/layout_tests/test_expectations.txt' does not exist
LayoutTests/platform/mac/test_expectations.txt:161:  fast/inline/continuation-outlines-with-layers.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/mac/test_expectations.txt:170:  tables/mozilla_expected_failures/collapsing_borders/bug41262-5.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/mac/test_expectations.txt:219:  ietestcenter/css3/valuesandunits/units-000.htm is also in a Skipped file.  [test/expectations] [5]
Total errors found: 8 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 143426 [details]
Patch

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

New failing tests:
fast/table/table-section-node-at-point-crash.html

[WebKit Review Bot]

Created attachment 143442 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Julien Chaffraix]

Comment on attachment 143426 [details]
Patch

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

The queues have been acting weird on your previous attachments. Assuming you double-check that the failure is not a real regression, r=me.

> LayoutTests/ChangeLog:20
> +        * platform/chromium/test_expectations.txt:
> +        * platform/efl/test_expectations.txt:
> +        * platform/gtk/test_expectations.txt:
> +        * platform/mac-snowleopard/fast/table/anonymous-table-wrapper-cleanup-expected.png: Added.
> +        * platform/mac-snowleopard/fast/table/anonymous-table-wrapper-cleanup-expected.txt: Added.
> +        * platform/mac/test_expectations.txt:
> +        * platform/qt/test_expectations.txt:

No need for all this complicated stuff: using Ahem, font size and no font-smoothing, your result should be cross-platform so just move the anonymous-table-wrapper-cleanup-expected.* files next to the test and no need to touch any test_expectation.txt (on top of that you forgot win).

[Levi Weintraub]

Comment on attachment 143426 [details]
Patch

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

>> LayoutTests/ChangeLog:20
>> +        * platform/qt/test_expectations.txt:
> 
> No need for all this complicated stuff: using Ahem, font size and no font-smoothing, your result should be cross-platform so just move the anonymous-table-wrapper-cleanup-expected.* files next to the test and no need to touch any test_expectation.txt (on top of that you forgot win).

I most certainly did *not* forget Win. It doesn't support test_expectations.txt: https://bugs.webkit.org/show_bug.cgi?id=38756

[Julien Chaffraix]

Comment on attachment 143426 [details]
Patch

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

>>> LayoutTests/ChangeLog:20
>>> +        * platform/qt/test_expectations.txt:
>> 
>> No need for all this complicated stuff: using Ahem, font size and no font-smoothing, your result should be cross-platform so just move the anonymous-table-wrapper-cleanup-expected.* files next to the test and no need to touch any test_expectation.txt (on top of that you forgot win).
> 
> I most certainly did *not* forget Win. It doesn't support test_expectations.txt: https://bugs.webkit.org/show_bug.cgi?id=38756

OK, you did not forget win but you might have turned the bot red nevertheless. For win, you need to edit the Skipped file, sir (also some other ports prefer Skipped files over test_expectations.txt but let's not split hairs here)

> LayoutTests/fast/table/anonymous-table-wrapper-cleanup.html:23
> +</div>

I didn't see that you missed 2 cases: table-column-group and table-caption. As a whole, I think you could just get away with just testing one display at time without wrapping them but that's fine as-is too.

[Levi Weintraub]

(In reply to comment #28)
> (From update of attachment 143426 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143426&action=review
> 
> The queues have been acting weird on your previous attachments. Assuming you double-check that the failure is not a real regression, r=me.

This patch exposes a separate rendering issue in fast/table/table-section-node-at-point-crash.html. I've verified that no unexpected nodes are removed, and the render tree looks as expected, but without the addition of another block, the RenderText result isn't properly layed out and rendered. I'm going to look into this before landing this.