Summary: | Nullptr deref in WebCore::RenderTreeBuilder::Block::attachIgnoringContinuation when parent and beforeChild are siblings | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jack <shihchieh_lee> | ||||||||||||
Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bfulgham, changseok, esprehn+autocc, ews-feeder, ews-watchlist, ggaren, glenn, kondapallykalyan, pdr, product-security, simon.fraser, webkit-bug-importer, zalan | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Jack
2020-05-19 17:53:30 PDT
Test case: <style> ol { position: fixed; } ol, body { -webkit-columns: 2; } </style> <script> function run() { q.appendChild(ol); window.requestIdleCallback(idleCallback); } function idleCallback() { range = document.createElement("main").ownerDocument.createRange(); range.setEndBefore(li); range.extractContents(); } </script> <body onload=run()><ol id=ol><li id=li style="border-top-width: medium; column-span: all;"></ol><q id=q></q> Minimized test case: <style> ol { position: fixed; } ol, body { -webkit-columns: 2; } </style> <script> function run() { range = document.createElement("main").ownerDocument.createRange(); range.setEndBefore(li); range.extractContents(); } </script> <body onload=run()><q><ol><li id=li style="column-span: all;"></ol></q> Cause of the crash: 1. After the document is loaded, updateStyleIfNeeded is called and render tree is being updated. 2. When walking through the nodes in updateRenderTree(), we find that OL is not inline and needs to be splitter and be attached to a RenderBlock. 3. In RenderTreeBuilder::BlockFlow::attach() we find that OL has multiColumnFlow, so we call RenderTreeBuilder::Block::attachIgnoringContinuation() and pass the RenderMultiColumnFlowThread in OL as parent and RenderMultiColumnSet as before child. 4. In Block::attachIgnoringContinuation(), we try to find the immediate child of parent by looking at ancestors of RenderMultiColumnSet. 5. However, since RenderMultiColumnFlowThread and RenderMultiColumnSet are siblings, the logic exhausts the render tree and deref nullptr. Step 2: childBecameNonInline: parent=0x542fe00b0, child=0x542fe0400, beforeChild=0x542fe0c30 (B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, (C)omposited, (+)Dirty style, (+)Dirty layout B---YGLC -+ RenderView at (0,0) size 0x0 renderer->(0x542f5d210) layout->[normal child][positioned child] B-----L- -+ HTML RenderBlock at (0,0) size 0x0 renderer->(0x542f5d6c0) node->(0x542f5f800) layout->[self][normal child] B------- -+ BODY RenderBody at (0,0) size 0x0 renderer->(0x542f5d7f0) node->(0x542f5fc30) layout->[self][normal child] B---YGL- -+ RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0f20) [Rs:0x0 Re:0x0] layout->[self][normal child] I------- -+* Q RenderInline renderer->(0x542fe00b0) node->(0x542f5fcc0) continuation->(0x542fe1610) layout->[self][normal child] I----G-- -+ <pseudo> RenderInline renderer->(0x542fe0190) node->(0x542f5fee0) layout->[self][normal child] I---YG-- -+ RenderQuote renderer->(0x542fe0270) layout->[self][normal child] I---YG-- -+ RenderText renderer->(0x542fe0360) layout->[self] B------- -+ OL RenderBlock at (0,0) size 0x0 renderer->(0x542fe0400) node->(0x542f5fd50) [Rs:0x0 Re:0x0] layout->[self][normal child] B---YGL- -+ RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0790) [Rs:0x0 Re:0x0] layout->[self][normal child] B---YG-- -+ RenderMultiColumnSpannerPlaceholder at (0,0) size 0x0 renderer->(0x542fe0980) [Rs:0x0 Re:0x0] layout->[self] B------- -+ LI RenderListItem at (0,0) size 0x0 renderer->(0x542fe0530) node->(0x542f5fdf0) [Rs:0x0 Re:0x0] layout->[self][normal child] I---YG-- -+ RenderListMarker at (0,0) size 0x0 renderer->(0x542fe0670) layout->[self] B---YG-- -+ RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe0a90) [Rs:0x0 Re:0x0] layout->[self] I----G-- -+ <pseudo> RenderInline renderer->(0x542fe0c30) node->(0x542fe0010) layout->[self][normal child] I---YG-- -+ RenderQuote renderer->(0x542fe0d10) layout->[self][normal child] I---YG-- -+ RenderText renderer->(0x542fe0e00) layout->[self] I------- -+ #text RenderText renderer->(0x542fe0ea0) node->(0x542f5fe80) length->(1) "\n" layout->[self] B---YG-- -+ RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe1110) layout->[self] Step 3: BlockFlow::attach: parent=0x542fe0400, child=0x542fe1a80, beforeChild=0x542fe0a90 (B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, (C)omposited, (+)Dirty style, (+)Dirty layout B---YGLC -+ RenderView at (0,0) size 0x0 renderer->(0x542f5d210) layout->[normal child][positioned child] B-----L- -+ HTML RenderBlock at (0,0) size 0x0 renderer->(0x542f5d6c0) node->(0x542f5f800) layout->[self][normal child] B------- -+ BODY RenderBody at (0,0) size 0x0 renderer->(0x542f5d7f0) node->(0x542f5fc30) layout->[self][normal child] B---YGL- -+ RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0f20) [Rs:0x0 Re:0x0] layout->[self][normal child] B---YG-- -+ RenderBlock at (0,0) size 0x0 renderer->(0x542fe1740) [Rs:0x0 Re:0x0] layout->[self][normal child] I------- -+ Q RenderInline renderer->(0x542fe00b0) node->(0x542f5fcc0) continuation->(0x542fe1610) layout->[self][normal child] I----G-- -+ <pseudo> RenderInline renderer->(0x542fe0190) node->(0x542f5fee0) layout->[self][normal child] I---YG-- -+ RenderQuote renderer->(0x542fe0270) layout->[self][normal child] I---YG-- -+ RenderText renderer->(0x542fe0360) layout->[self] B---YG-- -+ RenderBlock at (0,0) size 0x0 renderer->(0x542fe1610) continuation->(0x542fe19a0) [Rs:0x0 Re:0x0] layout->[self] B------- -+ OL RenderBlock at (0,0) size 0x0 renderer->(0x542fe0400) node->(0x542f5fd50) [Rs:0x0 Re:0x0] layout->[self][normal child] B---YGL- -+ RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0790) [Rs:0x0 Re:0x0] layout->[self][normal child] B---YG-- -+ RenderMultiColumnSpannerPlaceholder at (0,0) size 0x0 renderer->(0x542fe0980) [Rs:0x0 Re:0x0] layout->[self] B------- -+ LI RenderListItem at (0,0) size 0x0 renderer->(0x542fe0530) node->(0x542f5fdf0) [Rs:0x0 Re:0x0] layout->[self][normal child] I---YG-- -+ RenderListMarker at (0,0) size 0x0 renderer->(0x542fe0670) [Rs:0x0 Re:0x0] layout->[self] B---YG-- -+* RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe0a90) [Rs:0x0 Re:0x0] layout->[self] B---YG-- -+ RenderBlock at (0,0) size 0x0 renderer->(0x542fe1870) [Rs:0x0 Re:0x0] layout->[self][normal child] I------- -+ Q RenderInline renderer->(0x542fe19a0) node->(0x542f5fcc0) layout->[self][normal child] I----G-- -+ <pseudo> RenderInline renderer->(0x542fe0c30) node->(0x542fe0010) layout->[self][normal child] I---YG-- -+ RenderQuote renderer->(0x542fe0d10) layout->[self][normal child] I---YG-- -+ RenderText renderer->(0x542fe0e00) layout->[self] I------- -+ #text RenderText renderer->(0x542fe0ea0) node->(0x542f5fe80) length->(1) "\n" layout->[self] B---YG-- -+ RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe1110) layout->[self] Step 4: beforeChildContainer = 0x0, parent = 0x542fe0790, beforeChild = 0x542fe0a90 (B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, (C)omposited, (+)Dirty style, (+)Dirty layout B---YGLC -+ RenderView at (0,0) size 0x0 renderer->(0x542f5d210) layout->[normal child][positioned child] B-----L- -+ HTML RenderBlock at (0,0) size 0x0 renderer->(0x542f5d6c0) node->(0x542f5f800) layout->[self][normal child] B------- -+ BODY RenderBody at (0,0) size 0x0 renderer->(0x542f5d7f0) node->(0x542f5fc30) layout->[self][normal child] B---YGL- -+ RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0f20) [Rs:0x0 Re:0x0] layout->[self][normal child] B---YG-- -+ RenderBlock at (0,0) size 0x0 renderer->(0x542fe1740) [Rs:0x0 Re:0x0] layout->[self][normal child] I------- -+ Q RenderInline renderer->(0x542fe00b0) node->(0x542f5fcc0) continuation->(0x542fe1610) layout->[self][normal child] I----G-- -+ <pseudo> RenderInline renderer->(0x542fe0190) node->(0x542f5fee0) layout->[self][normal child] I---YG-- -+ RenderQuote renderer->(0x542fe0270) layout->[self][normal child] I---YG-- -+ RenderText renderer->(0x542fe0360) layout->[self] B---YG-- -+ RenderBlock at (0,0) size 0x0 renderer->(0x542fe1610) continuation->(0x542fe19a0) [Rs:0x0 Re:0x0] layout->[self] B------- -+ OL RenderBlock at (0,0) size 0x0 renderer->(0x542fe0400) node->(0x542f5fd50) [Rs:0x0 Re:0x0] layout->[self][normal child] B---YGL- -+ RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0790) [Rs:0x0 Re:0x0] layout->[self][normal child] B---YG-- -+ RenderMultiColumnSpannerPlaceholder at (0,0) size 0x0 renderer->(0x542fe0980) [Rs:0x0 Re:0x0] layout->[self] B------- -+ LI RenderListItem at (0,0) size 0x0 renderer->(0x542fe0530) node->(0x542f5fdf0) [Rs:0x0 Re:0x0] layout->[self][normal child] I---YG-- -+ RenderListMarker at (0,0) size 0x0 renderer->(0x542fe0670) [Rs:0x0 Re:0x0] layout->[self] B---YG-- -+* RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe0a90) [Rs:0x0 Re:0x0] layout->[self] B---YG-- -+ RenderBlock at (0,0) size 0x0 renderer->(0x542fe1870) [Rs:0x0 Re:0x0] layout->[self][normal child] I------- -+ Q RenderInline renderer->(0x542fe19a0) node->(0x542f5fcc0) layout->[self][normal child] I----G-- -+ <pseudo> RenderInline renderer->(0x542fe0c30) node->(0x542fe0010) layout->[self][normal child] I---YG-- -+ RenderQuote renderer->(0x542fe0d10) layout->[self][normal child] I---YG-- -+ RenderText renderer->(0x542fe0e00) layout->[self] I------- -+ #text RenderText renderer->(0x542fe0ea0) node->(0x542f5fe80) length->(1) "\n" layout->[self] B---YG-- -+ RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe1110) layout->[self] Created attachment 399802 [details]
Patch
The patch is for discussion. In this patch, beforeChild is set to nullptr before calling m_builder.attach to avoid passing siblings as parent and beforeChild. However, doing so triggers the assertion below: ASSERTION FAILED: !flow.spannerMap().get(placeholder.spanner()) ./rendering/updating/RenderTreeBuilderMultiColumn.cpp(270) : void WebCore::RenderTreeBuilder::MultiColumn::multiColumnDescendantInserted(WebCore::RenderMultiColumnFlow &, WebCore::RenderObject &) If the code returns without calling m_builder.attach, there is no assertion or crash. (In reply to Jack from comment #4) > Created attachment 399802 [details] > Patch Comment on attachment 399802 [details]
Patch
Looks like EWS tests are crashing.
Comment on attachment 399802 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399802&action=review > Source/WebCore/rendering/updating/RenderTreeBuilderBlockFlow.cpp:47 > + if (beforeChild->parent() == &parent) > + beforeChild = nullptr; This clearly needs some explanation. In many cases (probably the most common) the beforeChild's parent is the host for the insertion point. Thanks. Yeah, this patch is to demonstrate the test case. Please see the complete render tree dump in comment #3. In this case we are passing parent.multiColumnFlow() and beforeChild to m_builder.attach. if (parent.multiColumnFlow() && (!parent.isFieldset() || !child->isLegend())) { ... return m_builder.attach(*parent.multiColumnFlow(), WTFMove(child), beforeChild); } And the render tree show that parent.multiColumnFlow() and beforeChild are siblings. BlockFlow::attach: parent=0x542fe0400, child=0x542fe1a80, beforeChild=0x542fe0a90 B------- -+ OL RenderBlock at (0,0) size 0x0 renderer->(0x542fe0400) node->(0x542f5fd50) [Rs:0x0 Re:0x0] layout->[self][normal child] B---YGL- -+ RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0790) [Rs:0x0 Re:0x0] layout->[self][normal child] B---YG-- -+ RenderMultiColumnSpannerPlaceholder at (0,0) size 0x0 renderer->(0x542fe0980) [Rs:0x0 Re:0x0] layout->[self] B------- -+ LI RenderListItem at (0,0) size 0x0 renderer->(0x542fe0530) node->(0x542f5fdf0) [Rs:0x0 Re:0x0] layout->[self][normal child] I---YG-- -+ RenderListMarker at (0,0) size 0x0 renderer->(0x542fe0670) [Rs:0x0 Re:0x0] layout->[self] B---YG-- -+* RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe0a90) [Rs:0x0 Re:0x0] layout->[self] And when the code enters RenderTreeBuilder::Block::attachIgnoringContinuation(), the following while loop will exhaust the ancestors and crashes when beforeChildContainer becomes nullptr. if (beforeChild && beforeChild->parent() != &parent) { RenderElement* beforeChildContainer = beforeChild->parent(); while (beforeChildContainer->parent() != &parent) beforeChildContainer = beforeChildContainer->parent(); (In reply to zalan from comment #7) > Comment on attachment 399802 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=399802&action=review > > > Source/WebCore/rendering/updating/RenderTreeBuilderBlockFlow.cpp:47 > > + if (beforeChild->parent() == &parent) > > + beforeChild = nullptr; > > This clearly needs some explanation. In many cases (probably the most > common) the beforeChild's parent is the host for the insertion point. Besides, I was wondering if RenderTreeBuilder::attachInternal should move beforeChild to the RenderMultiColumnSpannerPlaceholder since it is the child of RenderMultiColumnFlowThread? B------- -+ OL RenderBlock at (0,0) size 0x0 renderer->(0x542fe0400) node->(0x542f5fd50) [Rs:0x0 Re:0x0] layout->[self][normal child] B---YGL- -+ RenderMultiColumnFlowThread at (0,0) size 0x0 renderer->(0x542fe0790) [Rs:0x0 Re:0x0] layout->[self][normal child] B---YG-- -+ RenderMultiColumnSpannerPlaceholder at (0,0) size 0x0 renderer->(0x542fe0980) [Rs:0x0 Re:0x0] layout->[self] B------- -+ LI RenderListItem at (0,0) size 0x0 renderer->(0x542fe0530) node->(0x542f5fdf0) [Rs:0x0 Re:0x0] layout->[self][normal child] I---YG-- -+ RenderListMarker at (0,0) size 0x0 renderer->(0x542fe0670) [Rs:0x0 Re:0x0] layout->[self] B---YG-- -+* RenderMultiColumnSet at (0,0) size 0x0 renderer->(0x542fe0a90) [Rs:0x0 Re:0x0] layout->[self] However, in the following code, findColumnSpannerPlaceholder cannot find the spanner in the spannerMap so return nullptr, therefore beforeChild is not moved to spannerPlaceholder. } else if (is<RenderBox>(beforeChild)) { // Adjust the beforeChild if it happens to be a spanner and the its actual location is inside the fragmented flow. auto& beforeChildBox = downcast<RenderBox>(*beforeChild); if (auto* enclosingFragmentedFlow = parent.enclosingFragmentedFlow()) { auto columnSpannerPlaceholderForBeforeChild = [&]() -> RenderMultiColumnSpannerPlaceholder* { if (!is<RenderMultiColumnFlow>(enclosingFragmentedFlow)) return nullptr; auto& multiColumnFlow = downcast<RenderMultiColumnFlow>(*enclosingFragmentedFlow); return multiColumnFlow.findColumnSpannerPlaceholder(&beforeChildBox); }; if (auto* spannerPlaceholder = columnSpannerPlaceholderForBeforeChild()) beforeChild = spannerPlaceholder; } } It seems there is something wrong when we pass siblings as parent and beforeChild into RenderTreeBuilder::Block::attachIgnoringContinuation since it assumes that parent needs to be an ancestor of beforeChild, otherwise many places in that function would assert or crash. Does this seem like a case that we need to support, or just a corner case that a patch is enough? The simplest patch is to bail out in attachIgnoringContinuation when the while loop exhaust ancestors. Created attachment 399848 [details]
Patch
Comment on attachment 399848 [details]
Patch
This is papering over the real issue (not clearing the inner fragmented context when the column style gets unset).
Created attachment 400075 [details]
Test reduction
Created attachment 400095 [details]
Patch
Comment on attachment 400095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400095&action=review > LayoutTests/fast/multicol/nested-multicol-with-spanner-and-continuation.html:3 > + -webkit-columns: 2; Is the prefix required? (In reply to Simon Fraser (smfr) from comment #16) > Comment on attachment 400095 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400095&action=review > > > LayoutTests/fast/multicol/nested-multicol-with-spanner-and-continuation.html:3 > > + -webkit-columns: 2; > > Is the prefix required? Yeah, certainly not Thanks. Created attachment 400105 [details]
Patch
Committed r262093: <https://trac.webkit.org/changeset/262093> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400105 [details]. |