Bug 93056

Created attachment 156244 [details] Patch Reimplemented algorithm from WKBug 92061

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.

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

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.

Created attachment 156720 [details] Patch Review from Julien.

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

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?

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

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?

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

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.

Created attachment 156805 [details] Patch for landing

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

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.

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.

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

Created attachment 156990 [details] Patch for landing

Created attachment 156993 [details] Patch Patch with added asserts

Comment on attachment 156993 [details] Patch This looks OK to me.

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)

Created attachment 157000 [details] Patch for landing

Comment on attachment 157000 [details] Patch for landing Clearing flags on attachment: 157000 Committed r124969: <http://trac.webkit.org/changeset/124969>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 93436

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

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

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

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.

Created attachment 157358 [details] Patch Recursively attach the previous quote.

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

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.

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.

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?

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

Created attachment 157525 [details] Patch for landing

Comment on attachment 157525 [details] Patch for landing Clearing flags on attachment: 157525 Committed r125220: <http://trac.webkit.org/changeset/125220>

All reviewed patches have been landed. Closing bug.