Bug 93056 – Reimplement RenderQuote placement algorithm

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

[Elliott Sprehn]

The algorithm for placing quotes in the linked list and maintaining them is broken and also involves walking renderer subtrees repeatedly in RenderObjectChildList.

[Elliott Sprehn]

Created attachment 156244 [details]
Patch

Reimplemented algorithm from WKBug 92061

[Julien Chaffraix]

Comment on attachment 156244 [details]
Patch

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

It's very close to being ready for landing. I would like to see another round though.

> Source/WebCore/ChangeLog:41
> +        * rendering/RenderObjectChildList.cpp:
> +        (WebCore::RenderObjectChildList::removeChildNode):
> +        (WebCore::RenderObjectChildList::appendChildNode):
> +        (WebCore::RenderObjectChildList::insertChildNode):
> +        * rendering/RenderQuote.cpp:
> +        (WebCore::RenderQuote::RenderQuote):
> +        (WebCore::RenderQuote::~RenderQuote):
> +        (WebCore::RenderQuote::willBeDestroyed):
> +        (WebCore::RenderQuote::originalText):
> +        (WebCore::RenderQuote::computePreferredLogicalWidths):
> +        (WebCore::RenderQuote::styleDidChange):
> +        (WebCore):
> +        (WebCore::RenderQuote::attachQuote):
> +        (WebCore::RenderQuote::detachQuote):
> +        (WebCore::RenderQuote::updateDepth):
> +        * rendering/RenderQuote.h:
> +        (RenderQuote):
> +        * rendering/RenderView.cpp:
> +        (WebCore::RenderView::RenderView):
> +        * rendering/RenderView.h:
> +        (WebCore):
> +        (WebCore::RenderView::setRenderQuoteHead):
> +        (WebCore::RenderView::renderQuoteHead):
> +        (RenderView):

The function-level part of the ChangeLog should be filled.

> Source/WebCore/rendering/RenderObjectChildList.cpp:121
> +        if (oldChild->isQuote())
> +            toRenderQuote(oldChild)->detachQuote();

I feel like we should add the counterpart to insertChildNode. It's unclear to me if it's needed but it would be safer. The downside is that it would also force the quotes to be attached before preferred widths computation.

> Source/WebCore/rendering/RenderQuote.cpp:-162
> -    ASSERT(m_depth != UNKNOWN_DEPTH);

Might be good to add ASSERT(m_attached) here to keep the existing assertion.

> Source/WebCore/rendering/RenderQuote.cpp:118
> +    if (!QuotesData::equals(newQuotes, oldQuotes))
> +        setNeedsLayoutAndPrefWidthsRecalc();

I think we should just return StyleDifferenceLayout in RenderStyle::diff for quotes differences and kill this function.

> Source/WebCore/rendering/RenderQuote.cpp:150
> +        quote->updateDepth();

Calling setNeedsLayoutAndPrefWidthsRecalc() during layout is bad as I am pretty sure it is the cause of some of our quote bugs. Please add a FIXME about removing this anti-pattern.

> Source/WebCore/rendering/RenderQuote.h:39
>  protected:

I don't see the need for the protected section as we have no renderer inheriting from us.

> Source/WebCore/rendering/RenderView.h:199
> +    RenderQuote* renderQuoteHead() { return m_renderQuoteHead; }

It should be a const function.

[Elliott Sprehn]

