Skip to content

Commit

Permalink
Revert "Bug 1842221 - Simplify ThinArc and friends. r=boris"
Browse files Browse the repository at this point in the history
This reverts commit 516aec4.

It could made Servo crash, e.g. when loading
http://wpt.live/css/css-tables/html-to-css-mapping-2.html
The stack traces weren't consistent, I suspect Undefined Behavior.
  • Loading branch information
Loirooriol committed Feb 28, 2024
1 parent c88d806 commit e2a4834
Show file tree
Hide file tree
Showing 6 changed files with 343 additions and 151 deletions.
30 changes: 20 additions & 10 deletions selectors/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

use crate::parser::{Combinator, Component, RelativeSelector, Selector, SelectorImpl};
use crate::sink::Push;
use servo_arc::{Arc, ThinArc};
use servo_arc::{Arc, HeaderWithLength, ThinArc};
use smallvec::{self, SmallVec};
use std::cmp;
use std::iter;
Expand Down Expand Up @@ -105,22 +105,32 @@ impl<Impl: SelectorImpl> SelectorBuilder<Impl> {
&mut self,
spec: SpecificityAndFlags,
) -> ThinArc<SpecificityAndFlags, Component<Impl>> {
// First, compute the total number of Components we'll need to allocate
// space for.
let full_len = self.simple_selectors.len() + self.combinators.len();

// Create the header.
let header = HeaderWithLength::new(spec, full_len);

// Create the Arc using an iterator that drains our buffers.
// Panic-safety: if SelectorBuilderIter is not iterated to the end, some simple selectors
// will safely leak.
let raw_simple_selectors = unsafe {
let simple_selectors_len = self.simple_selectors.len();
self.simple_selectors.set_len(0);
std::slice::from_raw_parts(self.simple_selectors.as_ptr(), simple_selectors_len)
};
let (rest, current) = split_from_end(raw_simple_selectors, self.current_len);

// Use a raw pointer to be able to call set_len despite "borrowing" the slice.
// This is similar to SmallVec::drain, but we use a slice here because
// we’re gonna traverse it non-linearly.
let raw_simple_selectors: *const [Component<Impl>] = &*self.simple_selectors;
unsafe {
// Panic-safety: if SelectorBuilderIter is not iterated to the end,
// some simple selectors will safely leak.
self.simple_selectors.set_len(0)
}
let (rest, current) = split_from_end(unsafe { &*raw_simple_selectors }, self.current_len);
let iter = SelectorBuilderIter {
current_simple_selectors: current.iter(),
rest_of_simple_selectors: rest,
combinators: self.combinators.drain(..).rev(),
};

Arc::from_header_and_iter(spec, iter)
Arc::into_thin(Arc::from_header_and_iter(header, iter))
}
}

