Unify the LibWeb Rust crates in preparation for Rust layout - #10897
Merged
kalenikaliaksandr merged 2 commits intoJul 28, 2026
Merged
Conversation
LibWeb carried two Rust staticlibs whose sources could not reference each other: libweb_rust held the HTML seams while libweb_css_rust held style computation, so upcoming layout work could reach Rust-owned style data only through C++ re-exports, and every staticlib shipped its own copy of std and the panic helpers. Move the CSS crate's sources into libweb_rust as the css module, split the pure computed-value payload types into their own module, and fold the crate's generators and cbindgen emissions into the shared build script. The wrapper's unconditional Ladybird allocator now covers the css code too; the HTML tokenizer transfers allocation ownership across its FFI boundary, so the allocator cannot be feature-gated per build flavor. One copy of abort_on_panic and bytes_from_raw remains, the workspace loses the standalone css crate, and CMake imports a single crate emitting all five existing headers unchanged.
The layout node arena and tree builder lived in a third Rust staticlib that could reach css-crate definitions only through C++-registered byte offsets, because crates cannot share types across staticlib boundaries. With style computation already in the shared crate, keeping tree building separate would force the upcoming layout engine through the same indirection. Move the arena, node data, and tree builder into libweb_rust as the layout module, transplant the TreeBuilderRustFFI.h namespace and export configuration from cbindgen.toml into the shared build script, and delete the standalone crate. The generated header keeps its path and content, so no C++ consumer changes.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (37)
💤 Files with no reviewable changes (9)
📝 WalkthroughWalkthroughThe PR consolidates LibWeb CSS and layout Rust code into the shared Rust crate, adds build-time CSS metadata and FFI header generation, introduces tokenizer, style, color, statistics, and layout contracts, and updates CMake archive integration and internal module paths. ChangesLibWeb Rust consolidation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
Libraries/LibWeb/Rust/build.rs (2)
773-831: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse
generate_ffi_headerandgenerate_ffi_header_strictinto one function.The two bodies are identical apart from the error arm; duplicating the builder/write logic invites drift as header emission evolves.
♻️ Suggested consolidation
-fn generate_ffi_header( - config: cbindgen::Config, - sources: &[PathBuf], - out_dir: &Path, - ffi_out_dir: &Path, - header: &Path, -) { +fn generate_ffi_header_impl( + config: cbindgen::Config, + sources: &[PathBuf], + out_dir: &Path, + ffi_out_dir: &Path, + header: &Path, + strict: bool, +) { let builder = sources .iter() .fold(cbindgen::Builder::new().with_config(config), |builder, source| { builder.with_src(source) }); builder.generate().map_or_else( |error| match error { - cbindgen::Error::ParseSyntaxError { .. } => {} + cbindgen::Error::ParseSyntaxError { .. } if !strict => {} other => panic!("{other:?}"), }, |bindings| { /* unchanged */ }, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWeb/Rust/build.rs` around lines 773 - 831, Consolidate generate_ffi_header and generate_ffi_header_strict into a single header-generation function, preserving the shared builder and output-writing logic. Add a parameter or equivalent mode to select whether cbindgen::Error::ParseSyntaxError is ignored or panics, and update all callers to use the unified function with the appropriate behavior.
406-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim and validate range bounds before emitting numeric literals.
split_once(',')keeps surrounding whitespace, so a JSON entry written aslength [0, ∞]yieldsmax == " ∞", which misses the"∞"arm and emits" ∞.0"— invalid Rust in the generated file, surfacing as a confusing rustc error rather than a build.rs error. Same for any non-decimal notation (e.g.1e3→1e3.0).♻️ Suggested hardening
- let format_bound = |bound: &str| match (type_name, bound) { - ("integer", "-∞") => "i32::MIN as f64".to_string(), - ("integer", "∞") => "i32::MAX as f64".to_string(), - (_, "-∞") => "f32::MIN as f64".to_string(), - (_, "∞") => "f32::MAX as f64".to_string(), - _ if bound.contains('.') => bound.to_string(), - _ => format!("{bound}.0"), - }; + let format_bound = |bound: &str| -> Result<String, Box<dyn Error>> { + let bound = bound.trim(); + Ok(match (type_name, bound) { + ("integer", "-∞") => "i32::MIN as f64".to_string(), + ("integer", "∞") => "i32::MAX as f64".to_string(), + (_, "-∞") => "f32::MIN as f64".to_string(), + (_, "∞") => "f32::MAX as f64".to_string(), + _ => { + let parsed: f64 = bound + .parse() + .map_err(|_| format!("bad numeric bound '{bound}' for {name}"))?; + format!("{parsed:?}") + } + }) + }; ranges.push(format!( "FfiPropertyNumericRange {{ value_type: {value_type}, min: {}, max: {} }}", - format_bound(min), - format_bound(max) + format_bound(min)?, + format_bound(max)? ));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWeb/Rust/build.rs` around lines 406 - 421, Update the range parsing around format_bound to trim whitespace from both bounds before formatting, and validate that each bound uses supported numeric notation. Preserve the infinity mappings and decimal handling, while preventing non-decimal values such as scientific notation from receiving an invalid “.0” suffix; return a descriptive build.rs error for malformed bounds before pushing the generated range.Libraries/LibWeb/Rust/src/css/css_tokenizer.rs (1)
341-358: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffMaterializing the whole input as
Vec<(usize, u32)>costs ~12–16 bytes per code point.For large stylesheets this is a sizeable allocation on top of the source itself, purely to get O(1) indexed lookahead. A
CharIndices-based cursor with a small peek buffer (lookahead never exceeds 3) would remove it. Fine to defer, but worth measuring on big sheets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWeb/Rust/src/css/css_tokenizer.rs` around lines 341 - 358, Replace the full-input `code_points` allocation in `Tokenizer::new` with a `CharIndices`-based cursor and a small lookahead buffer sized for the tokenizer’s maximum three-code-point lookahead. Update the `Tokenizer` state and related cursor access so indexing, position tracking, and existing tokenization behavior remain unchanged without materializing every code point.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Libraries/LibWeb/Rust/build.rs`:
- Around line 1000-1011: Add an explicit cargo:rerun-if-changed directive for
the external RustAllocator.rs input used by generate_ffi_header_strict,
alongside the existing source-watch directives in build.rs. Ensure the directive
references the same manifest-relative path so changes to RustAllocator.rs
regenerate Layout/TreeBuilderRustFFI.h.
In `@Libraries/LibWeb/Rust/src/css/css_tokenizer.rs`:
- Around line 467-481: The comment-scanning loop in the CSS tokenizer must
consume the final non-EOF code point of unterminated comments. Update the EOF
guard in the loop around peek_twin to stop only when first is EOF, while
preserving the existing terminator detection and consumption behavior for closed
comments.
In `@Libraries/LibWeb/Rust/src/css/ffi_stats.rs`:
- Around line 87-97: Update rust_style_ffi_counter_name and
rust_style_ffi_counter_value to validate the caller-supplied index before
accessing FFI_OP_NAMES or COUNTERS, returning a benign fallback for out-of-range
indices. Route the access through the crate’s existing abort_on_panic pattern,
consistent with the other FFI entrypoints, so no unchecked indexing panic can
escape these extern functions.
In `@Libraries/LibWeb/Rust/src/css/ffi_support.rs`:
- Around line 45-68: Make DomStringView::new unsafe and require an &'a
FfiCallScope parameter so callers explicitly uphold the raw-pointer contract and
the wrapper lifetime is tied to the active FFI call scope. Update its
documentation and all call sites to pass the scope and use an unsafe block,
while preserving len and code_unit_at behavior.
In `@Libraries/LibWeb/Rust/src/css/property_metadata.rs`:
- Around line 30-45: Make longhand property ID validation active in release
builds by replacing the debug-only check in longhand_index with an unconditional
assert before subtraction and indexing. Apply the same validation approach to
property_index, and ensure the FFI-backed metadata accessors
property_requires_computation_level, property_animation_type, and
property_metadata_numeric_ranges cannot index tables with out-of-range IDs.
---
Nitpick comments:
In `@Libraries/LibWeb/Rust/build.rs`:
- Around line 773-831: Consolidate generate_ffi_header and
generate_ffi_header_strict into a single header-generation function, preserving
the shared builder and output-writing logic. Add a parameter or equivalent mode
to select whether cbindgen::Error::ParseSyntaxError is ignored or panics, and
update all callers to use the unified function with the appropriate behavior.
- Around line 406-421: Update the range parsing around format_bound to trim
whitespace from both bounds before formatting, and validate that each bound uses
supported numeric notation. Preserve the infinity mappings and decimal handling,
while preventing non-decimal values such as scientific notation from receiving
an invalid “.0” suffix; return a descriptive build.rs error for malformed bounds
before pushing the generated range.
In `@Libraries/LibWeb/Rust/src/css/css_tokenizer.rs`:
- Around line 341-358: Replace the full-input `code_points` allocation in
`Tokenizer::new` with a `CharIndices`-based cursor and a small lookahead buffer
sized for the tokenizer’s maximum three-code-point lookahead. Update the
`Tokenizer` state and related cursor access so indexing, position tracking, and
existing tokenization behavior remain unchanged without materializing every code
point.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b81fa6f-44f1-46d1-83b1-aafff507fc8d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
Cargo.tomlLibraries/LibWeb/CMakeLists.txtLibraries/LibWeb/CSS/Rust/Cargo.tomlLibraries/LibWeb/CSS/Rust/build.rsLibraries/LibWeb/CSS/Rust/cbindgen.tomlLibraries/LibWeb/CSS/Rust/src/lib.rsLibraries/LibWeb/Layout/Rust/Cargo.tomlLibraries/LibWeb/Layout/Rust/build.rsLibraries/LibWeb/Layout/Rust/cbindgen.tomlLibraries/LibWeb/Layout/Rust/src/lib.rsLibraries/LibWeb/Rust/Cargo.tomlLibraries/LibWeb/Rust/build.rsLibraries/LibWeb/Rust/src/css/animation.rsLibraries/LibWeb/Rust/src/css/calc.rsLibraries/LibWeb/Rust/src/css/cascaded_properties.rsLibraries/LibWeb/Rust/src/css/color_conversion.rsLibraries/LibWeb/Rust/src/css/color_interpolation.rsLibraries/LibWeb/Rust/src/css/computed_value_types.rsLibraries/LibWeb/Rust/src/css/computed_values.rsLibraries/LibWeb/Rust/src/css/css_enums.rsLibraries/LibWeb/Rust/src/css/css_pixels.rsLibraries/LibWeb/Rust/src/css/css_tokenizer.rsLibraries/LibWeb/Rust/src/css/custom_properties.rsLibraries/LibWeb/Rust/src/css/display.rsLibraries/LibWeb/Rust/src/css/ffi_stats.rsLibraries/LibWeb/Rust/src/css/ffi_support.rsLibraries/LibWeb/Rust/src/css/mod.rsLibraries/LibWeb/Rust/src/css/property_metadata.rsLibraries/LibWeb/Rust/src/css/selector_engine.rsLibraries/LibWeb/Rust/src/css/style_compute.rsLibraries/LibWeb/Rust/src/css/style_value.rsLibraries/LibWeb/Rust/src/css/transition.rsLibraries/LibWeb/Rust/src/layout/layout_node_arena.rsLibraries/LibWeb/Rust/src/layout/mod.rsLibraries/LibWeb/Rust/src/layout/node_data.rsLibraries/LibWeb/Rust/src/layout/tree_builder.rsLibraries/LibWeb/Rust/src/lib.rs
💤 Files with no reviewable changes (9)
- Libraries/LibWeb/Layout/Rust/cbindgen.toml
- Libraries/LibWeb/CSS/Rust/cbindgen.toml
- Libraries/LibWeb/Layout/Rust/build.rs
- Libraries/LibWeb/Layout/Rust/src/lib.rs
- Libraries/LibWeb/Layout/Rust/Cargo.toml
- Cargo.toml
- Libraries/LibWeb/CSS/Rust/build.rs
- Libraries/LibWeb/CSS/Rust/src/lib.rs
- Libraries/LibWeb/CSS/Rust/Cargo.toml
Comment on lines
+1000
to
+1011
| generate_ffi_header_strict( | ||
| tree_builder_config, | ||
| &[ | ||
| manifest_dir.join("src/layout/layout_node_arena.rs"), | ||
| manifest_dir.join("src/layout/node_data.rs"), | ||
| manifest_dir.join("src/layout/tree_builder.rs"), | ||
| manifest_dir.join("../../RustAllocator.rs"), | ||
| ], | ||
| &out_dir, | ||
| &ffi_out_dir, | ||
| Path::new("Layout/TreeBuilderRustFFI.h"), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
../../RustAllocator.rs is a cbindgen input but has no rerun-if-changed.
cargo:rerun-if-changed=src (line 840) replaces Cargo's default whole-package watch, and this file lives outside the package anyway, so edits to it won't regenerate Layout/TreeBuilderRustFFI.h — exactly the stale-header failure mode generate_ffi_header_strict was added to prevent.
🔧 Proposed fix
+ let allocator_source = manifest_dir.join("../../RustAllocator.rs");
+ println!("cargo:rerun-if-changed={}", allocator_source.display());
generate_ffi_header_strict(
tree_builder_config,
&[
manifest_dir.join("src/layout/layout_node_arena.rs"),
manifest_dir.join("src/layout/node_data.rs"),
manifest_dir.join("src/layout/tree_builder.rs"),
- manifest_dir.join("../../RustAllocator.rs"),
+ allocator_source,
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| generate_ffi_header_strict( | |
| tree_builder_config, | |
| &[ | |
| manifest_dir.join("src/layout/layout_node_arena.rs"), | |
| manifest_dir.join("src/layout/node_data.rs"), | |
| manifest_dir.join("src/layout/tree_builder.rs"), | |
| manifest_dir.join("../../RustAllocator.rs"), | |
| ], | |
| &out_dir, | |
| &ffi_out_dir, | |
| Path::new("Layout/TreeBuilderRustFFI.h"), | |
| ); | |
| let allocator_source = manifest_dir.join("../../RustAllocator.rs"); | |
| println!("cargo:rerun-if-changed={}", allocator_source.display()); | |
| generate_ffi_header_strict( | |
| tree_builder_config, | |
| &[ | |
| manifest_dir.join("src/layout/layout_node_arena.rs"), | |
| manifest_dir.join("src/layout/node_data.rs"), | |
| manifest_dir.join("src/layout/tree_builder.rs"), | |
| allocator_source, | |
| ], | |
| &out_dir, | |
| &ffi_out_dir, | |
| Path::new("Layout/TreeBuilderRustFFI.h"), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Libraries/LibWeb/Rust/build.rs` around lines 1000 - 1011, Add an explicit
cargo:rerun-if-changed directive for the external RustAllocator.rs input used by
generate_ffi_header_strict, alongside the existing source-watch directives in
build.rs. Ensure the directive references the same manifest-relative path so
changes to RustAllocator.rs regenerate Layout/TreeBuilderRustFFI.h.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🧹 Nitpick comments (3)
Libraries/LibWeb/Rust/build.rs (2)
773-831: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse
generate_ffi_headerandgenerate_ffi_header_strictinto one function.The two bodies are identical apart from the error arm; duplicating the builder/write logic invites drift as header emission evolves.
♻️ Suggested consolidation
-fn generate_ffi_header( - config: cbindgen::Config, - sources: &[PathBuf], - out_dir: &Path, - ffi_out_dir: &Path, - header: &Path, -) { +fn generate_ffi_header_impl( + config: cbindgen::Config, + sources: &[PathBuf], + out_dir: &Path, + ffi_out_dir: &Path, + header: &Path, + strict: bool, +) { let builder = sources .iter() .fold(cbindgen::Builder::new().with_config(config), |builder, source| { builder.with_src(source) }); builder.generate().map_or_else( |error| match error { - cbindgen::Error::ParseSyntaxError { .. } => {} + cbindgen::Error::ParseSyntaxError { .. } if !strict => {} other => panic!("{other:?}"), }, |bindings| { /* unchanged */ }, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWeb/Rust/build.rs` around lines 773 - 831, Consolidate generate_ffi_header and generate_ffi_header_strict into a single header-generation function, preserving the shared builder and output-writing logic. Add a parameter or equivalent mode to select whether cbindgen::Error::ParseSyntaxError is ignored or panics, and update all callers to use the unified function with the appropriate behavior.
406-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim and validate range bounds before emitting numeric literals.
split_once(',')keeps surrounding whitespace, so a JSON entry written aslength [0, ∞]yieldsmax == " ∞", which misses the"∞"arm and emits" ∞.0"— invalid Rust in the generated file, surfacing as a confusing rustc error rather than a build.rs error. Same for any non-decimal notation (e.g.1e3→1e3.0).♻️ Suggested hardening
- let format_bound = |bound: &str| match (type_name, bound) { - ("integer", "-∞") => "i32::MIN as f64".to_string(), - ("integer", "∞") => "i32::MAX as f64".to_string(), - (_, "-∞") => "f32::MIN as f64".to_string(), - (_, "∞") => "f32::MAX as f64".to_string(), - _ if bound.contains('.') => bound.to_string(), - _ => format!("{bound}.0"), - }; + let format_bound = |bound: &str| -> Result<String, Box<dyn Error>> { + let bound = bound.trim(); + Ok(match (type_name, bound) { + ("integer", "-∞") => "i32::MIN as f64".to_string(), + ("integer", "∞") => "i32::MAX as f64".to_string(), + (_, "-∞") => "f32::MIN as f64".to_string(), + (_, "∞") => "f32::MAX as f64".to_string(), + _ => { + let parsed: f64 = bound + .parse() + .map_err(|_| format!("bad numeric bound '{bound}' for {name}"))?; + format!("{parsed:?}") + } + }) + }; ranges.push(format!( "FfiPropertyNumericRange {{ value_type: {value_type}, min: {}, max: {} }}", - format_bound(min), - format_bound(max) + format_bound(min)?, + format_bound(max)? ));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWeb/Rust/build.rs` around lines 406 - 421, Update the range parsing around format_bound to trim whitespace from both bounds before formatting, and validate that each bound uses supported numeric notation. Preserve the infinity mappings and decimal handling, while preventing non-decimal values such as scientific notation from receiving an invalid “.0” suffix; return a descriptive build.rs error for malformed bounds before pushing the generated range.Libraries/LibWeb/Rust/src/css/css_tokenizer.rs (1)
341-358: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffMaterializing the whole input as
Vec<(usize, u32)>costs ~12–16 bytes per code point.For large stylesheets this is a sizeable allocation on top of the source itself, purely to get O(1) indexed lookahead. A
CharIndices-based cursor with a small peek buffer (lookahead never exceeds 3) would remove it. Fine to defer, but worth measuring on big sheets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWeb/Rust/src/css/css_tokenizer.rs` around lines 341 - 358, Replace the full-input `code_points` allocation in `Tokenizer::new` with a `CharIndices`-based cursor and a small lookahead buffer sized for the tokenizer’s maximum three-code-point lookahead. Update the `Tokenizer` state and related cursor access so indexing, position tracking, and existing tokenization behavior remain unchanged without materializing every code point.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Libraries/LibWeb/Rust/build.rs`:
- Around line 1000-1011: Add an explicit cargo:rerun-if-changed directive for
the external RustAllocator.rs input used by generate_ffi_header_strict,
alongside the existing source-watch directives in build.rs. Ensure the directive
references the same manifest-relative path so changes to RustAllocator.rs
regenerate Layout/TreeBuilderRustFFI.h.
In `@Libraries/LibWeb/Rust/src/css/css_tokenizer.rs`:
- Around line 467-481: The comment-scanning loop in the CSS tokenizer must
consume the final non-EOF code point of unterminated comments. Update the EOF
guard in the loop around peek_twin to stop only when first is EOF, while
preserving the existing terminator detection and consumption behavior for closed
comments.
In `@Libraries/LibWeb/Rust/src/css/ffi_stats.rs`:
- Around line 87-97: Update rust_style_ffi_counter_name and
rust_style_ffi_counter_value to validate the caller-supplied index before
accessing FFI_OP_NAMES or COUNTERS, returning a benign fallback for out-of-range
indices. Route the access through the crate’s existing abort_on_panic pattern,
consistent with the other FFI entrypoints, so no unchecked indexing panic can
escape these extern functions.
In `@Libraries/LibWeb/Rust/src/css/ffi_support.rs`:
- Around line 45-68: Make DomStringView::new unsafe and require an &'a
FfiCallScope parameter so callers explicitly uphold the raw-pointer contract and
the wrapper lifetime is tied to the active FFI call scope. Update its
documentation and all call sites to pass the scope and use an unsafe block,
while preserving len and code_unit_at behavior.
In `@Libraries/LibWeb/Rust/src/css/property_metadata.rs`:
- Around line 30-45: Make longhand property ID validation active in release
builds by replacing the debug-only check in longhand_index with an unconditional
assert before subtraction and indexing. Apply the same validation approach to
property_index, and ensure the FFI-backed metadata accessors
property_requires_computation_level, property_animation_type, and
property_metadata_numeric_ranges cannot index tables with out-of-range IDs.
---
Nitpick comments:
In `@Libraries/LibWeb/Rust/build.rs`:
- Around line 773-831: Consolidate generate_ffi_header and
generate_ffi_header_strict into a single header-generation function, preserving
the shared builder and output-writing logic. Add a parameter or equivalent mode
to select whether cbindgen::Error::ParseSyntaxError is ignored or panics, and
update all callers to use the unified function with the appropriate behavior.
- Around line 406-421: Update the range parsing around format_bound to trim
whitespace from both bounds before formatting, and validate that each bound uses
supported numeric notation. Preserve the infinity mappings and decimal handling,
while preventing non-decimal values such as scientific notation from receiving
an invalid “.0” suffix; return a descriptive build.rs error for malformed bounds
before pushing the generated range.
In `@Libraries/LibWeb/Rust/src/css/css_tokenizer.rs`:
- Around line 341-358: Replace the full-input `code_points` allocation in
`Tokenizer::new` with a `CharIndices`-based cursor and a small lookahead buffer
sized for the tokenizer’s maximum three-code-point lookahead. Update the
`Tokenizer` state and related cursor access so indexing, position tracking, and
existing tokenization behavior remain unchanged without materializing every code
point.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b81fa6f-44f1-46d1-83b1-aafff507fc8d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
Cargo.tomlLibraries/LibWeb/CMakeLists.txtLibraries/LibWeb/CSS/Rust/Cargo.tomlLibraries/LibWeb/CSS/Rust/build.rsLibraries/LibWeb/CSS/Rust/cbindgen.tomlLibraries/LibWeb/CSS/Rust/src/lib.rsLibraries/LibWeb/Layout/Rust/Cargo.tomlLibraries/LibWeb/Layout/Rust/build.rsLibraries/LibWeb/Layout/Rust/cbindgen.tomlLibraries/LibWeb/Layout/Rust/src/lib.rsLibraries/LibWeb/Rust/Cargo.tomlLibraries/LibWeb/Rust/build.rsLibraries/LibWeb/Rust/src/css/animation.rsLibraries/LibWeb/Rust/src/css/calc.rsLibraries/LibWeb/Rust/src/css/cascaded_properties.rsLibraries/LibWeb/Rust/src/css/color_conversion.rsLibraries/LibWeb/Rust/src/css/color_interpolation.rsLibraries/LibWeb/Rust/src/css/computed_value_types.rsLibraries/LibWeb/Rust/src/css/computed_values.rsLibraries/LibWeb/Rust/src/css/css_enums.rsLibraries/LibWeb/Rust/src/css/css_pixels.rsLibraries/LibWeb/Rust/src/css/css_tokenizer.rsLibraries/LibWeb/Rust/src/css/custom_properties.rsLibraries/LibWeb/Rust/src/css/display.rsLibraries/LibWeb/Rust/src/css/ffi_stats.rsLibraries/LibWeb/Rust/src/css/ffi_support.rsLibraries/LibWeb/Rust/src/css/mod.rsLibraries/LibWeb/Rust/src/css/property_metadata.rsLibraries/LibWeb/Rust/src/css/selector_engine.rsLibraries/LibWeb/Rust/src/css/style_compute.rsLibraries/LibWeb/Rust/src/css/style_value.rsLibraries/LibWeb/Rust/src/css/transition.rsLibraries/LibWeb/Rust/src/layout/layout_node_arena.rsLibraries/LibWeb/Rust/src/layout/mod.rsLibraries/LibWeb/Rust/src/layout/node_data.rsLibraries/LibWeb/Rust/src/layout/tree_builder.rsLibraries/LibWeb/Rust/src/lib.rs
💤 Files with no reviewable changes (9)
- Libraries/LibWeb/Layout/Rust/cbindgen.toml
- Libraries/LibWeb/CSS/Rust/cbindgen.toml
- Libraries/LibWeb/Layout/Rust/build.rs
- Libraries/LibWeb/Layout/Rust/src/lib.rs
- Libraries/LibWeb/Layout/Rust/Cargo.toml
- Cargo.toml
- Libraries/LibWeb/CSS/Rust/build.rs
- Libraries/LibWeb/CSS/Rust/src/lib.rs
- Libraries/LibWeb/CSS/Rust/Cargo.toml
🛑 Comments failed to post (4)
Libraries/LibWeb/Rust/src/css/css_tokenizer.rs (1)
467-481: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -C6 'consume_comment' --type=cppRepository: LadybirdBrowser/ladybird
Length of output: 162
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the tokenizer file and nearby comment-handling code. git ls-files | rg 'Libraries/LibWeb/Rust/src/css/css_tokenizer\.rs|css_tokenizer\.rs|consume_comment|consume comments|comment' echo '---' wc -l Libraries/LibWeb/Rust/src/css/css_tokenizer.rs echo '---' sed -n '430,520p' Libraries/LibWeb/Rust/src/css/css_tokenizer.rs echo '---' # Search for prior/parallel implementations in the repo. rg -n -C3 'is_eof\(first\) \|\| is_eof\(second\)|peek_twin\(\)|consume_comment|consume comments' Libraries -g '!**/target/**' -g '!**/node_modules/**'Repository: LadybirdBrowser/ladybird
Length of output: 19025
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path p = Path('Libraries/LibWeb/Rust/src/css/css_tokenizer.rs') text = p.read_text() for needle in [ 'if is_eof(first) || is_eof(second) {', 'if is_eof(first) {', 'if is_asterisk(first) && is_solidus(second) {', ]: print(needle, text.find(needle)) PYRepository: LadybirdBrowser/ladybird
Length of output: 279
🏁 Script executed:
#!/bin/bash set -euo pipefail wc -l Tests/LibWeb/CSSTokenizer/expected/malformed-unterminated-comment.txt Tests/LibWeb/CSSTokenizer/input/malformed-unterminated-comment.css echo '--- INPUT ---' cat -n Tests/LibWeb/CSSTokenizer/input/malformed-unterminated-comment.css echo '--- EXPECTED ---' cat -n Tests/LibWeb/CSSTokenizer/expected/malformed-unterminated-comment.txtRepository: LadybirdBrowser/ladybird
Length of output: 889
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n --glob 'Tests/LibWeb/CSSTokenizer/**' 'unterminated comment|comment.*EOF|EOF.*comment|malformed' Tests/LibWeb/CSSTokenizer echo '---' find Tests/LibWeb/CSSTokenizer -maxdepth 2 -type f | sortRepository: LadybirdBrowser/ladybird
Length of output: 2407
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '960,1015p' Libraries/LibWeb/Rust/src/css/css_tokenizer.rs echo '---' sed -n '1015,1085p' Libraries/LibWeb/Rust/src/css/css_tokenizer.rs echo '---' sed -n '1010,1065p' Libraries/LibWeb/CSS/Parser/Tokenizer.cppRepository: LadybirdBrowser/ladybird
Length of output: 7695
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '983,1015p' Libraries/LibWeb/CSS/Parser/Tokenizer.cppRepository: LadybirdBrowser/ladybird
Length of output: 1345
Unterminated comments stop one code point too early.
if is_eof(first) || is_eof(second)returns before consuming the last non-EOF code point, so/* abcleavescto be tokenized separately instead of folding the whole comment into the whitespace token. Break only whenfirstis EOF.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWeb/Rust/src/css/css_tokenizer.rs` around lines 467 - 481, The comment-scanning loop in the CSS tokenizer must consume the final non-EOF code point of unterminated comments. Update the EOF guard in the loop around peek_twin to stop only when first is EOF, while preserving the existing terminator detection and consumption behavior for closed comments.Libraries/LibWeb/Rust/src/css/ffi_stats.rs (1)
87-97: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bounds-check the index before indexing the counter tables.
Both accessors index
FFI_OP_NAMES/COUNTERSwith a caller-suppliedusize; an out-of-range index panics inside anextern "C"function (aborting the process) instead of returning a benign value. The other FFI entrypoints in this crate go throughcrate::abort_on_panic, so this is also inconsistent.🛡️ Proposed fix
pub extern "C" fn rust_style_ffi_counter_name(index: usize) -> *const u8 { - FFI_OP_NAMES[index].as_ptr() + FFI_OP_NAMES + .get(index) + .map_or(std::ptr::null(), |name| name.as_ptr()) } /// Returns the current value of the counter at `index`. #[unsafe(no_mangle)] pub extern "C" fn rust_style_ffi_counter_value(index: usize) -> u64 { - COUNTERS[index].load(Ordering::Relaxed) + COUNTERS.get(index).map_or(0, |counter| counter.load(Ordering::Relaxed)) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./// Returns the nul-terminated name of the counter at `index`. #[unsafe(no_mangle)] pub extern "C" fn rust_style_ffi_counter_name(index: usize) -> *const u8 { FFI_OP_NAMES .get(index) .map_or(std::ptr::null(), |name| name.as_ptr()) } /// Returns the current value of the counter at `index`. #[unsafe(no_mangle)] pub extern "C" fn rust_style_ffi_counter_value(index: usize) -> u64 { COUNTERS.get(index).map_or(0, |counter| counter.load(Ordering::Relaxed)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWeb/Rust/src/css/ffi_stats.rs` around lines 87 - 97, Update rust_style_ffi_counter_name and rust_style_ffi_counter_value to validate the caller-supplied index before accessing FFI_OP_NAMES or COUNTERS, returning a benign fallback for out-of-range indices. Route the access through the crate’s existing abort_on_panic pattern, consistent with the other FFI entrypoints, so no unchecked indexing panic can escape these extern functions.Libraries/LibWeb/Rust/src/css/ffi_support.rs (1)
45-68: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Other (CWE-119): Improper Restriction of Operations within the Bounds of a Memory Buffer
Reachability: Internal
DomStringView::newis a safe fn that hands out a type whose safe methods dereference raw pointers.Two gaps in the stated contract:
newtakes an uncheckedFfiDomStringView(whose fields arepub) yet is safe, whilecode_unit_at— also safe — dereferencesview.data. Any incorrectdata/length/is_asciitriple is UB reachable from safe code. Makingnewanunsafe fnputs the obligation where the doc comment already says it lives.- The lifetime
'ais unconstrained bynew(the impl block isDomStringView<'_>), so the caller can infer any lifetime and thePhantomData<&'a FfiCallScope>never actually pins the view to the call scope. Taking&'a FfiCallScopeas a parameter would enforce it.🛡️ Proposed fix
-impl DomStringView<'_> { +impl<'a> DomStringView<'a> { /// The caller vouches that `view` borrows storage which stays valid for the current FFI call; /// the lifetime parameter ties the wrapper to that call scope. - pub(crate) fn new(view: FfiDomStringView) -> Self { + /// + /// # Safety + /// `view.data` must point at `view.length` valid bytes (ASCII) or aligned UTF-16 code units + /// that stay alive for the whole of `scope`. + pub(crate) unsafe fn new(view: FfiDomStringView, _scope: &'a FfiCallScope) -> Self { Self { view, marker: PhantomData, } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.impl<'a> DomStringView<'a> { /// The caller vouches that `view` borrows storage which stays valid for the current FFI call; /// the lifetime parameter ties the wrapper to that call scope. /// /// # Safety /// `view.data` must point at `view.length` valid bytes (ASCII) or aligned UTF-16 code units /// that stay alive for the whole of `scope`. pub(crate) unsafe fn new(view: FfiDomStringView, _scope: &'a FfiCallScope) -> Self { Self { view, marker: PhantomData, } } pub(crate) fn len(self) -> usize { self.view.length } pub(crate) fn code_unit_at(self, index: usize) -> u16 { assert!(index < self.len()); assert!(!self.view.data.is_null()); if self.view.is_ascii { // SAFETY: C++ guarantees that an ASCII view points at `length` bytes. return u16::from(unsafe { *(self.view.data.cast::<u8>().add(index)) }); } // SAFETY: C++ guarantees that a UTF-16 view points at `length` aligned code units. unsafe { *(self.view.data.cast::<u16>().add(index)) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWeb/Rust/src/css/ffi_support.rs` around lines 45 - 68, Make DomStringView::new unsafe and require an &'a FfiCallScope parameter so callers explicitly uphold the raw-pointer contract and the wrapper lifetime is tied to the active FFI call scope. Update its documentation and all call sites to pass the scope and use an unsafe block, while preserving len and code_unit_at behavior.Libraries/LibWeb/Rust/src/css/property_metadata.rs (1)
30-45: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
longhand_indexonly validates in debug builds, yet it backs FFI entrypoints that accept an arbitraryu16.
rust_property_metadata_requires_computation_level,rust_property_metadata_animation_typeandrust_property_metadata_numeric_rangespass a caller-suppliedproperty_idstraight through. In release thedebug_assert!is gone, theu16subtraction wraps for shorthand/custom IDs, and the table index panics inside anextern "C"function — i.e. an abort rather than a diagnosable error. A cheapassert!(or returning a default for out-of-range IDs) keeps the contract enforced in shipped builds.🛡️ Proposed fix
fn longhand_index(property_id: u16) -> usize { - debug_assert!((FIRST_LONGHAND_PROPERTY_ID..=LAST_LONGHAND_PROPERTY_ID).contains(&property_id)); + assert!( + (FIRST_LONGHAND_PROPERTY_ID..=LAST_LONGHAND_PROPERTY_ID).contains(&property_id), + "property_id {property_id} is not a longhand" + ); (property_id - FIRST_LONGHAND_PROPERTY_ID) as usize }The same reasoning applies to
property_indexon Line 143.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn longhand_index(property_id: u16) -> usize { assert!( (FIRST_LONGHAND_PROPERTY_ID..=LAST_LONGHAND_PROPERTY_ID).contains(&property_id), "property_id {property_id} is not a longhand" ); (property_id - FIRST_LONGHAND_PROPERTY_ID) as usize } pub fn property_is_inherited(property_id: u16) -> bool { (FIRST_INHERITED_PROPERTY_ID..=LAST_INHERITED_PROPERTY_ID).contains(&property_id) } pub fn property_requires_computation_level(property_id: u16) -> u8 { REQUIRES_COMPUTATION_LEVELS[longhand_index(property_id)] } pub fn property_animation_type(property_id: u16) -> u8 { PROPERTY_ANIMATION_TYPES[longhand_index(property_id)] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Libraries/LibWeb/Rust/src/css/property_metadata.rs` around lines 30 - 45, Make longhand property ID validation active in release builds by replacing the debug-only check in longhand_index with an unconditional assert before subtraction and indexing. Apply the same validation approach to property_index, and ensure the FFI-backed metadata accessors property_requires_computation_level, property_animation_type, and property_metadata_numeric_ranges cannot index tables with out-of-range IDs.
kalenikaliaksandr
merged commit Jul 28, 2026
3f94f3d
into
LadybirdBrowser:master
19 of 22 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
LibWeb currently builds three separate Rust staticlibs:
libweb_rust,libweb_css_rustandlibweb_layout_rust. Rust code in one staticlib cannot reference types in another, so even though style computation already stores computed values in Rust-defined style group payloads, a Rust layout engine sitting in its own crate can only reach that data through C++-mediated indirection: runtime-registered byte-offset schemas, per-pass re-encoded value snapshots, and hand-copied enum constant tables that must be kept in sync with the generated ones by convention.This PR removes that boundary before the layout engine lands. The two commits move style computation and then layout tree building into
libweb_rustascssandlayoutmodules. With one crate, upcoming layout work can read the Rust-native style group payloads as their actual types, use the enum constants generated from Enums.json directly, and call calc/style-value functions as plain Rust calls instead of extern "C" round trips.