Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Libraries/LibWeb/Rust/src/layout/formatting_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1649,7 +1649,7 @@ fn execute_formatting_context_run(
let root_used = std::rc::Rc::new(root_cells.materialize_record());
let run = FormattingContextRun {
purpose,
records: std::rc::Rc::new(RunRecords::new(box_, root_used)),
records: std::rc::Rc::new(RunRecords::new(callbacks.arena, box_, root_used)),
box_,
layout_mode,
callbacks,
Expand Down Expand Up @@ -2039,7 +2039,7 @@ pub unsafe extern "C" fn rust_layout_run_root_layout(
percentage_basis_block_size: Some(viewport_block_size),
..crate::layout::ContainingBlockConstraints::default()
};
let entry_records = std::rc::Rc::new(RunRecords::new_unrooted(root));
let entry_records = std::rc::Rc::new(RunRecords::new_unrooted(callbacks.arena, root));
let viewport_used = entry_records.create_used_values(&callbacks, root, root_constraints);
let entry_fragments = std::rc::Rc::new(RunFragmentBuilder::new_entry_accumulator(root));
let entry_run = FormattingContextRun {
Expand Down Expand Up @@ -2142,7 +2142,7 @@ pub unsafe extern "C" fn rust_layout_compute_subtree_layout(
let callbacks = unsafe { *callbacks };
let sink = unsafe { &*sink };

let entry_records = std::rc::Rc::new(RunRecords::new_unrooted(root));
let entry_records = std::rc::Rc::new(RunRecords::new_unrooted(callbacks.arena, root));
let root_used = used_values_from_paintable(&callbacks, root, paintable_to_replace)
.expect("partial relayout root must have committed geometry");
entry_records.register(root, root_used.clone());
Expand Down Expand Up @@ -2242,7 +2242,7 @@ pub unsafe extern "C" fn rust_layout_replay_saved_abspos_layout(
let containing_block = callbacks.containing_block(box_);
assert!(!containing_block.is_invalid());
let entry_fragments = std::rc::Rc::new(RunFragmentBuilder::new_entry_accumulator(containing_block));
let entry_records = std::rc::Rc::new(RunRecords::new_unrooted(containing_block));
let entry_records = std::rc::Rc::new(RunRecords::new_unrooted(callbacks.arena, containing_block));
let run = crate::layout::FormattingContextRun {
purpose: LayoutPurpose::Commit,
records: entry_records.clone(),
Expand Down
6 changes: 3 additions & 3 deletions Libraries/LibWeb/Rust/src/layout/grid_formatting_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2488,7 +2488,7 @@ impl GridFormattingContext {
scratch_root.has_definite_block_size.set(live.has_definite_block_size.get());
let scratch_run = FormattingContextRun {
purpose: LayoutPurpose::Measurement,
records: std::rc::Rc::new(RunRecords::new(subgrid.box_, scratch_root)),
records: std::rc::Rc::new(RunRecords::new(self.callbacks.arena, subgrid.box_, scratch_root)),
box_: subgrid.box_,
layout_mode: LayoutMode::IntrinsicSizing,
callbacks: self.callbacks,
Expand All @@ -2498,8 +2498,8 @@ impl GridFormattingContext {
};
let mut context = GridFormattingContext::new(&scratch_run, Some(self));
let mut available = self.available_space.unwrap();
if !axis.is_column() && self.used(subgrid).has_definite_inline_size() {
available.inline_size = AvailableSize::definite(self.used(subgrid).content_inline_size.get());
if !axis.is_column() && live.has_definite_inline_size() {
available.inline_size = AvailableSize::definite(live.content_inline_size.get());
}
let input = LayoutInput::new(available, self.track_sizing_constraints(), ParticipationInParentFormattingContext::Item);
context.reset_for_run(input);
Expand Down
79 changes: 79 additions & 0 deletions Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,14 @@ use crate::layout::AbsposLayoutInputs;
use crate::layout::AvailableSize;
use crate::layout::CssPixels;
use crate::layout::FfiReplacedContentFacts;
use crate::layout::UsedValues;
use crate::layout::node_data::{FfiStylePayloads, MAX_NODE_SLOT_COUNT, NodeData, NodeFlag, NodeSlotId};
use std::cell::Cell;
use std::cell::RefCell;
use std::collections::HashMap;
use std::ffi::c_void;
use std::hash::{Hash, Hasher};
use std::rc::Rc;
use std::thread;

pub(crate) const SLOTS_PER_CHUNK: usize = 256;
Expand Down Expand Up @@ -173,6 +176,12 @@ struct TextChunkCacheSlot {
entry: Option<Box<TextChunkCacheEntry>>,
}

#[derive(Default)]
struct RunRecordSlot {
nonce: u64, // 0 = vacant
record: Option<Rc<UsedValues>>,
}

// NodeData is sized to one cache line; the aligned chunk keeps every densely-strided slot
// line-aligned, and per-slot bookkeeping lives in a parallel array so it stays that way.
#[repr(align(64))]
Expand Down Expand Up @@ -218,6 +227,8 @@ pub(crate) struct LayoutNodeArena {
text_chunk_caches: RefCell<Vec<TextChunkCacheSlot>>,
replaced_content_facts: Vec<ReplacedContentFactsSlot>,
raw_table_column_spans: HashMap<NodeSlotId, u32>,
run_used_records: RefCell<Vec<RunRecordSlot>>,
next_run_nonce: Cell<u64>,
owner_thread: thread::ThreadId,
}

Expand All @@ -236,6 +247,8 @@ impl LayoutNodeArena {
text_chunk_caches: RefCell::new(Vec::new()),
replaced_content_facts: Vec::new(),
raw_table_column_spans: HashMap::new(),
run_used_records: RefCell::new(Vec::new()),
next_run_nonce: Cell::new(1),
owner_thread: thread::current().id(),
}
}
Expand Down Expand Up @@ -267,6 +280,9 @@ impl LayoutNodeArena {
self.chunks.push(chunk);
}
self.slot_metadata.push(SlotMetadata::default());
// Grown with the slot space up front: nearly every slot gets a run
// record each layout pass, so register() never has to resize.
self.run_used_records.get_mut().push(RunRecordSlot::default());
self.next_index = self
.next_index
.checked_add(1)
Expand Down Expand Up @@ -331,6 +347,15 @@ impl LayoutNodeArena {
if let Some(slot) = self.replaced_content_facts.get_mut(index as usize) {
*slot = ReplacedContentFactsSlot::default();
}
// free() never interleaves with a layout pass (C++ is blocked on the
// synchronous FFI entry), so a live record here means a run leaked.
if let Some(slot) = self.run_used_records.get_mut().get_mut(index as usize) {
debug_assert!(
slot.record.is_none(),
"layout node arena freed a slot with a live run record"
);
*slot = RunRecordSlot::default();
}
self.raw_table_column_spans.remove(&id);
*self.data_mut(index) = NodeData::default();

Expand Down Expand Up @@ -791,6 +816,60 @@ impl LayoutNodeArena {
unsafe { std::slice::from_raw_parts(entry.chunks.as_ptr(), entry.chunks.len()) }
}

pub(crate) fn allocate_run_nonce(&self) -> u64 {
let nonce = self.next_run_nonce.get();
self.next_run_nonce
.set(nonce.checked_add(1).expect("layout run nonce space exhausted"));
nonce
}

pub(crate) fn run_record(&self, slot_index: u32, run_nonce: u64) -> Option<Rc<UsedValues>> {
let records = self.run_used_records.borrow();
let slot = records.get(slot_index as usize)?;
if slot.nonce != run_nonce {
return None;
}
slot.record.clone()
}

pub(crate) fn replace_run_record(
&self,
slot_index: u32,
run_nonce: u64,
record: Rc<UsedValues>,
) -> Option<(u64, Rc<UsedValues>)> {
let mut records = self.run_used_records.borrow_mut();
let slot = records
.get_mut(slot_index as usize)
.expect("registered layout run record slot must exist");
let previous = std::mem::replace(
slot,
RunRecordSlot {
nonce: run_nonce,
record: Some(record),
},
);
previous.record.map(|record| (previous.nonce, record))
}

pub(crate) fn restore_run_record(&self, slot_index: u32, run_nonce: u64, previous: Option<(u64, Rc<UsedValues>)>) {
let mut records = self.run_used_records.borrow_mut();
let slot = records
.get_mut(slot_index as usize)
.expect("restored layout run record slot must exist");
debug_assert_eq!(
slot.nonce, run_nonce,
"layout run records were not restored in LIFO order"
);
*slot = match previous {
Some((nonce, record)) => RunRecordSlot {
nonce,
record: Some(record),
},
None => RunRecordSlot::default(),
};
Comment on lines +855 to +870

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Keep run-record lifecycle checks enabled in release builds.

debug_assert! and debug_assert_eq! are removed in release builds. A non-LIFO restore can overwrite another live run's record. Freeing a slot with a live record can clear that record before the run drops. Use assert! and assert_eq! at both sites.

  • Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs#L855-L870: reject a non-LIFO restoration before writing the slot.
  • Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs#L350-L358: reject freeing a slot that still has a live run record before clearing it.
📍 Affects 1 file
  • Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs#L855-L870 (this comment)
  • Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs#L350-L358
🤖 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/layout/layout_node_arena.rs` around lines 855 -
870, Replace the debug-only lifecycle checks with release-enforced assertions:
in restore_run_record, use assert_eq! to reject non-LIFO restoration before
overwriting the slot, and at the run-record slot freeing logic around lines
350-358, use assert! or assert_eq! to reject clearing a slot that still contains
a live record. Apply both changes in
Libraries/LibWeb/Rust/src/layout/layout_node_arena.rs at the specified ranges.

}

pub(crate) unsafe fn from_handle<'a>(arena: *mut c_void) -> &'a Self {
assert!(!arena.is_null(), "layout node arena handle is null");
// SAFETY: Layout passes borrow the document's arena synchronously,
Expand Down
52 changes: 43 additions & 9 deletions Libraries/LibWeb/Rust/src/layout/run_records.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,57 @@
* SPDX-License-Identifier: BSD-2-Clause
*/

/// The per-run registry of UsedValues records, backed by the slot-indexed side
/// table in the layout node arena. Registering a slot that a surrounding run
/// owns displaces that run's entry, and dropping the RunRecords restores it.
/// That is sound only because runs strictly nest on the call stack, so an
/// Rc<RunRecords> must never escape its run.
pub(crate) struct RunRecords {
root: Node,
map: RefCell<HashMap<u32, std::rc::Rc<UsedValues>>>,
arena: *mut c_void,
nonce: u64,
undo: RefCell<Vec<UndoEntry>>,
}

struct UndoEntry {
slot_index: u32,
previous: Option<(u64, std::rc::Rc<UsedValues>)>,
}

impl RunRecords {
pub(crate) fn new(root: Node, root_used: std::rc::Rc<UsedValues>) -> Self {
let records = Self::new_unrooted(root);
pub(crate) fn new(arena: *mut c_void, root: Node, root_used: std::rc::Rc<UsedValues>) -> Self {
let records = Self::new_unrooted(arena, root);
records.register(root, root_used);
records
}

pub(crate) fn new_unrooted(root: Node) -> Self {
pub(crate) fn new_unrooted(arena: *mut c_void, root: Node) -> Self {
// SAFETY: Layout passes borrow the document's arena synchronously, and
// the document keeps it alive for the duration of the pass.
let nonce = unsafe { LayoutNodeArena::from_handle(arena) }.allocate_run_nonce();
Self {
root,
map: RefCell::new(HashMap::new()),
arena,
nonce,
undo: RefCell::new(Vec::new()),
}
}

fn arena(&self) -> &LayoutNodeArena {
// SAFETY: See new_unrooted().
unsafe { LayoutNodeArena::from_handle(self.arena) }
}

pub(crate) fn register(&self, node: Node, used: std::rc::Rc<UsedValues>) {
let previous = self.map.borrow_mut().insert(node.slot_index(), used);
let slot_index = node.slot_index();
let previous = self.arena().replace_run_record(slot_index, self.nonce, used);
assert!(
previous.is_none(),
previous.as_ref().is_none_or(|(nonce, _)| *nonce != self.nonce),
"slot {} registered twice in the run rooted at slot {}",
node.slot_index(),
slot_index,
self.root.slot_index()
);
self.undo.borrow_mut().push(UndoEntry { slot_index, previous });
}

pub(crate) fn create_used_values(
Expand All @@ -57,6 +81,16 @@ impl RunRecords {
}

pub(crate) fn used_values_if_owned(&self, node: Node) -> Option<std::rc::Rc<UsedValues>> {
self.map.borrow().get(&node.slot_index()).cloned()
self.arena().run_record(node.slot_index(), self.nonce)
}
}

impl Drop for RunRecords {
fn drop(&mut self) {
let undo = std::mem::take(self.undo.get_mut());
let arena = self.arena();
for entry in undo.into_iter().rev() {
arena.restore_run_record(entry.slot_index, self.nonce, entry.previous);
}
}
}
2 changes: 1 addition & 1 deletion Libraries/LibWeb/Rust/src/layout/sizing_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1831,7 +1831,7 @@ impl SizingContext {

let table_run = crate::layout::FormattingContextRun {
purpose: LayoutPurpose::Measurement,
records: std::rc::Rc::new(RunRecords::new(table_box, table_used.clone())),
records: std::rc::Rc::new(RunRecords::new(measurement.callbacks().arena, table_box, table_used.clone())),
box_: table_box,
layout_mode: LayoutMode::IntrinsicSizing,
callbacks: *measurement.callbacks(),
Expand Down
Loading