Bug 223362 – Release assert in Vector::at in RenderLayoutState::establishLineGrid

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ryosuke Niwa]

e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000002d02a5829 WTF::CrashOnOverflow::crash() + 9 (CheckedArithmetic.h:134)
1   com.apple.WebCore             	0x00000002d02a57ee WTF::CrashOnOverflow::overflowed() + 14 (CheckedArithmetic.h:127)
2   com.apple.WebCore             	0x00000002d4d41332 WTF::Vector<std::__1::unique_ptr<WebCore::RenderLayoutState, std::__1::default_delete<WebCore::RenderLayoutState> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::at(unsigned long) const + 50 (Vector.h:707)
3   com.apple.WebCore             	0x00000002d5a463c9 WTF::Vector<std::__1::unique_ptr<WebCore::RenderLayoutState, std::__1::default_delete<WebCore::RenderLayoutState> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::operator[](unsigned long) const + 9 (Vector.h:722)
4   com.apple.WebCore             	0x00000002d5a45b5b WebCore::RenderLayoutState::establishLineGrid(WTF::Vector<std::__1::unique_ptr<WebCore::RenderLayoutState, std::__1::default_delete<WebCore::RenderLayoutState> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::RenderBlockFlow&) + 603 (RenderLayoutState.cpp:236)
5   com.apple.WebCore             	0x00000002d5a44c14 WebCore::RenderLayoutState::computePaginationInformation(WTF::Vector<std::__1::unique_ptr<WebCore::RenderLayoutState, std::__1::default_delete<WebCore::RenderLayoutState> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::RenderBox&, WebCore::LayoutUnit, bool) + 2036 (RenderLayoutState.cpp:166)
6   com.apple.WebCore             	0x00000002d5a439cd WebCore::RenderLayoutState::RenderLayoutState(WTF::Vector<std::__1::unique_ptr<WebCore::RenderLayoutState, std::__1::default_delete<WebCore::RenderLayoutState> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::RenderBox&, WebCore::LayoutSize const&, WebCore::LayoutUnit, bool) + 285 (RenderLayoutState.cpp:80)
7   com.apple.WebCore             	0x00000002d5a45169 WebCore::RenderLayoutState::RenderLayoutState(WTF::Vector<std::__1::unique_ptr<WebCore::RenderLayoutState, std::__1::default_delete<WebCore::RenderLayoutState> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::RenderBox&, WebCore::LayoutSize const&, WebCore::LayoutUnit, bool) + 9 (RenderLayoutState.cpp:74)
8   com.apple.WebCore             	0x00000002d4d20a13 std::__1::__unique_if<WebCore::RenderLayoutState>::__unique_single std::__1::make_unique<WebCore::RenderLayoutState, WTF::Vector<std::__1::unique_ptr<WebCore::RenderLayoutState, std::__1::default_delete<WebCore::RenderLayoutState> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::RenderBox&, WebCore::LayoutSize const&, WebCore::LayoutUnit&, bool&>(WTF::Vector<std::__1::unique_ptr<WebCore::RenderLayoutState, std::__1::default_delete<WebCore::RenderLayoutState> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::RenderBox&, WebCore::LayoutSize const&, WebCore::LayoutUnit&, bool&) + 179 (memory:3033)
9   com.apple.WebCore             	0x00000002d4d164ce decltype(auto) WTF::makeUnique<WebCore::RenderLayoutState, WTF::Vector<std::__1::unique_ptr<WebCore::RenderLayoutState, std::__1::default_delete<WebCore::RenderLayoutState> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::RenderBox&, WebCore::LayoutSize const&, WebCore::LayoutUnit&, bool&>(WTF::Vector<std::__1::unique_ptr<WebCore::RenderLayoutState, std::__1::default_delete<WebCore::RenderLayoutState> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::RenderBox&, WebCore::LayoutSize const&, WebCore::LayoutUnit&, bool&) + 90 (StdLibExtras.h:507) [inlined]
10  com.apple.WebCore             	0x00000002d4d164ce WebCore::FrameViewLayoutContext::pushLayoutState(WebCore::RenderBox&, WebCore::LayoutSize const&, WebCore::LayoutUnit, bool) + 446 (FrameViewLayoutContext.cpp:633)
11  com.apple.WebCore             	0x00000002d5a466c4 WebCore::LayoutStateMaintainer::LayoutStateMaintainer(WebCore::RenderBox&, WebCore::LayoutSize, bool, WebCore::LayoutUnit, bool) + 372 (RenderLayoutState.cpp:275)
12  com.apple.WebCore             	0x00000002d5a46829 WebCore::LayoutStateMaintainer::LayoutStateMaintainer(WebCore::RenderBox&, WebCore::LayoutSize, bool, WebCore::LayoutUnit, bool) + 9 (RenderLayoutState.cpp:274)
13  com.apple.WebCore             	0x00000002d57f1b04 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 948 (RenderBlockFlow.cpp:498)
14  com.apple.WebCore             	0x00000002d57c1525 WebCore::RenderBlock::layout() + 277 (RenderBlock.cpp:598)
15  com.apple.WebCore             	0x00000002d57f7075 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 1461 (RenderBlockFlow.cpp:762)
16  com.apple.WebCore             	0x00000002d57f3a9e WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) + 718 (RenderBlockFlow.cpp:673)
17  com.apple.WebCore             	0x00000002d57f1c28 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 1240 (RenderBlockFlow.cpp:525)
18  com.apple.WebCore             	0x00000002d57c1525 WebCore::RenderBlock::layout() + 277 (RenderBlock.cpp:598)
19  com.apple.WebCore             	0x00000002d56f7dd7 WebCore::RenderElement::layoutIfNeeded() + 71 (RenderElement.h:124)
20  com.apple.WebCore             	0x00000002d56f67d6 WebCore::ComplexLineLayout::layoutLineBoxes(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 1030 (ComplexLineLayout.cpp:1783)
21  com.apple.WebCore             	0x00000002d5810f15 WebCore::RenderBlockFlow::ensureLineBoxes() + 565 (RenderBlockFlow.cpp:3763)
22  com.apple.WebCore             	0x00000002d5a9ae5d WebCore::RenderObject::setSelectionState(WebCore::RenderObject::HighlightState) + 45 (RenderObject.cpp:1784)
23  com.apple.WebCore             	0x00000002d589412f WebCore::RenderBoxModelObject::setSelectionState(WebCore::RenderObject::HighlightState) + 111
24  com.apple.WebCore             	0x00000002d58122c2 WebCore::RenderBlockFlow::setSelectionState(WebCore::RenderObject::HighlightState) + 34 (RenderBlockFlow.cpp:3183)
25  com.apple.WebCore             	0x00000002d5894183 WebCore::RenderBoxModelObject::setSelectionState(WebCore::RenderObject::HighlightState) + 195 (RenderBoxModelObject.cpp:150)
26  com.apple.WebCore             	0x00000002d58122c2 WebCore::RenderBlockFlow::setSelectionState(WebCore::RenderObject::HighlightState) + 34 (RenderBlockFlow.cpp:3183)
27  com.apple.WebCore             	0x00000002d5894183 WebCore::RenderBoxModelObject::setSelectionState(WebCore::RenderObject::HighlightState) + 195 (RenderBoxModelObject.cpp:150)
28  com.apple.WebCore             	0x00000002d58122c2 WebCore::RenderBlockFlow::setSelectionState(WebCore::RenderObject::HighlightState) + 34 (RenderBlockFlow.cpp:3183)
29  com.apple.WebCore             	0x00000002d5b1f346 WebCore::RenderText::setSelectionState(WebCore::RenderObject::HighlightState) + 102 (RenderText.cpp:1282)
30  com.apple.WebCore             	0x00000002d5ba272f WebCore::RenderObject::setSelectionStateIfNeeded(WebCore::RenderObject::HighlightState) + 79 (RenderObject.h:1028)
31  com.apple.WebCore             	0x00000002d5b9f6cb WebCore::SelectionRangeData::apply(WebCore::RenderRange const&, WebCore::SelectionRangeData::RepaintMode) + 795 (SelectionRangeData.cpp:229)
32  com.apple.WebCore             	0x00000002d5b9f22f WebCore::SelectionRangeData::set(WebCore::RenderRange const&, WebCore::SelectionRangeData::RepaintMode) + 415 (SelectionRangeData.cpp:132)
33  com.apple.WebCore             	0x00000002d3edcb70 WebCore::FrameSelection::updateAppearance() + 1728 (FrameSelection.cpp:2181)
34  com.apple.WebCore             	0x00000002d3edc0c1 WebCore::FrameSelection::updateAndRevealSelection(WebCore::AXTextStateChangeIntent const&) + 257 (FrameSelection.cpp:474)
35  com.apple.WebCore             	0x00000002d3ea5743 WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>, WebCore::AXTextStateChangeIntent, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity) + 915 (FrameSelection.cpp:440)
36  com.apple.WebCore             	0x00000002d3ea450a WebCore::FrameSelection::selectAll() + 1306 (FrameSelection.cpp:2006)
37  com.apple.WebCore             	0x00000002d3ef83c1 WebCore::executeSelectAll(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 17 (EditorCommand.cpp:1021)
38  com.apple.WebCore             	0x00000002d3eba97c WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const + 220 (EditorCommand.cpp:1860)
39  com.apple.WebCore             	0x00000002d3b3ada4 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 244 (Document.cpp:5687)
40  com.apple.WebCore             	0x00000002d0d20e6a WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) + 1130 (JSDocument.cpp:5890)
41  com.apple.WebCore             	0x00000002d0d2095c long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) + 252 (JSDOMOperation.h:53)
42  com.apple.WebCore             	0x00000002d0d0b239 WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 9 (JSDocument.cpp:5895)