Expand Down
42 changes: 22 additions & 20 deletions selectors/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use cssparser::{BasicParseError, BasicParseErrorKind, ParseError, ParseErrorKind
use cssparser::{CowRcStr, Delimiter, SourceLocation};
use cssparser::{Parser as CssParser, ToCss, Token};
use precomputed_hash::PrecomputedHash;
use servo_arc::{ThinArc, UniqueArc};
use servo_arc::{HeaderWithLength, ThinArc, UniqueArc};
use smallvec::SmallVec;
use std::borrow::{Borrow, Cow};
use std::fmt::{self, Debug};
Expand Down Expand Up @@ -649,7 +649,7 @@ pub struct Selector<Impl: SelectorImpl>(
impl<Impl: SelectorImpl> Selector<Impl> {
/// See Arc::mark_as_intentionally_leaked
pub fn mark_as_intentionally_leaked(&self) {
self.0.mark_as_intentionally_leaked()
self.0.with_arc(|a| a.mark_as_intentionally_leaked())
}

fn ampersand() -> Self {
Expand All @@ -664,32 +664,32 @@ impl<Impl: SelectorImpl> Selector<Impl> {

#[inline]
pub fn specificity(&self) -> u32 {
self.0.header.specificity()
self.0.header.header.specificity()
}

#[inline]
fn flags(&self) -> SelectorFlags {
self.0.header.flags
self.0.header.header.flags
}

#[inline]
pub fn has_pseudo_element(&self) -> bool {
self.0.header.has_pseudo_element()
self.0.header.header.has_pseudo_element()
}

#[inline]
pub fn has_parent_selector(&self) -> bool {
self.0.header.has_parent_selector()
self.0.header.header.has_parent_selector()
}

#[inline]
pub fn is_slotted(&self) -> bool {
self.0.header.is_slotted()
self.0.header.header.is_slotted()
}

#[inline]
pub fn is_part(&self) -> bool {
self.0.header.is_part()
self.0.header.header.is_part()
}

#[inline]
Expand Down Expand Up @@ -779,7 +779,7 @@ impl<Impl: SelectorImpl> Selector<Impl> {
}

SelectorIter {
iter: self.0.slice()[..self.len() - 2].iter(),
iter: self.0.slice[..self.len() - 2].iter(),
next_combinator: None,
}
}
Expand Down Expand Up @@ -811,7 +811,7 @@ impl<Impl: SelectorImpl> Selector<Impl> {
/// skipping the rightmost |offset| Components.
#[inline]
pub fn iter_from(&self, offset: usize) -> SelectorIter<Impl> {
let iter = self.0.slice()[offset..].iter();
let iter = self.0.slice[offset..].iter();
SelectorIter {
iter,
next_combinator: None,
Expand All @@ -822,7 +822,7 @@ impl<Impl: SelectorImpl> Selector<Impl> {
/// or panics if the component is not a combinator.
#[inline]
pub fn combinator_at_match_order(&self, index: usize) -> Combinator {
match self.0.slice()[index] {
match self.0.slice[index] {
Component::Combinator(c) => c,
ref other => panic!(
"Not a combinator: {:?}, {:?}, index: {}",
Expand All @@ -835,14 +835,14 @@ impl<Impl: SelectorImpl> Selector<Impl> {
/// combinators, in matching order (from right to left).
#[inline]
pub fn iter_raw_match_order(&self) -> slice::Iter<Component<Impl>> {
self.0.slice().iter()
self.0.slice.iter()
}

/// Returns the combinator at index `index` (zero-indexed from the left),
/// or panics if the component is not a combinator.
#[inline]
pub fn combinator_at_parse_order(&self, index: usize) -> Combinator {
match self.0.slice()[self.len() - index - 1] {
match self.0.slice[self.len() - index - 1] {
Component::Combinator(c) => c,
ref other => panic!(
"Not a combinator: {:?}, {:?}, index: {}",
Expand All @@ -856,7 +856,7 @@ impl<Impl: SelectorImpl> Selector<Impl> {
/// `offset`.
#[inline]
pub fn iter_raw_parse_order_from(&self, offset: usize) -> Rev<slice::Iter<Component<Impl>>> {
self.0.slice()[..self.len() - offset].iter().rev()
self.0.slice[..self.len() - offset].iter().rev()
}

/// Creates a Selector from a vec of Components, specified in parse order. Used in tests.
Expand Down Expand Up @@ -981,7 +981,8 @@ impl<Impl: SelectorImpl> Selector<Impl> {
.chain(std::iter::once(Component::Is(
parent.to_vec().into_boxed_slice(),
)));
UniqueArc::from_header_and_iter_with_size(specificity_and_flags, iter, len)
let header = HeaderWithLength::new(specificity_and_flags, len);
UniqueArc::from_header_and_iter_with_size(header, iter, len)
} else {
let iter = self.iter_raw_match_order().map(|component| {
use self::Component::*;
Expand Down Expand Up @@ -1073,16 +1074,17 @@ impl<Impl: SelectorImpl> Selector<Impl> {
)),
}
});
UniqueArc::from_header_and_iter(specificity_and_flags, iter)
let header = HeaderWithLength::new(specificity_and_flags, iter.len());
UniqueArc::from_header_and_iter(header, iter)
};
items.header_mut().specificity = specificity.into();
Selector(items.shareable())
Selector(items.shareable_thin())
}

/// Returns count of simple selectors and combinators in the Selector.
#[inline]
pub fn len(&self) -> usize {
self.0.len()
self.0.slice.len()
}

/// Returns the address on the heap of the ThinArc for memory reporting.
Expand Down Expand Up @@ -1481,13 +1483,13 @@ impl<Impl: SelectorImpl> NthOfSelectorData<Impl> {
/// Returns the An+B part of the selector
#[inline]
pub fn nth_data(&self) -> &NthSelectorData {
&self.0.header
&self.0.header.header
}

/// Returns the selector list part of the selector
#[inline]
pub fn selectors(&self) -> &[Selector<Impl>] {
self.0.slice()
&self.0.slice
}
}

Expand Down
Loading

0 comments on commit e2a4834

Please sign in to comment.