Summary: | Debug assertion in RenderTreeUpdater::GeneratedContent::updateQuotesUpTo | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | Rob Buis <rbuis> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bfulgham, cgarcia, changseok, emilio, esprehn+autocc, ews-feeder, ews-watchlist, glenn, koivisto, kondapallykalyan, pdr, product-security, rbuis, rwlbuis, simon.fraser, svillar, webkit-bug-importer, zalan | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2020-11-04 11:20:29 PST
Created attachment 413188 [details]
Reduced test case
Created attachment 413417 [details]
Patch
Comment on attachment 413417 [details]
Patch
Please include the test case.
There is no security implication here? If so, we should move this to non-security component and add a test. No, I don't think there are security implications here. Can we also add a test in that case? I am not sure anymore the fix is correct, will look again soon. (In reply to Rob Buis from comment #7) > I am not sure anymore the fix is correct, will look again soon. Ok. (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? (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? Created attachment 424893 [details]
Patch
Created attachment 425007 [details]
Patch
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 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 ("e == lastQuote)
return;
}
- ASSERT(!lastQuote);
+ ASSERT(!lastQuote || m_updater.m_builder.hasBrokenContinuation());
}
static bool elementIsTargetedByKeyframeEffectRequiringPseudoElement(const Element* element, PseudoId pseudoId)
Created attachment 425294 [details]
Patch
Comment on attachment 425294 [details]
Patch
Could you include a test case, please?
Created attachment 425382 [details]
Patch
Committed r275606: <https://commits.webkit.org/r275606> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425382 [details]. |