<rdar://75428270>

[Ryosuke Niwa]

I can reproduce this crash with DumpRenderTree and WebKitTestRunner at r274459.

[Rob Buis]

Seems this lacks a test case.

[Ryosuke Niwa]

Created attachment 423989 [details]
Test case

Oops, sorry about that.

[Frédéric Wang (:fredw)]

Created attachment 424119 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 424119 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Fix out-of-bound access for layoutStateStack and ensure the whole  vector is browsed.

Nit: two spaces between "whole" and "vector".

> Source/WebCore/rendering/RenderLayoutState.cpp:235
> -        for (int i = layoutStateStack.size() - 1; i <= 0; --i) {
> +        for (int i = layoutStateStack.size() - 1; i >= 0; --i) {

It seems like we should also be adding some tests demonstrating whatever grid layout bug this will fix?

[Ryosuke Niwa]

Looks like this might be a regression from https://trac.webkit.org/r224612?

Hm... looks like this code was introduced in https://trac.webkit.org/r105176 but I guess this loop was never really tested?

And line-grid spec has changed so much over the last 9 years that it's gonna be hard to figure out what this loop was supposed to do.

Perhaps Alan or Simon can figure out what they ought to be doing.

[Frédéric Wang (:fredw)]

Comment on attachment 424119 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayoutState.cpp:235
>> +        for (int i = layoutStateStack.size() - 1; i >= 0; --i) {
> 
> It seems like we should also be adding some tests demonstrating whatever grid layout bug this will fix?

To be honest, I don't know what this code is about. But I see it was added in https://trac.webkit.org/changeset/224612/webkit as part of a refactoring with "No change in functionality".

[Frédéric Wang (:fredw)]

cc'ing zalan and antti

[alan]

Comment on attachment 424119 [details]
Patch

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

>>> Source/WebCore/rendering/RenderLayoutState.cpp:235
>>> +        for (int i = layoutStateStack.size() - 1; i >= 0; --i) {
>> 
>> It seems like we should also be adding some tests demonstrating whatever grid layout bug this will fix?
> 
> To be honest, I don't know what this code is about. But I see it was added in https://trac.webkit.org/changeset/224612/webkit as part of a refactoring with "No change in functionality".

This shows how little nested line grids is used (not to confuse it with css grid layout).

[Frédéric Wang (:fredw)]

Comment on attachment 424119 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderLayoutState.cpp:235
>>>> +        for (int i = layoutStateStack.size() - 1; i >= 0; --i) {
>>> 
>>> It seems like we should also be adding some tests demonstrating whatever grid layout bug this will fix?
>> 
>> To be honest, I don't know what this code is about. But I see it was added in https://trac.webkit.org/changeset/224612/webkit as part of a refactoring with "No change in functionality".
> 
> This shows how little nested line grids is used (not to confuse it with css grid layout).

So are you able to provide some hints on how we could write a test for this? Or are you saying we should just ignore that?

[alan]

(In reply to Frédéric Wang (:fredw) from comment #10)
> Comment on attachment 424119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424119&action=review
> 
> >>>> Source/WebCore/rendering/RenderLayoutState.cpp:235
> >>>> +        for (int i = layoutStateStack.size() - 1; i >= 0; --i) {
> >>> 
> >>> It seems like we should also be adding some tests demonstrating whatever grid layout bug this will fix?
> >> 
> >> To be honest, I don't know what this code is about. But I see it was added in https://trac.webkit.org/changeset/224612/webkit as part of a refactoring with "No change in functionality".
> > 
> > This shows how little nested line grids is used (not to confuse it with css grid layout).
> 
> So are you able to provide some hints on how we could write a test for this?
No, I have close to zero knowledge about this feature.

[Ryosuke Niwa]

(In reply to Frédéric Wang (:fredw) from comment #10)
> Comment on attachment 424119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424119&action=review
> 
> >>>> Source/WebCore/rendering/RenderLayoutState.cpp:235
> >>>> +        for (int i = layoutStateStack.size() - 1; i >= 0; --i) {
> >>> 
> >>> It seems like we should also be adding some tests demonstrating whatever grid layout bug this will fix?
> >> 
> >> To be honest, I don't know what this code is about. But I see it was added in https://trac.webkit.org/changeset/224612/webkit as part of a refactoring with "No change in functionality".
> > 
> > This shows how little nested line grids is used (not to confuse it with css grid layout).
> 
> So are you able to provide some hints on how we could write a test for this?
> Or are you saying we should just ignore that?

I initially thought this was related to grid layout but it looks like there is no way of knowing what this CSS property was supposed to do at this point so landing it without a test seems okay to me.

[Frédéric Wang (:fredw)]

Created attachment 424223 [details]
Patch for landing

[Frédéric Wang (:fredw)]

Comment on attachment 424119 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Fix out-of-bound access for layoutStateStack and ensure the whole  vector is browsed.
> 
> Nit: two spaces between "whole" and "vector".

Done.

>>>>>>> Source/WebCore/rendering/RenderLayoutState.cpp:235
>>>>>>> +        for (int i = layoutStateStack.size() - 1; i >= 0; --i) {
>>>>>> 
>>>>>> It seems like we should also be adding some tests demonstrating whatever grid layout bug this will fix?
>>>>> 
>>>>> To be honest, I don't know what this code is about. But I see it was added in https://trac.webkit.org/changeset/224612/webkit as part of a refactoring with "No change in functionality".
>>>> 
>>>> This shows how little nested line grids is used (not to confuse it with css grid layout).
>>> 
>>> So are you able to provide some hints on how we could write a test for this? Or are you saying we should just ignore that?
>> 
>> No, I have close to zero knowledge about this feature.
> 
> I initially thought this was related to grid layout but it looks like there is no way of knowing what this CSS property was supposed to do at this point so landing it without a test seems okay to me.

OK, let's try and land this then.

[Ryosuke Niwa]

(In reply to Frédéric Wang (:fredw) from comment #14)
>
> > I initially thought this was related to grid layout but it looks like there is no way of knowing what this CSS property was supposed to do at this point so landing it without a test seems okay to me.
> 
> OK, let's try and land this then.

Sounds good.

[EWS]

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

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