| Summary: | Release assert in Vector::at in RenderLayoutState::establishLineGrid | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
| Component: | Layout and Rendering | Assignee: | 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
Ryosuke Niwa
2021-03-17 01:03:37 PDT
I can reproduce this crash with DumpRenderTree and WebKitTestRunner at r274459. Seems this lacks a test case. Created attachment 423989 [details]
Test case
Oops, sorry about that.
Created attachment 424119 [details]
Patch
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? 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 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". cc'ing zalan and antti 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 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? (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. (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. Created attachment 424223 [details]
Patch for landing
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. (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. Committed r275164: <https://commits.webkit.org/r275164> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424223 [details]. |