Bug 74976 - Element still flowed below parent after changing from block to inline-block
Summary: Element still flowed below parent after changing from block to inline-block
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Abhishek Arya
URL: http://docs.google.com/demo
Keywords:
: 77420 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-12-20 16:56 PST by David Barr
Modified: 2012-09-28 20:50 PDT (History)
21 users (show)

See Also:


Attachments
Simple reproduction (719 bytes, text/html)
2011-12-20 16:56 PST, David Barr
no flags Details
Amended reproduction (textContent rather than innerText) (723 bytes, text/html)
2012-01-08 19:41 PST, David Barr
no flags Details
Short reproduction (180 bytes, text/html)
2012-01-24 19:16 PST, David Barr
no flags Details
Very simple repro (126 bytes, text/html)
2012-02-21 14:08 PST, Joel Micah Donovan
no flags Details
Clickable simple repro (184 bytes, text/html)
2012-05-07 17:13 PDT, Joel Micah Donovan
no flags Details
Patch (6.00 KB, patch)
2012-06-07 13:45 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff
Patch (6.00 KB, patch)
2012-06-07 13:56 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff
Patch (5.34 KB, patch)
2012-06-07 14:19 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (560.02 KB, application/zip)
2012-06-07 21:04 PDT, WebKit Review Bot
no flags Details
Patch (12.06 KB, patch)
2012-06-08 09:50 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff
Patch (13.25 KB, patch)
2012-06-11 16:27 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff
Patch (13.23 KB, patch)
2012-06-12 09:32 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff
Patch (13.90 KB, patch)
2012-06-13 09:55 PDT, Abhishek Arya
no flags Details | Formatted Diff | Diff
Patch (13.34 KB, patch)
2012-06-13 12:35 PDT, Abhishek Arya
jchaffraix: review+
jchaffraix: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Barr 2011-12-20 16:56:56 PST
Created attachment 120116 [details]
Simple reproduction

Chrome Mac sometimes forces layout before the referenced stylesheets have been loaded.
On the Google Docs demo page, this reveals an odd layout behaviour:
Link dimensions still zero after child element changes from block to inline-block.
I've attached a simple reproduction that doesn't reply on the race condition.

From the downstream bug report (http://crbug.com/103423):
Chrome Version       : 15.0.874.106
URLs (if applicable) : http://docs.google.com/demo
OS version               : 10.6.8

Behavior in Firefox 7.x (if applicable): Works fine
Behavior in Chrome for Linux: Works fine

What steps will reproduce the problem?
1. Launch a fresh copy of Chrome
2. Open http://docs.google.com/demo

What is the expected result?

The "Sign In" link and "Get started" button should appear on the top row, across from the Google logo.

What happens instead?

Instead, they are displaced, seemingly wrapped to the next line (see screenshot). Reloading the page fixes the issue, and it does not come back until Chrome is quit, and then restarted (closing and reopening the window is not sufficient). This doesn't happen in Firefox 7, or Chrome on Linux.
Comment 1 David Barr 2011-12-20 17:05:54 PST
I've witnessed this test pass on Opera 11.60 but no other browser.
Comment 2 Eric Seidel (no email) 2012-01-08 19:33:43 PST
The test is invalid, at least for FF, since FF doesn't support innerText.

I would use element.style.display = "inline-block" instead of setting the textContent of a style element anyway. :)