(In reply to comment #2)
> (From update of attachment 156244 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156244&action=review
> ..
> The function-level part of the ChangeLog should be filled.

Yeah I've been avoiding doing that until the last minute since it's so hard to regenerate it if I need to change more methods.

> 
> > Source/WebCore/rendering/RenderObjectChildList.cpp:121
> > +        if (oldChild->isQuote())
> > +            toRenderQuote(oldChild)->detachQuote();
> 
> I feel like we should add the counterpart to insertChildNode. It's unclear to me if it's needed but it would be safer. The downside is that it would also force the quotes to be attached before preferred widths computation.

This doesn't work because you get attached too early since your anonymous parent isn't actually in the main render tree when the RenderQuote is inserted. As discussed in person I'll add a comment.

Either way this approach still isn't right since some things will compute the size on detached render trees, but this is good for a start.

> > Source/WebCore/rendering/RenderQuote.cpp:118
> > +    if (!QuotesData::equals(newQuotes, oldQuotes))
> > +        setNeedsLayoutAndPrefWidthsRecalc();
> 
> I think we should just return StyleDifferenceLayout in RenderStyle::diff for quotes differences and kill this function.

I don't understand what you mean?

> > Source/WebCore/rendering/RenderQuote.cpp:150
> > +        quote->updateDepth();
> 
> Calling setNeedsLayoutAndPrefWidthsRecalc() during layout is bad as I am pretty sure it is the cause of some of our quote bugs. Please add a FIXME about removing this anti-pattern.

Okay, though I'm not sure what the way to fix that is yet. We could walk the whole list of RenderQuote at the start of every layout and compute the depths and mark them there?

[Elliott Sprehn]

Comment on attachment 156244 [details]
Patch

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

> Source/WebCore/rendering/RenderQuote.cpp:144
> +    if (!m_previous) {

This if() block is going to cause a bug if anyone calls set computePreferredLogicalWidths() before this is in the main render tree. We'd walk all the way up to the top of our subtree, think we're the root and insert ourselves before the actual first RenderQuote in the tree. One fix would be to do if (predecessor->isRenderView()) in the loop and move the code there.

Even so we end up with the wrong value for m_depth = 0 for detached computePreferredLogicalWidths calls making RenderQuote's later in the document have the wrong value.

The only way I can think to fix this is to do previousInPreOrder over the DOM instead of the render tree, since you need to know your depth in a detached subtree to return a correct value from computePreferredLogicalWidths.

[Elliott Sprehn]

Created attachment 156720 [details]
Patch

Review from Julien.

[Elliott Sprehn]

(In reply to comment #2)
> (From update of attachment 156244 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156244&action=review
>  ...
> 
> > Source/WebCore/rendering/RenderQuote.cpp:-162
> > -    ASSERT(m_depth != UNKNOWN_DEPTH);
> 
> Might be good to add ASSERT(m_attached) here to keep the existing assertion.

I didn't add this assertion instead opting for checking for isRooted() in attachQuote() and allowing detached quote width computations. This is going to compute the wrong width causing possible extra layouts (and also makes it a bit more expensive since it has to walk up the render tree), but it avoids having an invalid m_depth values or inserting at the start improperly.

[Eric Seidel (no email)]

Comment on attachment 156720 [details]
Patch

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

> Source/WebCore/rendering/RenderObjectChildList.cpp:287
> +        // See appendChildNode for why you can't attachQuote here.

Better would be:  // Calling attachQuote() here would be too early (before anonymous renderers are inserted) see appendChild() for more explanation.

> Source/WebCore/rendering/RenderQuote.cpp:33
> -    , m_depth(UNKNOWN_DEPTH)
> +    , m_depth(0)

Now we're tracking this state separately with attached?

[Elliott Sprehn]

(In reply to comment #7)
> (From update of attachment 156720 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156720&action=review
> ...
> > Source/WebCore/rendering/RenderQuote.cpp:33
> > -    , m_depth(UNKNOWN_DEPTH)
> > +    , m_depth(0)
> 
> Now we're tracking this state separately with attached?

Yeah. Instead of allowing a negative value for m_depth, it starts at 0 so you always get a computed pref width (even if it's the width of the first depth which may be wrong).

I had thought about making m_depth a size_t, and then making getOpenQuote and getCloseQuote also take size_t. Then you just check if (!m_depth) return emptyAtom.impl(); in the CLOSE_QUOTE case, but Julien didn't like using size_t like that.

[Eric Seidel (no email)]

Comment on attachment 156720 [details]
Patch

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

I'm really glad you're working on this.

> Source/WebCore/rendering/RenderObjectChildList.cpp:121
>          if (oldChild->isRenderRegion())
>              toRenderRegion(oldChild)->detachRegion();
>  
> +        if (oldChild->isQuote())
> +            toRenderQuote(oldChild)->detachQuote();

This feels a lot like the willRemove callback.  But maybe that's a DOM thing?

> Source/WebCore/rendering/RenderView.h:305
> -    unsigned m_renderQuoteCount;
> +    RenderQuote* m_renderQuoteHead;

So this is larger on 64 bit, meaning if this has a COMPILE_ASSERT For size, you may need to update it.  But it shouldn't matter, since we dont' have a lot of RenderView objects and don't care about their size.

> Source/WebCore/rendering/style/RenderStyle.cpp:583
> +    if (!QuotesData::equals(rareInheritedData->quotes.get(), other->rareInheritedData->quotes.get()))

can rareInheritedData be null?  OR since it's inherited it will always be non-null since the root style gets one?

[Elliott Sprehn]

(In reply to comment #9)
> (From update of attachment 156720 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156720&action=review
> 
> I'm really glad you're working on this.

Thanks! :)

> 
> > Source/WebCore/rendering/RenderObjectChildList.cpp:121
> >          if (oldChild->isRenderRegion())
> >              toRenderRegion(oldChild)->detachRegion();
> >  
> > +        if (oldChild->isQuote())
> > +            toRenderQuote(oldChild)->detachQuote();
> 
> This feels a lot like the willRemove callback.  But maybe that's a DOM thing?

Yeah it's a DOM thing.

> 
> > Source/WebCore/rendering/RenderView.h:305
> > -    unsigned m_renderQuoteCount;
> > +    RenderQuote* m_renderQuoteHead;
> 
> So this is larger on 64 bit, meaning if this has a COMPILE_ASSERT For size, you may need to update it.  But it shouldn't matter, since we dont' have a lot of RenderView objects and don't care about their size.

Yeah. There's apparently no assert since I added the m_renderQuoteCount code and it didn't assert that time either.

> 
> > Source/WebCore/rendering/style/RenderStyle.cpp:583
> > +    if (!QuotesData::equals(rareInheritedData->quotes.get(), other->rareInheritedData->quotes.get()))
> 
> can rareInheritedData be null?  OR since it's inherited it will always be non-null since the root style gets one?

Yeah. You always have one. There's a few hundred lines of these things above this line that don't check too.

[Eric Seidel (no email)]

Comment on attachment 156720 [details]
Patch

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

> Source/WebCore/rendering/RenderQuote.cpp:157
> +    else if (view())
> +        view()->setRenderQuoteHead(m_next);

Don't we need an ASSERT here?  If view() is ever null, we had better be tearing down the whole document or we're leaving a stale pointer, no?  Also, could m_next be not attached?  Do we need to assert m_next->m_attached in this function?

> Source/WebCore/rendering/RenderQuote.cpp:159
> +    if (m_next)
> +        m_next->m_previous = m_previous;

Similarly, do we need to assert that m_previous is still attached?

> Source/WebCore/rendering/RenderQuote.cpp:189
> +    // FIXME: Don't dirty pref widths inside of layout.

Could you explain better here.  I agree that sounds wrong, but this comment isn't very actionable as-is.

[Elliott Sprehn]

Created attachment 156805 [details]
Patch for landing

[Elliott Sprehn]

(In reply to comment #11)
> (From update of attachment 156720 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156720&action=review
> 
> > Source/WebCore/rendering/RenderQuote.cpp:157
> > +    else if (view())
> > +        view()->setRenderQuoteHead(m_next);
> 
> Don't we need an ASSERT here?  If view() is ever null, we had better be tearing down the whole document or we're leaving a stale pointer, no?  

view() is only null if we're destroying the whole tree (by design). documentBeingDestroyed is actually equivalent to return !view().

>Also, could m_next be not attached?  Do we need to assert m_next->m_attached in this function?

They could, and that's fine. When they do get attached they'll walk the list of RenderQuote's and update their depths to make them correct. I'm nervous about asserting that here since code could call computePreferredLogicalWidths on the RenderQuote's out of order which should be fine: they should just update their depths and then request a recalc later.

I don't know if that works due to the issue with calling setNeeds*() inside layout() but in theory it does.

> 
> > Source/WebCore/rendering/RenderQuote.cpp:159
> > +    if (m_next)
> > +        m_next->m_previous = m_previous;
> 
> Similarly, do we need to assert that m_previous is still attached?

Same.

[Eric Seidel (no email)]

Comment on attachment 156720 [details]
Patch

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

>>> Source/WebCore/rendering/RenderQuote.cpp:157
>>> +        view()->setRenderQuoteHead(m_next);
>> 
>> Don't we need an ASSERT here?  If view() is ever null, we had better be tearing down the whole document or we're leaving a stale pointer, no?  Also, could m_next be not attached?  Do we need to assert m_next->m_attached in this function?
> 
> view() is only null if we're destroying the whole tree (by design). documentBeingDestroyed is actually equivalent to return !view().

OK, but if m_next is *not* attached, and then it's destroyed, it will never "detach" and clean up the weak pointers.  Seems like the ASSERT is needed to prevent us from screwing ourselves here.

[Julien Chaffraix]

Comment on attachment 156720 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderQuote.cpp:157
>>>> +        view()->setRenderQuoteHead(m_next);
>>> 
>>> Don't we need an ASSERT here?  If view() is ever null, we had better be tearing down the whole document or we're leaving a stale pointer, no?  Also, could m_next be not attached?  Do we need to assert m_next->m_attached in this function?
>> 
>> view() is only null if we're destroying the whole tree (by design). documentBeingDestroyed is actually equivalent to return !view().
> 
> OK, but if m_next is *not* attached, and then it's destroyed, it will never "detach" and clean up the weak pointers.  Seems like the ASSERT is needed to prevent us from screwing ourselves here.

The only place where we set m_next are attach and detach and AFAICT m_next is set to an attached quote as we always walking the tree in reverse tree order in attach. However I agree that an ASSERT would be a good thing.

I would also just ASSERT(!view()) instead of NULL-checking following the comment below.

> Source/WebCore/rendering/RenderQuote.cpp:160
> +    if (!documentBeingDestroyed()) {

AFAICT you shouldn't be calling detachQuote in this case, period. I would move the if (!documentBeingDestroyed()) check to willBeDestroyed and ASSERT at the beginning of this function.

[Elliott Sprehn]

(In reply to comment #15)
> (From update of attachment 156720 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156720&action=review
> ...
> 
> > Source/WebCore/rendering/RenderQuote.cpp:160
> > +    if (!documentBeingDestroyed()) {
> 
> AFAICT you shouldn't be calling detachQuote in this case, period. I would move the if (!documentBeingDestroyed()) check to willBeDestroyed and ASSERT at the beginning of this function.

That means you can't have the destructor asserts that ensure the whole thing got cleaned up.

RenderQuote::~RenderQuote()
{
    ASSERT(!m_attached);
    ASSERT(!m_next && !m_previous);
}

By design I made sure every node is detached even if the document is being destroyed. This should be very fast since it's just setting pointers to null and no updating depths.

If we never call detach during destruction you can't assert about that, since by the time the destructor is called the view is already null in all cases.

[Elliott Sprehn]

Created attachment 156990 [details]
Patch for landing

[Elliott Sprehn]

Created attachment 156993 [details]
Patch

Patch with added asserts

[Eric Seidel (no email)]

Comment on attachment 156993 [details]
Patch

This looks OK to me.

[Julien Chaffraix]

Comment on attachment 156993 [details]
Patch

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

I agree with Eric but have 2 last comments that should be fixed.

> Source/WebCore/rendering/RenderQuote.cpp:160
> +        view()->setRenderQuoteHead(m_next);

See Eric's comment here, we should have:

ASSERT(!m_next || m_next->isAttached());

That will catch if we can have a potential dangling pointer in RenderView.

(setRenderQuoteHead could also have this ASSERT but it's difficult to implement with the current code and I don't think it's worth the effort)

> Source/WebCore/rendering/RenderQuote.cpp:194
> +    // FIXME: Don't dirty pref widths inside of layout because a parent may do
> +    // setNeedsLayout(false) which would prevent the needed recalc. Instead use
> +    // a pre-layout hook.

Setting the layout bit is what is screwing us here. However dirtying the preferred widths is also bad. I think mentioning both would be better:

// FIXME: Don't call setNeedsLayout or dirty our preferred widths during layout. This is likely to fail anyway as
// one of our ancestor will call setNeedsLayout(false), preventing the future layout to occur on |this|. The solution
// is to move that to a pre-layout phase.

(I know some people prefer the word phase as hook is too loaded)

[Elliott Sprehn]

Created attachment 157000 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 157000 [details]
Patch for landing

Clearing flags on attachment: 157000

Committed r124969: <http://trac.webkit.org/changeset/124969>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 93436

[Takashi Toyoshima]

I'm sorry, but I'll revert this change because of assertion failure in Cr-Linux/Mac10.6/Win.

e.g.
crash log for DumpRenderTree (pid 3362):
STDOUT: <empty>
STDERR: ASSERTION FAILED: !m_previous || m_previous->m_attached
STDERR: /Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/RenderQuote.cpp(150) : void WebCore::RenderQuote::attachQuote()
STDERR: 1   0x5a5423f9 WebCore::RenderQuote::attachQuote()
STDERR: 2   0x5a541fb7 WebCore::RenderQuote::computePreferredLogicalWidths(float)
STDERR: 3   0x5a3c7374 WebCore::dirtyLineBoxesForRenderer(WebCore::RenderObject*, bool)
STDERR: 4   0x5a3c6910 WebCore::RenderBlock::layoutInlineChildren(bool, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&)
STDERR: 5   0x5a34502b WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit)
STDERR: 6   0x5a344135 WebCore::RenderBlock::layout()
STDERR: 7   0x5a35122d WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&)
STDERR: 8   0x5a347a98 WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::FractionalLayoutUnit&)
STDERR: 9   0x5a345055 WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit)
STDERR: 10  0x5a344135 WebCore::RenderBlock::layout()
STDERR: 11  0x5a35122d WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&)
STDERR: 12  0x5a347a98 WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::FractionalLayoutUnit&)
STDERR: 13  0x5a345055 WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit)
STDERR: 14  0x5a344135 WebCore::RenderBlock::layout()
STDERR: 15  0x5a5cfd7b WebCore::RenderView::layout()
STDERR: 16  0x5a15a20d WebCore::FrameView::layout(bool)
STDERR: 17  0x58c53b3f WebCore::Document::implicitClose()
STDERR: 18  0x59ff0ef2 WebCore::FrameLoader::checkCallImplicitClose()
STDERR: 19  0x59ff0a9e WebCore::FrameLoader::checkCompleted()
STDERR: 20  0x59fef5e3 WebCore::FrameLoader::finishedParsing()
STDERR: 21  0x58c62843 WebCore::Document::finishedParsing()
STDERR: 22  0x5927561e WebCore::HTMLTreeBuilder::finished()
STDERR: 23  0x59236da1 WebCore::HTMLDocumentParser::end()
STDERR: 24  0x59235b3f WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd()
STDERR: 25  0x5923587b WebCore::HTMLDocumentParser::prepareToStopParsing()
STDERR: 26  0x59236e25 WebCore::HTMLDocumentParser::attemptToEnd()
STDERR: 27  0x59236ea9 WebCore::HTMLDocumentParser::finish()
STDERR: 28  0x59fda5d5 WebCore::DocumentWriter::end()
STDERR: 29  0x59fbbf3c WebCore::DocumentLoader::finishedLoading()
STDERR: 30  0x5a01ab8b WebCore::MainResourceLoader::didFinishLoading(double)
STDERR: 31  0x5a03d33f WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double)
STDERR: [3362:-1602484928:3517846309732:ERROR:process_util_posix.cc(143)] Received signal 11
STDERR: 	0   libbase.dylib                       0x60bac86f base::debug::StackTrace::StackTrace() + 63
STDERR: 	1   libbase.dylib                       0x60bac80b base::debug::StackTrace::StackTrace() + 43
STDERR: 	2   libbase.dylib                       0x60c7f457 base::(anonymous namespace)::StackDumpSignalHandler(int, __siginfo*, __darwin_ucontext*) + 295
STDERR: 	3   libSystem.B.dylib                   0x9539405b _sigtramp + 43
STDERR: 	4   ???                                 0xffffffff 0x0 + 4294967295
STDERR: 	5   libwebkit.dylib                     0x5a541fb7 WebCore::RenderQuote::computePreferredLogicalWidths(float) + 71
STDERR: 	6   libwebkit.dylib                     0x5a3c7374 WebCore::dirtyLineBoxesForRenderer(WebCore::RenderObject*, bool) + 164
STDERR: 	7   libwebkit.dylib                     0x5a3c6910 WebCore::RenderBlock::layoutInlineChildren(bool, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) + 1536
STDERR: 	8   libwebkit.dylib                     0x5a34502b WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) + 1755
STDERR: 	9   libwebkit.dylib                     0x5a344135 WebCore::RenderBlock::layout() + 149
STDERR: 	10  libwebkit.dylib                     0x5a35122d WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) + 1117
STDERR: 	11  libwebkit.dylib                     0x5a347a98 WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::FractionalLayoutUnit&) + 1560
STDERR: 	12  libwebkit.dylib                     0x5a345055 WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) + 1797
STDERR: 	13  libwebkit.dylib                     0x5a344135 WebCore::RenderBlock::layout() + 149
STDERR: 	14  libwebkit.dylib                     0x5a35122d WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) + 1117
STDERR: 	15  libwebkit.dylib                     0x5a347a98 WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::FractionalLayoutUnit&) + 1560
STDERR: 	16  libwebkit.dylib                     0x5a345055 WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) + 1797
STDERR: 	17  libwebkit.dylib                     0x5a344135 WebCore::RenderBlock::layout() + 149
STDERR: 	18  libwebkit.dylib                     0x5a5cfd7b WebCore::RenderView::layout() + 1211
STDERR: 	19  libwebkit.dylib                     0x5a15a20d WebCore::FrameView::layout(bool) + 3677
STDERR: 	20  libwebkit.dylib                     0x58c53b3f WebCore::Document::implicitClose() + 1071
STDERR: 	21  libwebkit.dylib                     0x59ff0ef2 WebCore::FrameLoader::checkCallImplicitClose() + 178
STDERR: 	22  libwebkit.dylib                     0x59ff0a9e WebCore::FrameLoader::checkCompleted() + 366
STDERR: 	23  libwebkit.dylib                     0x59fef5e3 WebCore::FrameLoader::finishedParsing() + 195
STDERR: 	24  libwebkit.dylib                     0x58c62843 WebCore::Document::finishedParsing() + 659
STDERR: 	25  libwebkit.dylib                     0x5927561e WebCore::HTMLTreeBuilder::finished() + 190
STDERR: 	26  libwebkit.dylib                     0x59236da1 WebCore::HTMLDocumentParser::end() + 289
STDERR: 	27  libwebkit.dylib                     0x59235b3f WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd() + 335
STDERR: 	28  libwebkit.dylib                     0x5923587b WebCore::HTMLDocumentParser::prepareToStopParsing() + 315
STDERR: 	29  libwebkit.dylib                     0x59236e25 WebCore::HTMLDocumentParser::attemptToEnd() + 85
STDERR: 	30  libwebkit.dylib                     0x59236ea9 WebCore::HTMLDocumentParser::finish() + 89
STDERR: 	31  libwebkit.dylib                     0x59fda5d5 WebCore::DocumentWriter::end() + 437
STDERR: 	32  libwebkit.dylib                     0x59fbbf3c WebCore::DocumentLoader::finishedLoading() + 236
STDERR: 	33  libwebkit.dylib                     0x5a01ab8b WebCore::MainResourceLoader::didFinishLoading(double) + 315
STDERR: 	34  libwebkit.dylib                     0x5a03d33f WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) + 79
STDERR: 	35  libwebkit.dylib                     0x59554e17 WebCore::ResourceHandleInternal::didFinishLoading(WebKit::WebURLLoader*, double) + 295
STDERR: 	36  libglue.dylib                       0x638812fd webkit_glue::WebURLLoaderImpl::Context::OnCompletedRequest(net::URLRequestStatus const&, std::string const&, base::TimeTicks const&) + 1197
STDERR: 	37  DumpRenderTree                      0x5725b28d (anonymous namespace)::RequestProxy::NotifyCompletedRequest(net::URLRequestStatus const&, std::string const&, base::TimeTicks const&) + 109
STDERR: 	38  DumpRenderTree                      0x5725bab5 base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(net::URLRequestStatus const&, std::string const&, base::TimeTicks const&)>::Run((anonymous namespace)::RequestProxy*, net::URLRequestStatus const&, std::string const&, base::TimeTicks const&) + 213
STDERR: 	39  DumpRenderTree                      0x5725b9b6 base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(net::URLRequestStatus const&, std::string const&, base::TimeTicks const&)>, void ()((anonymous namespace)::RequestProxy* const&, net::URLRequestStatus const&, std::string const&, base::TimeTicks const&)>::MakeItSo(base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(net::URLRequestStatus const&, std::string const&, base::TimeTicks const&)>, (anonymous namespace)::RequestProxy* const&, net::URLRequestStatus const&, std::string const&, base::TimeTicks const&) + 150
STDERR: 	40  DumpRenderTree                      0x5725b8f5 base::internal::Invoker<4, base::internal::BindState<base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(net::URLRequestStatus const&, std::string const&, base::TimeTicks const&)>, void ()((anonymous namespace)::RequestProxy*, net::URLRequestStatus const&, std::string const&, base::TimeTicks const&), void ()((anonymous namespace)::RequestProxy*, net::URLRequestStatus, std::string, base::TimeTicks)>, void ()((anonymous namespace)::RequestProxy*, net::URLRequestStatus const&, std::string const&, base::TimeTicks const&)>::Run(base::internal::BindStateBase*) + 229
STDERR: 	41  libbase.dylib                       0x60b9c7ab base::Callback<void ()()>::Run() const + 75
STDERR: 	42  libbase.dylib                       0x60c26718 MessageLoop::RunTask(base::PendingTask const&) + 856
STDERR: 	43  libbase.dylib                       0x60c26ac2 MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) + 98
STDERR: 	44  libbase.dylib                       0x60c26d02 MessageLoop::DoWork() + 322
STDERR: 	45  libbase.dylib                       0x60b7b14b base::MessagePumpCFRunLoopBase::RunWork() + 107
STDERR: 	46  libbase.dylib                       0x60b7a912 base::MessagePumpCFRunLoopBase::RunWorkSource(void*) + 50
STDERR: 	47  CoreFoundation                      0x9072542b __CFRunLoopDoSources0 + 1563
STDERR: 	48  CoreFoundation                      0x90722eef __CFRunLoopRun + 1071
STDERR: 	49  CoreFoundation                      0x907223c4 CFRunLoopRunSpecific + 452
STDERR: 	50  CoreFoundation                      0x907221f1 CFRunLoopRunInMode + 97
STDERR: 	51  HIToolbox                           0x92d14e04 RunCurrentEventLoopInMode + 392
STDERR: 	52  HIToolbox                           0x92d14bb9 ReceiveNextEventCommon + 354
STDERR: 	53  HIToolbox                           0x92d14a3e BlockUntilNextEventMatchingListInMode + 81
STDERR: 	54  AppKit                              0x993a1595 _DPSNextEvent + 847
STDERR: 	55  AppKit                              0x993a0dd6 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 156
STDERR: 	56  AppKit                              0x993631f3 -[NSApplication run] + 821
STDERR: 	57  libbase.dylib                       0x60b7c07e base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) + 350
STDERR: 	58  libbase.dylib                       0x60b7aeb8 base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) + 104
STDERR: 	59  libbase.dylib                       0x60c26072 MessageLoop::RunInternal() + 290
STDERR: 	60  libbase.dylib                       0x60c25f2b MessageLoop::RunHandler() + 43
STDERR: 	61  libbase.dylib                       0x60c8b2f8 base::RunLoop::Run() + 72
STDERR: ax: bbadbeef, bx: 1, cx: 2d97292e, dx: 2d97292e
STDERR: di: 5ae7962f, si: 5ae795e1, bp: bfffb508, sp: bfffb4c0, ss: 23, flags: 210286
STDERR: ip: 5a542403, cs: 1b, ds: 23, es: 23, fs: 0, gs: f

