Bug 67007

Summary: r93794 results in ruby tests causing assertion failure in RenderBlock.cpp
Product: WebKit Reporter: Peter Kasting <pkasting>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED CONFIGURATION CHANGED    
Severity: Normal CC: ahmad.saleem792, bdakin, dglazkov, hyatt, inferno, koivisto, mitz, pnormand, rniwa, rolandsteiner, simon.fraser, tkent, webkit.review.bot, zalan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch to disable the tests for now
none
Patch inferno: review-, webkit.review.bot: commit-queue-

Peter Kasting
Reported 2011-08-25 17:58:52 PDT
See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Fruby%2Fafter-block-doesnt-crash.html%2Cfast%2Fruby%2Fafter-table-doesnt-crash.html%2Cfast%2Fruby%2Fgenerated-after-counter-doesnt-crash.html%2Cfast%2Fruby%2Fgenerated-before-and-after-counter-doesnt-crash.html . Four ruby tests are causing this assertion failure: ASSERTION FAILED: anonymousChild->isTable() third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp(698) : virtual void WebCore::RenderBlock::addChildIgnoringAnonymousColumnBlocks(WebCore::RenderObject*, WebCore::RenderObject*) Here's a stack: WebCore::RenderBlock::addChildIgnoringAnonymousColumnBlocks() [0x16c1b7f] WebCore::RenderBlock::addChildIgnoringContinuation() [0x16c220c] WebCore::RenderBlock::addChild() [0x16c214e] WebCore::RenderRubyAsBlock::addChild() [0x17aefeb] WebCore::NodeRendererFactory::createRendererIfNeeded() [0xe5e358] WebCore::Node::createRendererIfNeeded() [0xe44edb] WebCore::Text::attach() [0xe92804] WebCore::ContainerNode::attach() [0xdc4e2f] WebCore::Element::attach() [0xe1b8b0] WebCore::Node::reattach() [0xdbff60] WebCore::Element::recalcStyle() [0xe1bff9] WebCore::Element::recalcStyle() [0xe1c624] I suspect this is a pre-existing bug in the rubythat is merely exposed by this change. Because the change was probably a security fix and I don't want to roll it out + the rebaselines that landed later, I'm going to instead mark it as an expected failure for Chromium (and skip it for other ports).
Attachments
Patch to disable the tests for now (8.06 KB, patch)
2011-09-01 12:07 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (60.45 KB, patch)
2011-09-27 05:06 PDT, Roland Steiner 		inferno: review-
webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Peter Kasting
Comment 1 2011-08-25 18:06:04 PDT
Skipped these for all platforms in r93846. Please revert that expectations change once this is fixed.
Abhishek Arya
Comment 2 2011-08-25 21:38:39 PDT
Peter, thanks for skipping this. I was going to do that too. Basically, ruby tags are not able to handle :before and :after properly. my change exposes the incorrect behavior. The current tests should be rendertree tests (and not just crashing tests). This will tell that ruby :after child with table display are created at the right place. Roland, will you have some time to take a closer look at this.
Abhishek Arya
Comment 3 2011-08-25 22:26:30 PDT
See the renderings below before my patch. <html> <style type="text/css"> ruby { float: left; } ruby::after{ display: table; content: "AFTER"; } </style> <ruby>CONTENT</ruby> Content-Type: text/plain layer at (0,0) size 800x600 RenderView at (0,0) size 800x600 layer at (0,0) size 800x600 RenderBlock {HTML} at (0,0) size 800x600 RenderBody {BODY} at (8,8) size 784x584 RenderRuby (block) {RUBY} at (0,0) size 129x22 RenderBlock (generated) at (0,0) size 52x18 RenderTable at (0,0) size 52x18 RenderTableSection (anonymous) at (0,0) size 52x18 RenderTableRow (anonymous) at (0,0) size 52x18 RenderTableCell (anonymous) at (0,0) size 52x18 [r=0 c=0 rs=1 cs=1] RenderText at (0,0) size 52x18 text run at (0,0) width 52: "AFTER" RenderRubyRun (anonymous) at (52,4) size 77x18 RenderRubyBase (anonymous) at (0,0) size 77x18 RenderText {#text} at (0,0) size 77x18 text run at (0,0) width 77: "CONTENT" the after child is inccorectly placed before content.
Abhishek Arya
Comment 4 2011-08-25 23:02:04 PDT
after my patch, rendering is correct (even though assert fails telling that we need to pass this properly through the renderruby specific machinery(like tables, see next condition after the assert) to handle these :after child) layer at (0,0) size 800x600 RenderView at (0,0) size 800x600 layer at (0,0) size 800x600 RenderBlock {HTML} at (0,0) size 800x600 RenderBody {BODY} at (8,8) size 784x584 RenderRuby (block) {RUBY} at (0,0) size 129x22 RenderRubyRun (anonymous) at (0,4) size 77x18 RenderRubyBase (anonymous) at (0,0) size 77x18 RenderText {#text} at (0,0) size 77x18 text run at (0,0) width 77: "CONTENT" RenderBlock (generated) at (77,0) size 52x18 RenderTable at (0,0) size 52x18 RenderTableSection (anonymous) at (0,0) size 52x18 RenderTableRow (anonymous) at (0,0) size 52x18 RenderTableCell (anonymous) at (0,0) size 52x18 [r=0 c=0 rs=1 cs=1] RenderText at (0,0) size 52x18 text run at (0,0) width 52: "AFTER"
Roland Steiner
Comment 5 2011-08-25 23:11:16 PDT
(In reply to comment #4) > after my patch, rendering is correct (even though assert fails telling that we need to pass this properly through the renderruby specific machinery(like tables, see next condition after the assert) to handle these :after child) did you say "patch"? :) AFAICT on a quick glance, the bug in the ruby code should be fixed by replacing - RenderInline::addChild(lastRun); + RenderInline::addChild(lastRun, rubyAfterBlock(this)); - RenderBlock::addChild(lastRun); + RenderBlock::addChild(lastRun, rubyAfterBlock(this)); I.e., place any new child before any already existing AFTER block. However, this will still cause the initially mentioned ASSERT with, e.g., display: table-row, because addChildIgnoringAnonymousColumnBlocks tests for isAnonymousBlock, which requires a BLOCK or BOX. However, ruby uses INLINE_BLOCK... >_< As far as I'm concerned, the check for display style doesn't belong in isAnonymousBlock, but removing it will very likely open a Pandora's box of trouble. It seems the safest way to handle generated content with <ruby> is doing what I tried avoiding: adding special RenderRubyRun objects just for them... *sigh*
Abhishek Arya
Comment 6 2011-08-26 07:22:40 PDT
*** Bug 67036 has been marked as a duplicate of this bug. ***
Abhishek Arya
Comment 7 2011-08-29 13:57:47 PDT
Ok, so we don't have any bug here and dont need the fix in c#5. Roland, i will leave this bug to you to fix the assert and add some more rendertree tests. 1. RenderBlock beforeChild calculation is fixed with my fix in r93794. <html> <body style="font: 1em/1 Ahem, sans-serif;"> <style type="text/css"> ruby { display: block; } ruby::after{ content: "AFTER"; display: table-cell; } </style> <ruby id="test"></ruby> <script> document.body.offsetTop; var ruby = document.getElementById('test'); ruby.appendChild(document.createTextNode("CONTENT")); </script> </body> </html> 2. RenderInline beforeChild calculation already exists, void RenderInline::addChildIgnoringContinuation(RenderObject* newChild, RenderObject* beforeChild) { // Make sure we don't append things after :after-generated content if we have it. if (!beforeChild && isAfterContent(lastChild())) beforeChild = lastChild(); <html> <body style="font: 1em/1 Ahem, sans-serif;"> <style type="text/css"> ruby::after{ content: "AFTER"; } </style> <ruby id="test"></ruby> <script> document.body.offsetTop; var ruby = document.getElementById('test'); ruby.appendChild(document.createTextNode("CONTENT")); </script> </body> </html>
Alexey Proskuryakov
Comment 8 2011-08-31 17:01:51 PDT
Ronald, do you expect to work on this soon? Even though there are crashing expectations, it's no good to have crash reporter window mysteriously pop up when running regression tests locally, so we need to add these to Skipped list.
Roland Steiner
Comment 9 2011-09-01 07:51:06 PDT
(In reply to comment #8) > Ronald, do you expect to work on this soon? I am out of office for the next 2 weeks, so my debugging resources are limited atm. > Even though there are crashing expectations, it's no good to have crash reporter window mysteriously pop up when running regression tests locally, so we need to add these to Skipped list. Could you please do this for the time being?
Abhishek Arya
Comment 10 2011-09-01 11:59:54 PDT
*** Bug 67145 has been marked as a duplicate of this bug. ***
Simon Fraser (smfr)
Comment 11 2011-09-01 12:07:15 PDT
Created attachment 106000 [details] Patch to disable the tests for now
Simon Fraser (smfr)
Comment 12 2011-09-01 12:11:13 PDT
Tests disabled in http://trac.webkit.org/changeset/94327
Roland Steiner
Comment 13 2011-09-27 05:06:44 PDT
Created attachment 108826 [details] Patch Wrapping generated content into their own ruby runs. This should sidestep all the various issues we encountered with implicit assumptions on anonymous blocks. IMHO it also makes the code more readable.
WebKit Review Bot
Comment 14 2011-09-27 05:32:19 PDT
Comment on attachment 108826 [details] Patch Attachment 108826 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9882269 New failing tests: fast/ruby/ruby-beforeafter.html
Roland Steiner
Comment 15 2011-10-16 23:49:52 PDT
In case the failing test is preventing someone from reviewing: This is most likely just a missing rebaseline.
Ryosuke Niwa
Comment 16 2012-04-22 22:32:00 PDT
Is this patch still valid? Or is it obsolete?
Roland Steiner
Comment 17 2012-04-23 00:31:21 PDT
I think it is still valid(In reply to comment #16) > Is this patch still valid? Or is it obsolete? I think it should still be valid, fundamentally.
Abhishek Arya
Comment 18 2012-04-26 19:21:25 PDT
Comment on attachment 108826 [details] Patch We don't want ruby to have its own code for managing generated content. This will open a can of security worms. The bug looks to be in RenderBlock::addChildIgnoringAnonymousColumnBlocks because it cannot passing the child addition to the correct anonymous parent and then hits that assert. We recently had similar bug fixed for fullscreen. See http://trac.webkit.org/changeset/111277. I don't think you need to change to isAnonymousBlock (as you said in c#5), but you do need to teach addChildIgnoringAnonymousColumnBlocks to handle ruby wrappers. Also see my c#7 and please first try to see if the test causes incorrect rendering, like does any testcase result in the wrong location of :before, :after content. Once we have a testcase showing wrong rendering, then it will be easier to see the problem.
Ahmad Saleem
Comment 19 2024-05-17 14:49:03 PDT
@Antti & Alan - is it applicable for newer CSS-Ruby implementation?
alan
Comment 20 2024-05-17 16:16:31 PDT
we don't have any of these classes anymore.
Note You need to log in before you can comment on or make changes to this bug.