To move forward with this, we need a test which we know works in the browsers under discussion, and ideally results from IE and FF of running the test.  It's not immediately clear to me if this is a bug or not.
Comment 3 David Barr 2012-01-08 19:41:16 PST
Created attachment 121612 [details]
Amended reproduction (textContent rather than innerText)
Comment 4 David Barr 2012-01-08 20:02:48 PST
(In reply to comment #3)
> Created an attachment (id=121612) [details]
> Amended reproduction (textContent rather than innerText)

With this change, the reproduction passes on:
Firefox 7.0.1
Opera 11.60
IE 10

Still fails with WebKit r104403.
Comment 5 David Barr 2012-01-12 03:11:14 PST
Another data point, I bisected the amended reproduction against Chromium binaries, and it failed as early as the first available binary. So this is a long-standing bug rather than a regression.
Comment 6 David Barr 2012-01-24 19:16:10 PST
Created attachment 123873 [details]
Short reproduction
Comment 7 David Barr 2012-02-08 19:35:07 PST
*** Bug 77420 has been marked as a duplicate of this bug. ***
Comment 8 Joel Micah Donovan 2012-02-21 14:06:26 PST
I've reduced this problem to a more general one.  For any parent element with style display "inline", if a child's display property changes from "block" to "inline" or "inline-block", the position of the child does not change accordingly.  This even happens if the child is not actually a block-level element by default.  It reproduces in WebKit browsers but not Firefox or Opera.

If the child's display property starts out "inline", then changes to "block", and then changes back to "inline", its position does not revert to its initial position.

I'm attaching a very short reproduction to get the point across.
Comment 9 Joel Micah Donovan 2012-02-21 14:08:29 PST
Created attachment 128040 [details]
Very simple repro

Hover the text to apply the display:block style.  Move the mouse away and the position doesn't revert.
Comment 10 Eric Seidel (no email) 2012-05-07 13:59:47 PDT
This line of Node::styleDiff:

    if (display1 != display2 || fl1 != fl2 || colSpan1 != colSpan2 
        || (specifiesColumns1 != specifiesColumns2 && doc->settings()->regionBasedColumnsEnabled())
        || (s1 && s2 && !s1->contentDataEquivalent(s2)))
        ch = Detach;

Should cause the node to re-attach after the hover change.

It's posible that the <b> is not getting marked for recalc as it's exiting the hover state immediately since its moving itself?
Comment 11 Joel Micah Donovan 2012-05-07 16:33:05 PDT
(In reply to comment #10)
> It's posible that the <b> is not getting marked for recalc as it's exiting the hover state immediately since its moving itself?

I don't think that's the issue.  I only used the hover selector to simplify the repro.  Even if you use JavaScript to change class of the <a>, rather than using the hover selector, it still reproduces the bug.
Comment 12 Eric Seidel (no email) 2012-05-07 16:54:51 PDT
This should be easy to understand in a debugger.  Unfortunately my checkout is not currently set up for easy debugging. :)
Comment 13 Ojan Vafai 2012-05-07 17:04:26 PDT
Tony and I were chatting and we think this might have something to do with anonymous boxes not getting merged back in. Just speculation though. See the "short reproduction" test case for a little more theorizing on why we think that.
Comment 14 Joel Micah Donovan 2012-05-07 17:13:23 PDT
Created attachment 140627 [details]
Clickable simple repro

Okay, then, here's another repro test case.  Is a few bytes bigger than the previous one, but instead of hovering the text, you click it.  Clicks toggle a class that colors the text green and sets the display property of the <b> tags to "block".  If you click the text twice, the green color is removed but the position of the text does not revert.
Comment 15 Eric Seidel (no email) 2012-05-07 17:35:39 PDT
If that's true, that should be easy to detect in Safari using the "View Rendering Tree" Debug Menu option on a single-process window.
Comment 16 Eric Seidel (no email) 2012-05-07 17:37:41 PDT
Yes, it looks like anonymous blocks being left around.  Unfortunately its not easy to copy/paste the rendering tree here w/o firing up DRT.
Comment 17 Eric Seidel (no email) 2012-05-07 17:40:16 PDT
Julien has recently been working with anonymous block cleanup and might have an idea for a quick fix.
Comment 18 Julien Chaffraix 2012-05-07 18:11:46 PDT
(In reply to comment #17)
> Julien has recently been working with anonymous block cleanup and might have an idea for a quick fix.

I may :)

I added a new function (RenderObject::destroyAndCleanupAnonymousWrappers) for table cells (see bug 7180) that does some anonymous cleaning at detach time. Currently it's only enabled for table cells as there was some potential badness from this patch (due to some classes (e.g. RenderRubyRun) implementing some clean-ups at removeChild time).

I haven't heard people complaining about the patch so I think it's time to enable it for everyone. We just need to be careful with the removeChild clean-ups that may have to be removed (at least those super-seeded by the function).
Comment 19 Ojan Vafai 2012-05-10 12:09:39 PDT
Possibly the same root cause as bug 86090.
Comment 20 Eric Seidel (no email) 2012-05-11 16:43:19 PDT
Here is a layout test:
<!doctype html>
<body>
<a id="target"><b>ONE</b><b id="two">TWO</b></a>
<script>
window.target = document.getElementById("target");
window.two = document.getElementById("two");

window.target.offsetTop; // force a layout
window.two.style.display = "block";
window.target.offsetTop; // force a layout
window.two.style.display = "inline";
</script>
</body>

And resulting render tree:

layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x52
  RenderBlock {HTML} at (0,0) size 800x52
    RenderBody {BODY} at (8,8) size 784x36
      RenderBlock (anonymous) at (0,0) size 784x18
        RenderInline {A} at (0,0) size 35x18
          RenderInline {B} at (0,0) size 35x18
            RenderText {#text} at (0,0) size 35x18
              text run at (0,0) width 35: "ONE"
      RenderBlock (anonymous) at (0,18) size 784x0
      RenderBlock (anonymous) at (0,18) size 784x18
        RenderInline {A} at (0,0) size 39x18
          RenderInline {B} at (0,0) size 39x18
            RenderText {#text} at (0,0) size 39x18
              text run at (0,0) width 39: "TWO"
        RenderText {#text} at (0,0) size 0x0
        RenderText {#text} at (0,0) size 0x0

You can see that even after restoring "two" to being inline, the "target" is still wrapped in an anonymous block from the continuation.

Basically the continuation is not getting cleaned up. :(
Comment 21 Eric Seidel (no email) 2012-05-11 16:49:55 PDT
To help me visualize continuations, this is the rendering tree *after* "two" has been marked "block" but before its returned to inline:

layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x52
  RenderBlock {HTML} at (0,0) size 800x52
    RenderBody {BODY} at (8,8) size 784x36
      RenderBlock (anonymous) at (0,0) size 784x18
        RenderInline {A} at (0,0) size 35x18
          RenderInline {B} at (0,0) size 35x18
            RenderText {#text} at (0,0) size 35x18
              text run at (0,0) width 35: "ONE"
      RenderBlock (anonymous) at (0,18) size 784x18
        RenderBlock {B} at (0,0) size 784x18
          RenderText {#text} at (0,0) size 39x18
            text run at (0,0) width 39: "TWO"
      RenderBlock (anonymous) at (0,36) size 784x0
        RenderInline {A} at (0,0) size 0x0
        RenderText {#text} at (0,0) size 0x0
        RenderText {#text} at (0,0) size 0x0
Comment 22 Dave Hyatt 2012-05-14 11:58:00 PDT
Yeah, this is a long-standing bug. There's just no code that attempts to detect this situation and clean up. It would be RenderBlock that would detect it, when an anonymous block that is part of a continuation chain empties out.

RenderBlock::removeChild.
Comment 23 Abhishek Arya 2012-06-07 13:45:54 PDT
Created attachment 146378 [details]
Patch
Comment 24 Abhishek Arya 2012-06-07 13:52:57 PDT
(In reply to comment #23)
> Created an attachment (id=146378) [details]
> Patch

There could be one more cleanup step in the tree to merge the RenderInline and its clone (see tree in patch testcase), but then it shouldnt impact rendering. I have fixed the continuation link so that when the inline is destroyed, its clone will be destroyed too :).
Comment 25 Abhishek Arya 2012-06-07 13:55:52 PDT
(In reply to comment #23)
> Created an attachment (id=146378) [details]
> Patch

wtf - INVALID: Image lacks a checksum. This will fail with a MISSING error in run-webkit-tests. Always generate new png files using run-webkit-tests. 

I used new-run-webkit-tests. retrying with run-webkit-tests.
Comment 26 Abhishek Arya 2012-06-07 13:56:30 PDT
Created attachment 146381 [details]
Patch
Comment 27 Tony Chang 2012-06-07 14:01:25 PDT
(In reply to comment #25)
> (In reply to comment #23)
> > Created an attachment (id=146378) [details] [details]
> > Patch
> 
> wtf - INVALID: Image lacks a checksum. This will fail with a MISSING error in run-webkit-tests. Always generate new png files using run-webkit-tests. 
> 
> I used new-run-webkit-tests. retrying with run-webkit-tests.

This is bug 88368.  You can ignore it.
Comment 28 Tony Chang 2012-06-07 14:04:16 PDT
Comment on attachment 146381 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        * rendering/RenderBlock.cpp:
> +        (WebCore::RenderBlock::removeChild):

Nit: Please explain what is happening here.

> LayoutTests/ChangeLog:10
> +        * fast/inline/inline-empty-block-continuation-remove-expected.png: Added.
> +        * fast/inline/inline-empty-block-continuation-remove-expected.txt: Added.
> +        * fast/inline/inline-empty-block-continuation-remove.html: Added.

Nit: Can we use a reftest instead? The reference HTML would just not have any JS.
Comment 29 Abhishek Arya 2012-06-07 14:19:21 PDT
Created attachment 146389 [details]
Patch
Comment 30 WebKit Review Bot 2012-06-07 21:04:30 PDT
Comment on attachment 146389 [details]
Patch

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

New failing tests:
fast/invalid/007.html
fast/forms/formmove3.html
tables/mozilla/bugs/bug647.html
editing/deleting/delete-3800834-fix.html
fast/forms/preserveFormDuringResidualStyle.html
fast/dynamic/011.html
tables/mozilla/other/wa_table_tr_align.html
fast/invalid/004.html
fast/invalid/003.html
fast/invalid/001.html
fast/invalid/019.html
fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html
Comment 31 WebKit Review Bot 2012-06-07 21:04:37 PDT
Created attachment 146467 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 32 Abhishek Arya 2012-06-07 21:19:41 PDT
(In reply to comment #30)
> (From update of attachment 146389 [details])
> Attachment 146389 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12911477
> 
> New failing tests:
> fast/invalid/007.html
> fast/forms/formmove3.html
> tables/mozilla/bugs/bug647.html
> editing/deleting/delete-3800834-fix.html
> fast/forms/preserveFormDuringResidualStyle.html
> fast/dynamic/011.html
> tables/mozilla/other/wa_table_tr_align.html
> fast/invalid/004.html
> fast/invalid/003.html
> fast/invalid/001.html
> fast/invalid/019.html
> fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html

awesome cr-linux bot. lots of re-baselining to do tmrw ma. empty anonymous block continuation can't dance anymore.
Comment 33 Abhishek Arya 2012-06-08 09:50:21 PDT
Created attachment 146587 [details]
Patch
Comment 34 Tony Chang 2012-06-11 10:13:48 PDT
Hyatt: Can you review this patch?
Comment 35 Julien Chaffraix 2012-06-11 15:38:51 PDT
Comment on attachment 146587 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:1247
> +            while (curr) {
> +                if (curr == parentRenderer) {
> +                    // This means that we didn't find anything that has |this| in its
> +                    // continuation chain.
> +                    curr = 0;
> +                    break;
> +                }
> +                if (curr->virtualContinuation() == this)
> +                    break; // Found our previous continuation.
> +                curr = curr->previousInPreOrder();
> +            }

It's too bad there is no way of getting your previous continuation's RenderBoxModelObject. I wonder if it's something we should expose as it involves a lot of tree traversal.

Regardless of this debate, you want the following function previousInPreOrder(const RenderObject* stayWithin) here. With that function, this code can be transformed into a very simple for-loop.

> Source/WebCore/rendering/RenderBlock.cpp:1254
> +                if (curr->isRenderInline())
> +                    toRenderInline(curr)->setContinuation(nextContinuation);
> +                else if (curr->isRenderBlock())
> +                    toRenderBlock(curr)->setContinuation(nextContinuation);
> +                else
> +                    ASSERT_NOT_REACHED();

Looks like toRenderBoxModelObject(curr)->setContinuation(nextContinuation) should be enough here, though the current logic would work.

> Source/WebCore/rendering/RenderBlock.cpp:1257
> +            destroy();

The current pattern for RenderBlock's child classes is to call RenderBlock::removeChild and *then* to do some post-removal clean-ups, isn't it dangerous to blow ourselves here? The 2 classes I have in mind are: RenderTable and RenderRubyRun. RenderTable should be fine due to the above isAnonymousBlockContinuation() check but I am not 100% sure if RenderRubyRun couldn't be badly impacted.

> Source/WebCore/rendering/RenderBlock.cpp:1261
> +            // If this was our last child be sure to clear out our line boxes.
> +            if (childrenInline())
> +                deleteLineBoxTree();

If beingDestroyed() is true, didn't willBeDestroyed already handled that?
Comment 36 Abhishek Arya 2012-06-11 16:16:10 PDT
(In reply to comment #35)
> (From update of attachment 146587 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146587&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:1247
> > +            while (curr) {
> > +                if (curr == parentRenderer) {
> > +                    // This means that we didn't find anything that has |this| in its
> > +                    // continuation chain.
> > +                    curr = 0;
> > +                    break;
> > +                }
> > +                if (curr->virtualContinuation() == this)
> > +                    break; // Found our previous continuation.
> > +                curr = curr->previousInPreOrder();
> > +            }
> 
> It's too bad there is no way of getting your previous continuation's RenderBoxModelObject. I wonder if it's something we should expose as it involves a lot of tree traversal.

Yeah we need to find a way to hook reverse continuations, but i think we should think of it in another bug.

> 
> Regardless of this debate, you want the following function previousInPreOrder(const RenderObject* stayWithin) here. With that function, this code can be transformed into a very simple for-loop.

Yeah the stayWithin part didnt existed, but happy to add it now.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:1254
> > +                if (curr->isRenderInline())
> > +                    toRenderInline(curr)->setContinuation(nextContinuation);
> > +                else if (curr->isRenderBlock())
> > +                    toRenderBlock(curr)->setContinuation(nextContinuation);
> > +                else
> > +                    ASSERT_NOT_REACHED();
> 
> Looks like toRenderBoxModelObject(curr)->setContinuation(nextContinuation) should be enough here, though the current logic would work.

Fixing.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:1257
> > +            destroy();
> 
> The current pattern for RenderBlock's child classes is to call RenderBlock::removeChild and *then* to do some post-removal clean-ups, isn't it dangerous to blow ourselves here? The 2 classes I have in mind are: RenderTable and RenderRubyRun. RenderTable should be fine due to the above isAnonymousBlockContinuation() check but I am not 100% sure if RenderRubyRun couldn't be badly impacted.

I had checked all the callers of RenderBlock::removeChild, including Ruby ones. They don't seem to use |this| afterwards. Anyway, ClusterFuzz will tell us if we cause some UAF here. Also, i don't see a way for delay cleanup later.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:1261
> > +            // If this was our last child be sure to clear out our line boxes.
> > +            if (childrenInline())
> > +                deleteLineBoxTree();
> 
> If beingDestroyed() is true, didn't willBeDestroyed already handled that?

This came up in another bug too. when we move children, our tree is no longer valid. willBedestroyed will get confused when it executes the following code.

if (isAnonymousBlock()) {
                for (InlineFlowBox* box = firstLineBox(); box; box = box->nextLineBox()) {
                    while (InlineBox* childBox = box->firstChild())
                        childBox->remove();
                }
            }
Comment 37 Abhishek Arya 2012-06-11 16:22:26 PDT
> > > Source/WebCore/rendering/RenderBlock.cpp:1254
> > > +                if (curr->isRenderInline())
> > > +                    toRenderInline(curr)->setContinuation(nextContinuation);
> > > +                else if (curr->isRenderBlock())
> > > +                    toRenderBlock(curr)->setContinuation(nextContinuation);
> > > +                else
> > > +                    ASSERT_NOT_REACHED();
> > 
> > Looks like toRenderBoxModelObject(curr)->setContinuation(nextContinuation) should be enough here, though the current logic would work.
> 
> Fixing.
> 

Ah! i remember what was the problem here. setContinuation is marked protected and RenderBlock and RenderInline explicitly call "using RenderBoxModelObject::setContinuation;" to prevent others from misusing it. I don't think we should change that. reverting back.
Comment 38 Abhishek Arya 2012-06-11 16:27:17 PDT
Created attachment 146956 [details]
Patch
Comment 39 Abhishek Arya 2012-06-12 08:57:03 PDT
(In reply to comment #38)
> Created an attachment (id=146956) [details]
> Patch

Fixing the patch for EWS and also this code needs to be executed in all cases, even when we are cleaning up the anonymous block continuation. see last part of c#36

if (childrenInline())
> > +                deleteLineBoxTree();
Comment 40 Abhishek Arya 2012-06-12 09:32:09 PDT
Created attachment 147100 [details]
Patch
Comment 41 Julien Chaffraix 2012-06-12 15:41:02 PDT
Comment on attachment 147100 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:1241
> +            RenderObject* parentRenderer = parent();

This could be removed. parent() is inlined and we don't expect it to change during the loop. I don't mind keeping it but in this case, |parentRenderer| should be const.

> Source/WebCore/rendering/RenderBlock.cpp:1243
> +            while (curr) {

I really prefer |for| loops, especially for very simple case like this one as it makes it obvious what is going on. It seems you disagree with me on that.

> Source/WebCore/rendering/RenderBlock.cpp:1254
> +               if (curr->isRenderInline())
> +                   toRenderInline(curr)->setContinuation(nextContinuation);
> +               else if (curr->isRenderBlock())
> +                   toRenderBlock(curr)->setContinuation(nextContinuation);
> +               else
> +                   ASSERT_NOT_REACHED();

A friend declaration in RenderBoxModelObject for this function looks better to me than this. As you pointed out, we don't want to expose setContinuation() to everyone to prevent misuse.

> Source/WebCore/rendering/RenderBlock.cpp:1257
> +            destroy();

> I had checked all the callers of RenderBlock::removeChild, including Ruby ones. They don't seem to use |this| afterwards. Anyway, ClusterFuzz will tell us if we cause some UAF here. Also, i don't see a way for delay cleanup later.

I don't agree that a clusterFuzz based approach is a good idea. It's definitely a cool tool but it doesn't make something dangerous less dangerous or turn bad practices into good ones. Here a huge issue is that someone comes and changes RenderRubyRun then we have a vulnerability. Also |removeChild| is called by some other functions, I bet you haven't reviewed all the callers to see if |this| was touched.

There are only 2 places where we call destroy() on |this|: RenderObject::destroyAndCleanupAnonymousWrappers (fine as it was designed carefully around destroy()) and RenderRubyRun::removeChild (...). This makes me wonder if we couldn't get a scaffolding to remove those at a safer time (maybe resurrecting the idea of pre-layout hooks / phases)

> Source/WebCore/rendering/RenderObject.h:195
> +    RenderObject* previousInPreOrder(const RenderObject* stayWithin = 0) const;

Please split this in 2 functions, like it is done for nextInPreOrder. Most callers don't care about staying inside a subtree and will pay an unneeded cost for that.

> LayoutTests/fast/inline/inline-empty-block-continuation-remove.html:2
> +<html style="font-family: ahem; font-size: 50px; -webkit-font-smoothing: none;">

Why do we need to use Ahem here? Ref-tests are pretty immune to font family usually.

> LayoutTests/fast/inline/inline-empty-block-continuation-remove.html:4
> +<!-- WebKit Bug 88022 - Cleanup empty anonymous block continuation -->
> +<!-- Orange and purple boxes should be on the same line -->

If we don't need Ahem, this should be dumped too.
Comment 42 Abhishek Arya 2012-06-12 23:14:36 PDT
(In reply to comment #41)
> (From update of attachment 147100 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147100&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:1241
> > +            RenderObject* parentRenderer = parent();
> 
> This could be removed. parent() is inlined and we don't expect it to change during the loop. I don't mind keeping it but in this case, |parentRenderer| should be const.

Fixed.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:1243
> > +            while (curr) {
> 
> I really prefer |for| loops, especially for very simple case like this one as it makes it obvious what is going on. It seems you disagree with me on that.

Fixed.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:1254
> > +               if (curr->isRenderInline())
> > +                   toRenderInline(curr)->setContinuation(nextContinuation);
> > +               else if (curr->isRenderBlock())
> > +                   toRenderBlock(curr)->setContinuation(nextContinuation);
> > +               else
> > +                   ASSERT_NOT_REACHED();
> 
> A friend declaration in RenderBoxModelObject for this function looks better to me than this. As you pointed out, we don't want to expose setContinuation() to everyone to prevent misuse.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:1257
> > +            destroy();
> 
> > I had checked all the callers of RenderBlock::removeChild, including Ruby ones. They don't seem to use |this| afterwards. Anyway, ClusterFuzz will tell us if we cause some UAF here. Also, i don't see a way for delay cleanup later.
> 
> I don't agree that a clusterFuzz based approach is a good idea. It's definitely a cool tool but it doesn't make something dangerous less dangerous or turn bad practices into good ones. Here a huge issue is that someone comes and changes RenderRubyRun then we have a vulnerability. Also |removeChild| is called by some other functions, I bet you haven't reviewed all the callers to see if |this| was touched.

1. I don't think we need to worry about RenderRuby since they are not linked by continuation chains. If we see the code calling setContinuation, it is only either multi-column layout code or the code handling blocks inside inlines.
2. I do think this change and destroyAndCleanupAnonymousWrappers need rethought so that we can workaround and not need to destroy |this|. For now, i don't see as unsafe.

> 
> There are only 2 places where we call destroy() on |this|: RenderObject::destroyAndCleanupAnonymousWrappers (fine as it was designed carefully around destroy()) and RenderRubyRun::removeChild (...). This makes me wonder if we couldn't get a scaffolding to remove those at a safer time (maybe resurrecting the idea of pre-layout hooks / phases)
> 
> > Source/WebCore/rendering/RenderObject.h:195
> > +    RenderObject* previousInPreOrder(const RenderObject* stayWithin = 0) const;
> 
> Please split this in 2 functions, like it is done for nextInPreOrder. Most callers don't care about staying inside a subtree and will pay an unneeded cost for that.

Fixed.

> 
> > LayoutTests/fast/inline/inline-empty-block-continuation-remove.html:2
> > +<html style="font-family: ahem; font-size: 50px; -webkit-font-smoothing: none;">
> 
> Why do we need to use Ahem here? Ref-tests are pretty immune to font family usually.

Fixed. This was left behind after i changed this from a pixel test to a ref test.

> 
> > LayoutTests/fast/inline/inline-empty-block-continuation-remove.html:4
> > +<!-- WebKit Bug 88022 - Cleanup empty anonymous block continuation -->
> > +<!-- Orange and purple boxes should be on the same line -->
> 
> If we don't need Ahem, this should be dumped too.

Fixed.
Comment 43 Abhishek Arya 2012-06-13 09:55:51 PDT
Created attachment 147346 [details]
Patch
Comment 44 Abhishek Arya 2012-06-13 12:35:44 PDT
Created attachment 147391 [details]
Patch
Comment 45 Julien Chaffraix 2012-06-15 10:24:09 PDT
Comment on attachment 147391 [details]
Patch

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

Still not a super fan of blowing up |this| during removeChild but let's see how it turns out.

> Source/WebCore/rendering/RenderBlock.cpp:1240
> +            RenderBoxModelObject* nextContinuation = continuation();

You could move the variable in the |for| loop as it's only used if you find your previous continuation.

> LayoutTests/platform/mac/TestExpectations:263
> +BUG_INFERNO : fast/invalid/007.html = TEXT

Why do you use BUG_INFERNO here. It should point to this bug at least (WK74976).

Some people advocate not touching expectations if you think you will stay on top of the bots and do the rebaseline, which is also fine by me.
Comment 46 Abhishek Arya 2012-06-15 10:58:10 PDT
Committed r120477: <http://trac.webkit.org/changeset/120477>
Comment 47 Eric Seidel (no email) 2012-06-19 20:24:14 PDT
Comment on attachment 147391 [details]
Patch

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

I'm really impressed that you took this on.  Thank you very much Abhishek.

> Source/WebCore/rendering/RenderBlock.cpp:1242
> +            for (RenderObject* curr = this; curr; curr = curr->previousInPreOrder(parent())) {
> +                if (curr->virtualContinuation() != this)

I might have separated this into to checks, one which checks isBoxModelObject() and then one which called continuation(), but this is fine too.  I'm surprised we even have a virtualContinuation() function.

I also might have separated this whole for into a separate static findPreviousContinuation(RenderObject *) helper function.  Then this becomes a simple if statement, and the static can have early return 0; statments instead of continue/break which seem a little confusing to my eyes.
Comment 48 Abhishek Arya 2012-06-19 22:21:32 PDT
(In reply to comment #47)
> (From update of attachment 147391 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147391&action=review
> 
> I'm really impressed that you took this on.  Thank you very much Abhishek.
> 
> > Source/WebCore/rendering/RenderBlock.cpp:1242
> > +            for (RenderObject* curr = this; curr; curr = curr->previousInPreOrder(parent())) {
> > +                if (curr->virtualContinuation() != this)
> 
> I might have separated this into to checks, one which checks isBoxModelObject() and then one which called continuation(), but this is fine too.  I'm surprised we even have a virtualContinuation() function.
> 
> I also might have separated this whole for into a separate static findPreviousContinuation(RenderObject *) helper function.  Then this becomes a simple if statement, and the static can have early return 0; statments instead of continue/break which seem a little confusing to my eyes.

I will fix this in the near future. I keep diving in that area of code quite often. 

Just a fyi, this is minor followup fix (crash detected by ClusterFuzz :) here - http://trac.webkit.org/changeset/120737
Comment 49 Abhishek Arya 2012-06-19 22:22:47 PDT
And thanks Eric for your kind words. you are always welcome!