Bug 218576 – Debug assertion in RenderTreeUpdater::GeneratedContent::updateQuotesUpTo

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

e.g.

./rendering/updating/RenderTreeUpdaterGeneratedContent.cpp(69) : void WebCore::RenderTreeUpdater::GeneratedContent::updateQuotesUpTo(WebCore::RenderQuote *)
1   0x32cc632a9 WTFCrash
2   0x30e461b8b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x312d57b17 WebCore::RenderTreeUpdater::GeneratedContent::updateQuotesUpTo(WebCore::RenderQuote*)
4   0x312d55f97 WebCore::RenderTreeUpdater::GeneratedContent::updatePseudoElement(WebCore::Element&, WTF::Optional<WebCore::Style::ElementUpdate> const&, WebCore::PseudoId)
5   0x312d559df WebCore::RenderTreeUpdater::updateAfterDescendants(WebCore::Element&, WebCore::Style::ElementUpdates const*)
6   0x312d55970 WebCore::RenderTreeUpdater::popParent()
7   0x312d54f00 WebCore::RenderTreeUpdater::popParentsToDepth(unsigned int)
8   0x312d54c58 WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&)
9   0x312d543e3 WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >)
10  0x31137af3d WebCore::Document::updateRenderTree(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >)
11  0x31137b4ad WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType)
12  0x31137be40 WebCore::Document::updateStyleIfNeeded()
13  0x311382afb WebCore::Document::implicitClose()
14  0x311f4039b WebCore::FrameLoader::checkCallImplicitClose()
15  0x311f3fdfa WebCore::FrameLoader::checkCompleted()
16  0x311f3e1a7 WebCore::FrameLoader::finishedParsing()
17  0x311396e36 WebCore::Document::finishedParsing()
18  0x311abf9c8 WebCore::HTMLConstructionSite::finishedParsing()
19  0x311b054e7 WebCore::HTMLTreeBuilder::finished()
20  0x311ac6e68 WebCore::HTMLDocumentParser::end()
21  0x311ac4cd8 WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd()
22  0x311ac4a07 WebCore::HTMLDocumentParser::prepareToStopParsing()
23  0x311ac6ed2 WebCore::HTMLDocumentParser::attemptToEnd()
24  0x311ac6fa9 WebCore::HTMLDocumentParser::finish()
25  0x311ed9b82 WebCore::DocumentWriter::end()
26  0x311ed8b84 WebCore::DocumentLoader::finishedLoading()
27  0x311ed8581 WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource&, WebCore::NetworkLoadMetrics const&)
28  0x31204c54a WebCore::CachedResource::checkNotify(WebCore::NetworkLoadMetrics const&)
29  0x3120480bc WebCore::CachedResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&)
30  0x3120495ac WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&)
31  0x311fcfc44 WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&)

<rdar://problem/69565848>

[Ryosuke Niwa]

Created attachment 413188 [details]
Reduced test case

[Rob Buis]

Created attachment 413417 [details]
Patch

[Antti Koivisto]

Comment on attachment 413417 [details]
Patch

Please include the test case.

[Ryosuke Niwa]

There is no security implication here? If so, we should move this to non-security component and add a test.

[Antti Koivisto]

No, I don't think there are security implications here.

[Ryosuke Niwa]

Can we also add a test in that case?

[Rob Buis]

I am not sure anymore the fix is correct, will look again soon.

[Ryosuke Niwa]

