Bug 86261

Created attachment 141514 [details] Fixes the patch

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

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).

(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!

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

Created attachment 141533 [details] Patch

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

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.

Closing the barn door? :)

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

(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.

Created attachment 142049 [details] Patch

(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 on attachment 142049 [details] Patch The change is super cool, I would love to see some testing for the <col> and <caption> cases.

(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.

(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.

(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.

Created attachment 143408 [details] Patch

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 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 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 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.

(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.

Created attachment 143426 [details] Patch

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 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

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 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 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 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.

(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.