Bug 223362

Summary: Release assert in Vector::at in RenderLayoutState::establishLineGrid
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Layout and RenderingAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cgarcia, ews-feeder, fred.wang, gpoo, koivisto, product-security, rbuis, simon.fraser, svillar, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test case
none
Patch
zalan: review+, ews-feeder: commit-queue-
Patch for landing none

Description Ryosuke Niwa 2021-03-17 01:03:37 PDT
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>
Comment 1 Ryosuke Niwa 2021-03-17 01:04:47 PDT
I can reproduce this crash with DumpRenderTree and WebKitTestRunner at r274459.
Comment 2 Rob Buis 2021-03-19 13:08:22 PDT
Seems this lacks a test case.
Comment 3 Ryosuke Niwa 2021-03-22 23:12:46 PDT
Created attachment 423989 [details]
Test case

Oops, sorry about that.
Comment 4 Frédéric Wang (:fredw) 2021-03-24 03:25:20 PDT
Created attachment 424119 [details]
Patch
Comment 5 Ryosuke Niwa 2021-03-24 04:00:10 PDT
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?
Comment 6 Ryosuke Niwa 2021-03-24 04:14:49 PDT
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.
Comment 7 Frédéric Wang (:fredw) 2021-03-24 04:20:30 PDT
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".
Comment 8 Frédéric Wang (:fredw) 2021-03-24 04:21:22 PDT
cc'ing zalan and antti
Comment 9 zalan 2021-03-24 07:12:37 PDT
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).
Comment 10 Frédéric Wang (:fredw) 2021-03-24 07:26:51 PDT
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?
Comment 11 zalan 2021-03-24 10:38:18 PDT
(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.
Comment 12 Ryosuke Niwa 2021-03-24 13:06:24 PDT
(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.
Comment 13 Frédéric Wang (:fredw) 2021-03-25 02:08:24 PDT
Created attachment 424223 [details]
Patch for landing
Comment 14 Frédéric Wang (:fredw) 2021-03-25 02:08:54 PDT
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.
Comment 15 Ryosuke Niwa 2021-03-28 22:55:03 PDT
(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.
Comment 16 EWS 2021-03-29 10:35:00 PDT
Committed r275164: <https://commits.webkit.org/r275164>

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