Bug 74976 – Element still flowed below parent after changing from block to inline-block

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[David Barr]

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.

[David Barr]

I've witnessed this test pass on Opera 11.60 but no other browser.

[Eric Seidel (no email)]

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.

[David Barr]

Created attachment 121612 [details]
Amended reproduction (textContent rather than innerText)

[David Barr]

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

[David Barr]

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.

[David Barr]

Created attachment 123873 [details]
Short reproduction

[David Barr]

*** Bug 77420 has been marked as a duplicate of this bug. ***

[Joel Micah Donovan]

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.

[Joel Micah Donovan]

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.

[Eric Seidel (no email)]

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?

[Joel Micah Donovan]

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

[Eric Seidel (no email)]

This should be easy to understand in a debugger.  Unfortunately my checkout is not currently set up for easy debugging. :)

[Ojan Vafai]

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.

[Joel Micah Donovan]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Julien has recently been working with anonymous block cleanup and might have an idea for a quick fix.

[Julien Chaffraix]

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

[Ojan Vafai]

Possibly the same root cause as bug 86090.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Dave Hyatt]

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.

[Abhishek Arya]

Created attachment 146378 [details]
Patch

[Abhishek Arya]

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

[Abhishek Arya]

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

[Abhishek Arya]

Created attachment 146381 [details]
Patch

[Tony Chang]

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

[Tony Chang]

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.

[Abhishek Arya]

Created attachment 146389 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Abhishek Arya]

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

[Abhishek Arya]

Created attachment 146587 [details]
Patch

[Tony Chang]

Hyatt: Can you review this patch?

[Julien Chaffraix]

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?

[Abhishek Arya]

(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();
                }
            }

[Abhishek Arya]

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

[Abhishek Arya]

Created attachment 146956 [details]
Patch

[Abhishek Arya]

(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();

[Abhishek Arya]

Created attachment 147100 [details]
Patch

[Julien Chaffraix]

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.

[Abhishek Arya]

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

[Abhishek Arya]

Created attachment 147346 [details]
Patch

[Abhishek Arya]

Created attachment 147391 [details]
Patch

[Julien Chaffraix]

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.

[Abhishek Arya]

Committed r120477: <http://trac.webkit.org/changeset/120477>

[Eric Seidel (no email)]

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.

[Abhishek Arya]

(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

[Abhishek Arya]

And thanks Eric for your kind words. you are always welcome!