Repository navigation
LibWeb: specific auto grid layout causes crash (verification failed: grid_size.is_definite()) #23154
Description
First of all, this is an amazing project and I am seriously impressed with both SerenityOS as well as the Ladybird browser!
Out of curiosity I fired up Ladybird and pointed it at the company website (https://dialogic.nl) and... it crashes. Below are the stack trace as well as a minimal reproduction (serve up using python3 -m http.server then navigate to the file). As far as I can tell this is valid HTML/CSS and all other browsers render it correctly (even if it were incorrect I suppose Ladybird should not crash).
Unless this is super complicated I could take a stab at fixing the issue myself, but I would need some pointers on how to proceed... I am not too familiar with the code yet, nor with browser engines... but it could be a fun challenge.
<!DOCTYPE html>
<html>
<head>
<style>
.four {
display: grid;
grid-template-columns: repeat(4, 1fr);
grid-template-rows: repeat(auto-fit, auto);
}
</style>
</head>
<body>
<div class="four">
<div>FOO</div>
<div>BAR</div>
<div>BAR</div>
<div>BAR</div>
</div>
</body>
</html>VERIFICATION FAILED: grid_size.is_definite() at /Users/tommy/Repos/serenity/Userland/Libraries/LibWeb/Layout/GridFormattingContext.cpp:60
0 liblagom-ak.0.0.0.dylib 0x0000000100ee6ebc ak_verification_failed + 216
1 liblagom-web.0.0.0.dylib 0x0000000102249584 Web::Layout::GridFormattingContext::resolve_definite_track_size(Web::CSS::GridSize const&, Web::Layout::AvailableSpace const&) + 392
2 liblagom-web.0.0.0.dylib 0x0000000102249974 Web::Layout::GridFormattingContext::count_of_repeated_auto_fill_or_fit_tracks(Web::Layout::GridDimension) + 548
3 liblagom-web.0.0.0.dylib 0x00000001022574e8 AK::Function<void (Web::CSS::GridTrackSizeList const&)>::CallableWrapper<Web::Layout::GridFormattingContext::init_grid_lines(Web::Layout::GridDimension)::$_2>::call(Web::CSS::GridTrackSizeList const&) + 568
4 liblagom-web.0.0.0.dylib 0x00000001022550d8 AK::Function<void (Web::CSS::GridTrackSizeList const&)>::operator()(Web::CSS::GridTrackSizeList const&) const + 84
5 liblagom-web.0.0.0.dylib 0x0000000102254060 Web::Layout::GridFormattingContext::init_grid_lines(Web::Layout::GridDimension) + 212
6 liblagom-web.0.0.0.dylib 0x0000000102253a84 Web::Layout::GridFormattingContext::run(Web::Layout::Box const&, Web::Layout::LayoutMode, Web::Layout::AvailableSpace const&) + 80
7 liblagom-web.0.0.0.dylib 0x000000010222e464 Web::Layout::BlockFormattingContext::layout_block_level_box(Web::Layout::Box const&, Web::Layout::BlockContainer const&, Web::Layout::LayoutMode, Web::CSSPixels&, Web::Layout::AvailableSpace const&) + 1656
8 liblagom-web.0.0.0.dylib 0x000000010222c0a0 Web::Layout::BlockFormattingContext::layout_block_level_children(Web::Layout::BlockContainer const&, Web::Layout::LayoutMode, Web::Layout::AvailableSpace const&) + 224
9 liblagom-web.0.0.0.dylib 0x000000010222e758 Web::Layout::BlockFormattingContext::layout_block_level_box(Web::Layout::Box const&, Web::Layout::BlockContainer const&, Web::Layout::LayoutMode, Web::CSSPixels&, Web::Layout::AvailableSpace const&) + 2412
10 liblagom-web.0.0.0.dylib 0x000000010222c0a0 Web::Layout::BlockFormattingContext::layout_block_level_children(Web::Layout::BlockContainer const&, Web::Layout::LayoutMode, Web::Layout::AvailableSpace const&) + 224
11 liblagom-web.0.0.0.dylib 0x000000010222b8dc Web::Layout::BlockFormattingContext::run(Web::Layout::Box const&, Web::Layout::LayoutMode, Web::Layout::AvailableSpace const&) + 132
12 liblagom-web.0.0.0.dylib 0x000000010222e464 Web::Layout::BlockFormattingContext::layout_block_level_box(Web::Layout::Box const&, Web::Layout::BlockContainer const&, Web::Layout::LayoutMode, Web::CSSPixels&, Web::Layout::AvailableSpace const&) + 1656
13 liblagom-web.0.0.0.dylib 0x000000010222c0a0 Web::Layout::BlockFormattingContext::layout_block_level_children(Web::Layout::BlockContainer const&, Web::Layout::LayoutMode, Web::Layout::AvailableSpace const&) + 224
14 liblagom-web.0.0.0.dylib 0x000000010203ed5c Web::DOM::Document::update_layout() + 776
15 liblagom-core.0.0.0.dylib 0x0000000100e6ec04 AK::Function<void ()>::operator()() const + 84
16 liblagom-core.0.0.0.dylib 0x0000000100e6e0d0 Core::EventReceiver::dispatch_event(Core::Event&, Core::EventReceiver*) + 112
17 liblagom-core.0.0.0.dylib 0x0000000100e896c8 Core::ThreadEventQueue::process() + 452
18 liblagom-core.0.0.0.dylib 0x0000000100e680a0 Core::EventLoopImplementationUnix::exec() + 44
19 liblagom-core.0.0.0.dylib 0x0000000100e66e00 Core::EventLoop::exec() + 80
20 WebContent 0x0000000100429b20 serenity_main(Main::Arguments) + 3924
21 WebContent 0x00000001004b9dd4 main + 216
22 dyld 0x00000001892cd0e0 start + 2360
I am running the lagom/AppKit build of Ladybird on macOS, my Git HEAD is at 7833dc0.
Activity
AtkinsSJ commentedon Feb 11, 2024
Hi there, thanks for taking the time to reduce this down! I'll ping @kalenikaliaksandr who knows the most about grid layouts I think.
kalenikaliaksandr commentedon Feb 11, 2024
hey, seems like the problem is that we assume that
autocan't be used as a repeat value in auto-fit/fill, which is not true.there is specification that could help https://www.w3.org/TR/css-grid-2/#auto-repeat
and our grid formatting code lives in https://github.com/SerenityOS/serenity/blob/master/Userland/Libraries/LibWeb/Layout/GridFormattingContext.cpp
pixelspark commentedon Feb 11, 2024
@kalenikaliaksandr at your link I find this: "Automatic repetitions (auto-fill or auto-fit) cannot be combined with intrinsic or flexible sizes." It appears 'auto' is an intrisic size.
So then my CSS is actually invalid and we need to implement some proper fallback behavior in LibWeb!
I assume the way to go is to ignore the whole
grid-template-rowsdeclaration? Safari appears to do this (should have checked this earlier!):Where should this be done in LibWeb (CSS) code?
kalenikaliaksandr commentedon Feb 11, 2024
aha, if the value is invalid, it should be discarded by CSS parser. parsing of grid track sizes should happen somewhere here
serenity/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp
Line 5507 in 7833dc0
pixelspark commentedon Feb 11, 2024
OK, I added a check for 'auto' (apparently
.is_auto()exists for track sizes but is some sort of length percentage behind the scenes, so we need to check for the token itself)