Bug 218576 - Debug assertion in RenderTreeUpdater::GeneratedContent::updateQuotesUpTo
Summary: Debug assertion in RenderTreeUpdater::GeneratedContent::updateQuotesUpTo
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-04 11:20 PST by Ryosuke Niwa
Modified: 2021-04-07 07:48 PDT (History)
18 users (show)

See Also:


Attachments
Reduced test case (296 bytes, text/html)
2020-11-04 11:23 PST, Ryosuke Niwa
no flags Details
Patch (1.80 KB, patch)
2020-11-06 03:51 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (4.29 KB, patch)
2021-04-01 07:01 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (4.25 KB, patch)
2021-04-02 05:01 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (3.65 KB, patch)
2021-04-06 10:08 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (5.60 KB, patch)
2021-04-07 04:37 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2020-11-04 11:20:29 PST
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>
Comment 1 Ryosuke Niwa 2020-11-04 11:23:40 PST
Created attachment 413188 [details]
Reduced test case
Comment 2 Rob Buis 2020-11-06 03:51:40 PST
Created attachment 413417 [details]
Patch
Comment 3 Antti Koivisto 2020-11-06 08:41:11 PST
Comment on attachment 413417 [details]
Patch

Please include the test case.
Comment 4 Ryosuke Niwa 2020-11-06 19:47:29 PST
There is no security implication here? If so, we should move this to non-security component and add a test.
Comment 5 Antti Koivisto 2020-11-06 22:35:18 PST
No, I don't think there are security implications here.
Comment 6 Ryosuke Niwa 2020-11-07 00:00:52 PST
Can we also add a test in that case?
Comment 7 Rob Buis 2020-12-02 09:01:34 PST
I am not sure anymore the fix is correct, will look again soon.
Comment 8 Ryosuke Niwa 2020-12-03 19:16:48 PST
(In reply to Rob Buis from comment #7)
> I am not sure anymore the fix is correct, will look again soon.

Ok.
Comment 9 Ryosuke Niwa 2020-12-18 20:01:19 PST
(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?
Comment 10 Rob Buis 2021-03-30 13:43:03 PDT
(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?
Comment 11 Rob Buis 2021-04-01 07:01:10 PDT
Created attachment 424893 [details]
Patch
Comment 12 Rob Buis 2021-04-02 05:01:23 PDT
Created attachment 425007 [details]
Patch
Comment 13 Ryosuke Niwa 2021-04-02 16:11:54 PDT
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?
Comment 14 zalan 2021-04-05 16:38:40 PDT
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)
Comment 15 Rob Buis 2021-04-06 10:08:40 PDT
Created attachment 425294 [details]
Patch
Comment 16 zalan 2021-04-06 11:32:16 PDT
Comment on attachment 425294 [details]
Patch

Could you include a test case, please?
Comment 17 Rob Buis 2021-04-07 04:37:37 PDT
Created attachment 425382 [details]
Patch
Comment 18 EWS 2021-04-07 07:48:02 PDT
Committed r275606: <https://commits.webkit.org/r275606>

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