Bug 86261 - Removing the last child of an anonymous table or table section leaves the anonymous wrapper
Summary: Removing the last child of an anonymous table or table section leaves the ano...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL: http://code.google.com/p/chromium/iss...
Keywords:
Depends on: 86671
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-11 15:33 PDT by Levi Weintraub
Modified: 2012-10-02 18:13 PDT (History)
8 users (show)

See Also:


Attachments
test case (204 bytes, text/html)
2012-05-11 15:33 PDT, Levi Weintraub
no flags Details
Fixes the patch (632 bytes, patch)
2012-05-11 15:39 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (4.04 KB, patch)
2012-05-11 17:23 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (4.17 KB, patch)
2012-05-15 14:11 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (67.44 KB, patch)
2012-05-22 17:19 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (28.21 KB, patch)
2012-05-22 18:25 PDT, Levi Weintraub
jchaffraix: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (528.41 KB, application/zip)
2012-05-22 20:29 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2012-05-11 15:33:22 PDT
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.
Comment 1 Levi Weintraub 2012-05-11 15:39:59 PDT
Created attachment 141514 [details]
Fixes the patch
Comment 2 Levi Weintraub 2012-05-11 15:41:30 PDT
(In reply to comment #1)
> Created an attachment (id=141514) [details]
> Fixes the patch

Fixes the *crash* even ;)
Comment 3 Julien Chaffraix 2012-05-11 15:47:09 PDT
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).
Comment 4 Levi Weintraub 2012-05-11 15:47:54 PDT
(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!
Comment 5 Julien Chaffraix 2012-05-11 15:52:37 PDT
See bug 52123 for the CSS tests are failing due to us not cleaning up our anonymous wrappers.
Comment 6 Levi Weintraub 2012-05-11 17:23:44 PDT
Created attachment 141533 [details]
Patch
Comment 7 Levi Weintraub 2012-05-14 10:57:08 PDT
(In reply to comment #6)
> Created an attachment (id=141533) [details]
> Patch

Ping :)
Comment 8 Julien Chaffraix 2012-05-14 11:11:42 PDT
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.
Comment 9 Robert Hogan 2012-05-15 11:20:39 PDT
Closing the barn door? :)
Comment 10 Abhishek Arya 2012-05-15 11:38:04 PDT
(In reply to comment #9)
> Closing the barn door? :)

Why is this marked security ? The crash stack is speaking null ptr crash,
Comment 11 Robert Hogan 2012-05-15 12:03:57 PDT
(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.
Comment 12 Levi Weintraub 2012-05-15 14:11:06 PDT
Created attachment 142049 [details]
Patch
Comment 13 Levi Weintraub 2012-05-15 14:13:09 PDT
(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.
Comment 14 Julien Chaffraix 2012-05-15 16:23:47 PDT
Comment on attachment 142049 [details]
Patch

The change is super cool, I would love to see some testing for the <col> and <caption> cases.
Comment 15 Levi Weintraub 2012-05-16 10:50:47 PDT
(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.
Comment 16 Julien Chaffraix 2012-05-16 12:06:54 PDT
(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.
Comment 17 Levi Weintraub 2012-05-16 18:20:16 PDT
(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.
Comment 18 Levi Weintraub 2012-05-22 17:19:29 PDT
Created attachment 143408 [details]
Patch
Comment 19 WebKit Review Bot 2012-05-22 17:24:33 PDT
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.
Comment 20 Julien Chaffraix 2012-05-22 17:34:58 PDT
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.
Comment 21 Levi Weintraub 2012-05-22 17:50:04 PDT
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!
Comment 22 Julien Chaffraix 2012-05-22 18:11:56 PDT
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.
Comment 23 Levi Weintraub 2012-05-22 18:15:15 PDT
(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.
Comment 24 Levi Weintraub 2012-05-22 18:25:52 PDT
Created attachment 143426 [details]
Patch
Comment 25 WebKit Review Bot 2012-05-22 18:28:59 PDT
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.
Comment 26 WebKit Review Bot 2012-05-22 20:29:48 PDT
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
Comment 27 WebKit Review Bot 2012-05-22 20:29:53 PDT
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
Comment 28 Julien Chaffraix 2012-05-22 20:42:19 PDT
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).
Comment 29 Levi Weintraub 2012-05-22 22:48:23 PDT
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
Comment 30 Julien Chaffraix 2012-05-23 07:07:11 PDT
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.
Comment 31 Levi Weintraub 2012-05-23 10:34:01 PDT
(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.