(In reply to Rob Buis from comment #7)
> I am not sure anymore the fix is correct, will look again soon.

Ok.

[Ryosuke Niwa]

(In reply to Rob Buis from comment #7)
> I am not sure anymore the fix is correct, will look again soon.

Any update on this?

[Rob Buis]

(In reply to Ryosuke Niwa from comment #9)
> (In reply to Rob Buis from comment #7)
> > I am not sure anymore the fix is correct, will look again soon.
> 
> Any update on this?

Sorry for the delay! I finally found the root cause, when hitting splitDepth >= cMaxSplitDepth in RenderTreeBuilder::Inline::splitInlines, the tree re-ordering will ruin the existing RenderQuote order, making m_previousUpdatedQuote ordered *after* lastQuote.
Possible solutions:
- remove the ASSERT.
- keep patch as-is.
- clear m_previousUpdatedQuote if splitDepth >= cMaxSplitDepth is hit.
- still update the RenderQuote if not found when starting looking from m_previousUpdatedQuote.
I am not very familiar with the splitting, continuations or quote's though, thoughts?

[Rob Buis]

Created attachment 424893 [details]
Patch

[Rob Buis]

Created attachment 425007 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 425007 [details]
Patch

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

> Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:56
> +    if (m_updater.m_builder.maxSplitDepthOccured()) {
> +        m_previousUpdatedQuote = nullptr;
> +        updateQuotesUpTo(nullptr);
> +    } else {
> +        updateQuotesUpTo(nullptr);
> +        m_previousUpdatedQuote = nullptr;

Why are we re-ordering these?

> Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:173
> -    if (m_updater.renderView().hasQuotesNeedingUpdate()) {
> +    if (m_updater.renderView().hasQuotesNeedingUpdate() && !m_updater.m_builder.maxSplitDepthOccured()) {

What is this check out?

[zalan]

Comment on attachment 425007 [details]
Patch

In this invalid state when the continuation link is broken pretty much all bets are off. Since we don't have a fully expressed tree (gap in the descendants)  it's something that's hard to recover from (also this is a pathological case)
I would just do something like this: 

diff --git a/Source/WebCore/rendering/updating/RenderTreeBuilder.h b/Source/WebCore/rendering/updating/RenderTreeBuilder.h
index 34906eeaa60f..4da422c1b2d4 100644
--- a/Source/WebCore/rendering/updating/RenderTreeBuilder.h
+++ b/Source/WebCore/rendering/updating/RenderTreeBuilder.h
@@ -62,6 +62,8 @@ public:
     void createPlaceholderForFullScreen(RenderFullScreen&, std::unique_ptr<RenderStyle>, const LayoutRect&);
 #endif
 
+    bool hasBrokenContinuation() const { return m_hasBrokenContinuation; }
+
 private:
     void attachInternal(RenderElement& parent, RenderPtr<RenderObject>, RenderObject* beforeChild);
 
@@ -92,6 +94,8 @@ private:
 
     void reportVisuallyNonEmptyContent(const RenderElement& parent, const RenderObject& child);
 
+    void setHasBrokenContinuation() { m_hasBrokenContinuation = true; }
+
     class FirstLetter;
     class List;
     class MultiColumn;
@@ -150,6 +154,7 @@ private:
 #if ENABLE(FULLSCREEN_API)
     std::unique_ptr<FullScreen> m_fullScreenBuilder;
 #endif
+    bool m_hasBrokenContinuation { false };
 };
 
 }
diff --git a/Source/WebCore/rendering/updating/RenderTreeBuilderInline.cpp b/Source/WebCore/rendering/updating/RenderTreeBuilderInline.cpp
index 9ff8247a2bf8..35de076cb970 100644
--- a/Source/WebCore/rendering/updating/RenderTreeBuilderInline.cpp
+++ b/Source/WebCore/rendering/updating/RenderTreeBuilderInline.cpp
@@ -352,7 +352,8 @@ void RenderTreeBuilder::Inline::splitInlines(RenderInline& parent, RenderBlock*
                 sibling->setNeedsLayoutAndPrefWidthsRecalc();
                 sibling = next;
             }
-        }
+        } else
+          m_builder.setHasBrokenContinuation();
 
         // Keep walking up the chain.
         currentChild = current;
diff --git a/Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp b/Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp
index 62e7e2c77b2b..19b34a4726c9 100644
--- a/Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp
+++ b/Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp
@@ -66,7 +66,7 @@ void RenderTreeUpdater::GeneratedContent::updateQuotesUpTo(RenderQuote* lastQuot
         if (&quote == lastQuote)
             return;
     }
-    ASSERT(!lastQuote);
+    ASSERT(!lastQuote || m_updater.m_builder.hasBrokenContinuation());
 }
 
 static bool elementIsTargetedByKeyframeEffectRequiringPseudoElement(const Element* element, PseudoId pseudoId)

[Rob Buis]

Created attachment 425294 [details]
Patch

[zalan]

Comment on attachment 425294 [details]
Patch

Could you include a test case, please?

[Rob Buis]

Created attachment 425382 [details]
Patch

[EWS]

Committed r275606: <https://commits.webkit.org/r275606>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425382 [details].