[Elliott Sprehn]

(In reply to comment #25)
> I'm sorry, but I'll revert this change because of assertion failure in Cr-Linux/Mac10.6/Win.
> 
> e.g.
> crash log for DumpRenderTree (pid 3362):
> STDOUT: <empty>
> STDERR: ASSERTION FAILED: !m_previous || m_previous->m_attached

Which test was this?

[Takashi Toyoshima]

fast/table/crash-section-logical-height-changed-needsCellRecalc.html is the one which crashes after this change.

[Takashi Toyoshima]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Ftable%2Fcrash-section-logical-height-changed-needsCellRecalc.html

Linux build looks running without crash for now.
But, Mac10.6 and Win still continue to crash.

[Elliott Sprehn]

Created attachment 157358 [details]
Patch

Recursively attach the previous quote.

[Elliott Sprehn]

(In reply to comment #29)
> Created an attachment (id=157358) [details]
> Patch
> 
> Recursively attach the previous quote.

@Eric I thought about this more and I think we can recursively attach the previous instead of skipping. This also lets you avoid walking the list to compute the depths repeatedly as things in the middle become attached.

[Eric Seidel (no email)]

Comment on attachment 157358 [details]
Patch

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

> Source/WebCore/rendering/RenderQuote.cpp:151
> +        m_previous->attachQuote();

We may need to consider writing this as a loop to avoid any chance of blowing out the stack, since the number of nodes in a document is unlimited.

[Elliott Sprehn]

Created attachment 157495 [details]
Patch

Skip unattached quotes. We could rewrite that code as a loop, but it adds complexity and I dont think the performance benefit of skipping updates matters. We need to remove the quote depth updates eventually anyway.

[Eric Seidel (no email)]

Comment on attachment 157495 [details]
Patch

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

Looks OK.

> Source/WebCore/rendering/RenderQuote.cpp:130
> +    for (RenderObject* predecessor = previousInPreOrder(); predecessor; predecessor = predecessor->previousInPreOrder()) {

Wow, so this will end up walking all the way back through the rendering tree. That seems expensive. :)  Thankfully we only do this when we know there is already a quote in the doc.

> Source/WebCore/rendering/RenderQuote.cpp:131
> +        if (!predecessor->isQuote() || !toRenderQuote(predecessor)->m_attached)

Presumably a (private) accessor would be better than grabbing at a private member here, but it doens't really matter.

I also would have added a small comment about *why* we skip un-attached quotes.

> Source/WebCore/rendering/RenderQuote.h:53
>      int m_depth;
>      RenderQuote* m_next;
>      RenderQuote* m_previous;
> -    void placeQuote();
> +    bool m_attached;

Do we care about the size of this object?  Should we be stealing a bit from m_depth to use for m_attached?

[Elliott Sprehn]

(In reply to comment #33)
> (From update of attachment 157495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157495&action=review
> ..
> > Source/WebCore/rendering/RenderQuote.cpp:130
> > +    for (RenderObject* predecessor = previousInPreOrder(); predecessor; predecessor = predecessor->previousInPreOrder()) {
> 
> Wow, so this will end up walking all the way back through the rendering tree. That seems expensive. :)  Thankfully we only do this when we know there is already a quote in the doc.

Yup. That's why I wanted to start using that bit for counters in RenderObject to mark the existence of generated content so we can walk parents avoiding sections of the tree that have no content.

I guess we could do this during style resolve instead?

> 
> > Source/WebCore/rendering/RenderQuote.h:53
> >      int m_depth;
> >      RenderQuote* m_next;
> >      RenderQuote* m_previous;
> > -    void placeQuote();
> > +    bool m_attached;
> 
> Do we care about the size of this object?  Should we be stealing a bit from m_depth to use for m_attached?

These things are so rare I'd rather the code was obvious. I guess we could go back to using -1?

[Elliott Sprehn]

Created attachment 157525 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 157525 [details]
Patch for landing

Clearing flags on attachment: 157525

Committed r125220: <http://trac.webkit.org/changeset/125220>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.