Skip to content

servo_arc: bad drop in WASM target #129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
noahbald opened this issue Mar 1, 2025 · 8 comments · Fixed by #174
Closed

servo_arc: bad drop in WASM target #129

noahbald opened this issue Mar 1, 2025 · 8 comments · Fixed by #174

Comments

@noahbald
Copy link

noahbald commented Mar 1, 2025

Sorry if this issue is a bit vague because I'm having a hard time finding the exact cause.
I'm compiling to WASM using wasm-bindgen 0.2.100 and when running I receive the following panic

panicked at /rust/deps/dlmalloc-0.2.7/src/dlmalloc.rs:1198:13:
assertion failed: psize <= size + max_overhead

And this seems to happening when I create a selector with a selector list, e.g.

pub struct Parser<E: crate::element::Element> {
    element: PhantomData<E>,
}

impl<'i, E: crate::element::Element> selectors::parser::Parser<'i> for Parser<E> {
    type Impl = E::Impl;
    type Error = SelectorParseErrorKind<'i>;
}

impl<E: crate::element::Element> Default for Parser<E> {
    fn default() -> Self {
        Self {
            element: PhantomData,
        }
    }
}

impl<E: crate::element::Element> Selector<E> {
    pub fn matches_with_scope_and_cache(
        &self,
        element: &E,
        scope: Option<E>,
        selector_caches: &mut SelectorCaches,
    ) -> bool {
        let context = &mut matching::MatchingContext::new(
            matching::MatchingMode::Normal,
            None,
            selector_caches,
            matching::QuirksMode::NoQuirks,
            matching::NeedsSelectorFlags::No,
            matching::MatchingForInvalidation::No,
        );
        context.scope_element = scope.map(|x| selectors::Element::opaque(&x));
        matching::matches_selector_list(&self.0, element, context)
    }
}

#[test]
fn bad_demalloc() {
  // This will panic in wasm
  Selector::new( "script,a" )
}

related: rustwasm/wasm-pack#1389

@noahbald
Copy link
Author

noahbald commented Mar 5, 2025

https://github.com/noahbald/stylo-selector-fail-129
Minimal reproduction using scraper

@noahbald
Copy link
Author

noahbald commented Mar 6, 2025

I've found this line to be the source of the panic

std::ptr::drop_in_place(inner);

@noahbald
Copy link
Author

noahbald commented Mar 6, 2025

I've found that adding the following to /selectors/Cargo.toml resolves the issue

[target.'cfg(target_arch = "wasm32")'.dependencies.servo_arc]
path = "../servo_arc"
features = ["track_alloc_size"]

Any maintainer know if this is a reasonable fix?

@noahbald noahbald changed the title selectors: bad malloc when compiling to WASM servo_arc: bad drop in WASM target Mar 6, 2025
@noahbald
Copy link
Author

noahbald commented Mar 6, 2025

Also, updating my crate's Cargo.toml to include servo_arc with track_alloc_size enabled fixes the issue, so a patch may not be needed. Perhaps should be added to docs?

@jdm
Copy link
Member

jdm commented Mar 21, 2025

I suspect that none of the maintainers (like myself) have any idea why this happens and why that feature fixes the issue. If you have suggestions for how to document it, I'm listening!

@nicoburns
Copy link
Collaborator

I suspect that none of the maintainers (like myself) have any idea why this happens and why that feature fixes the issue. If you have suggestions for how to document it, I'm listening!

I believe it is generally the case that one needs to track allocation size to safely deallocate. jemalloc which both Servo and Firefox use is a bit of an exception in that regard.

The alloc_size field enabled by this feature is documented by the following comment:

// NOTE(emilio): This needs to be here so that HeaderSlice<> is deallocated properly if the
// allocator relies on getting the right Layout. We don't need to track the right alignment,
// since we know that statically.
//
// This member could be completely avoided once min_specialization feature is stable (by
// implementing a trait for HeaderSlice that gives you the right layout). For now, servo-only
// since Gecko doesn't need it (its allocator doesn't need the size for the alignments we care
// about). See https://github.com/rust-lang/rust/issues/31844.
alloc_size: usize,

Probably track_alloc_size ought to be a default feature.

@Loirooriol
Copy link
Contributor

Some context in #28

@kornelski
Copy link

This isn't limited to WASM. It also fails MIRI tests. Here's a reproducer:

[dependencies]
selectors = "0.27"
cssparser = "0.35"
precomputed-hash = "0.1.1"

rustc 1.88.0-nightly (077cedc2a 2025-04-19)

#[test]
fn native_test() {
    drop(SelectorsParser::parse("a[href]"));
}

use cssparser::{Parser as CssParser, ParserInput, ToCss};
use selectors::parser::{
    NonTSPseudoClass, Parser, PseudoElement, SelectorImpl, SelectorList,
    SelectorParseErrorKind,
};
use selectors::parser::ParseRelative;
use std::fmt;

#[derive(Debug, Clone, Eq, PartialEq)]
pub(crate) struct SelectorImplDescriptor;

#[derive(Clone, Default, Eq, PartialEq)]
pub struct CssString(Box<str>);

impl<'a> From<&'a str> for CssString {
    fn from(value: &'a str) -> Self {
        Self(value.into())
    }
}

impl ToCss for CssString {
    fn to_css<W: fmt::Write>(&self, dest: &mut W) -> fmt::Result {
        dest.write_str(&self.0)
    }
}

impl precomputed_hash::PrecomputedHash for CssString {
    fn precomputed_hash(&self) -> u32 {
        if let Some(v) = self.0.as_bytes().get(..4) {
            let mut tmp = [0u8; 4];
            tmp.copy_from_slice(v);
            u32::from_ne_bytes(tmp)
        } else {
            0
        }
    }
}
impl precomputed_hash::PrecomputedHash for Namespace {
    fn precomputed_hash(&self) -> u32 {
        *self as u32
    }
}

// Pub only for integration tests
#[derive(Default, Copy, Clone, Eq, PartialEq, Debug)]
pub enum Namespace {
    #[default]
    Html = 0,
    Svg = 1,
    MathML = 2,
}

impl Namespace {
    #[inline]
    #[must_use]
    pub const fn uri(self) -> &'static str {
        use Namespace::{Html, MathML, Svg};

        // NOTE: https://infra.spec.whatwg.org/#namespaces
        match self {
            Html => "http://www.w3.org/1999/xhtml",
            Svg => "http://www.w3.org/2000/svg",
            MathML => "http://www.w3.org/1998/Math/MathML",
        }
    }
}

impl SelectorImpl for SelectorImplDescriptor {
    type AttrValue = CssString;
    type Identifier = CssString;
    type LocalName = CssString;
    type NamespacePrefix = CssString;
    type NamespaceUrl = CssString;
    type BorrowedNamespaceUrl = CssString;
    type BorrowedLocalName = CssString;

    type NonTSPseudoClass = NonTSPseudoClassStub;
    type PseudoElement = PseudoElementStub;

    type ExtraMatchingData<'unused> = ();
}

#[derive(PartialEq, Eq, Clone, Debug, Hash)]
pub(crate) enum PseudoElementStub {}

impl ToCss for PseudoElementStub {
    fn to_css<W: fmt::Write>(&self, _dest: &mut W) -> fmt::Result {
        Ok(())
    }
}

impl PseudoElement for PseudoElementStub {
    type Impl = SelectorImplDescriptor;
}

#[derive(PartialEq, Eq, Clone, Debug, Hash)]
pub(crate) enum NonTSPseudoClassStub {}

impl NonTSPseudoClass for NonTSPseudoClassStub {
    type Impl = SelectorImplDescriptor;

    fn is_active_or_hover(&self) -> bool {
        false
    }

    fn is_user_action_state(&self) -> bool {
        false
    }
}

impl ToCss for NonTSPseudoClassStub {
    fn to_css<W: fmt::Write>(&self, _dest: &mut W) -> fmt::Result {
        Ok(())
    }
}

#[allow(dead_code)]
struct SelectorsParser;

impl SelectorsParser {
    pub fn parse(selector: &str) -> Result<SelectorList<SelectorImplDescriptor>, ()> {
        let mut input = ParserInput::new(selector);
        let mut css_parser = CssParser::new(&mut input);

        SelectorList::parse(&Self, &mut css_parser, ParseRelative::No).map_err(drop)
    }
}

impl<'i> Parser<'i> for SelectorsParser {
    type Impl = SelectorImplDescriptor;
    type Error = SelectorParseErrorKind<'i>;
}
Miri crash log
test native_test ... error: Undefined Behavior: attempting a write access using <286855> at alloc121684[0x18], but that tag does not exist in the borrow stack for this location
   --> ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/servo_arc-0.4.0/lib.rs:806:21
    |
806 | /                     ptr::write(
807 | |                         current,
808 | |                         items
809 | |                             .next()
810 | |                             .expect("ExactSizeIterator over-reported length"),
811 | |                     );
    | |                     ^
    | |                     |
    | |_____________________attempting a write access using <286855> at alloc121684[0x18], but that tag does not exist in the borrow stack for this location
    |                       this error occurs as part of an access at alloc121684[0x18..0x40]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <286855> would have been created here, but this is a zero-size retag ([0x18..0x18]) so the tag in question does not exist anywhere
   --> src/lib.rs:141:9
    |
141 |         SelectorList::parse(&Self, &mut css_parser, ParseRelative::No).map_err(drop)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: BACKTRACE (of the first span) on thread `native_test`:
    = note: inside `servo_arc::Arc::<servo_arc::HeaderSlice<selectors::builder::SpecificityAndFlags, selectors::parser::Component<SelectorImplDescriptor>>>::from_header_and_iter_alloc::<{closure@servo_arc::Arc<servo_arc::HeaderSlice<selectors::builder::SpecificityAndFlags, selectors::parser::Component<SelectorImplDescriptor>>>::from_header_and_iter_with_size<selectors::builder::ExactChain<smallvec::Drain<'_, [selectors::parser::Component<SelectorImplDescriptor>; 32]>, std::iter::Cloned<std::slice::Iter<'_, selectors::parser::Component<SelectorImplDescriptor>>>>>::{closure#0}}, selectors::builder::ExactChain<smallvec::Drain<'_, [selectors::parser::Component<SelectorImplDescriptor>; 32]>, std::iter::Cloned<std::slice::Iter<'_, selectors::parser::Component<SelectorImplDescriptor>>>>>` at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/servo_arc-0.4.0/lib.rs:806:21: 811:22
    = note: inside `servo_arc::Arc::<servo_arc::HeaderSlice<selectors::builder::SpecificityAndFlags, selectors::parser::Component<SelectorImplDescriptor>>>::from_header_and_iter_with_size::<selectors::builder::ExactChain<smallvec::Drain<'_, [selectors::parser::Component<SelectorImplDescriptor>; 32]>, std::iter::Cloned<std::slice::Iter<'_, selectors::parser::Component<SelectorImplDescriptor>>>>>` at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/servo_arc-0.4.0/lib.rs:855:9: 861:10
    = note: inside `servo_arc::Arc::<servo_arc::HeaderSlice<selectors::builder::SpecificityAndFlags, selectors::parser::Component<SelectorImplDescriptor>>>::from_header_and_iter::<selectors::builder::ExactChain<smallvec::Drain<'_, [selectors::parser::Component<SelectorImplDescriptor>; 32]>, std::iter::Cloned<std::slice::Iter<'_, selectors::parser::Component<SelectorImplDescriptor>>>>>` at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/servo_arc-0.4.0/lib.rs:872:9: 872:65
    = note: inside `selectors::builder::SelectorBuilder::<SelectorImplDescriptor>::build_with_specificity_and_flags` at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/selectors-0.27.0/builder.rs:125:20: 125:125
    = note: inside `selectors::builder::SelectorBuilder::<SelectorImplDescriptor>::build` at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/selectors-0.27.0/builder.rs:94:9: 94:66
    = note: inside `selectors::parser::parse_selector::<'_, SelectorsParser, SelectorImplDescriptor>` at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/selectors-0.27.0/parser.rs:2752:24: 2752:53
    = note: inside closure at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/selectors-0.27.0/parser.rs:567:36: 567:88
    = note: inside `cssparser::Parser::<'_, '_>::parse_entirely::<{closure@selectors::SelectorList<SelectorImplDescriptor>::parse_with_state<'_, SelectorsParser>::{closure#0}}, selectors::parser::Selector<SelectorImplDescriptor>, selectors::parser::SelectorParseErrorKind<'_>>` at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cssparser-0.35.0/src/parser.rs:693:22: 693:33
    = note: inside `cssparser::parser::parse_until_before::<'_, '_, {closure@selectors::SelectorList<SelectorImplDescriptor>::parse_with_state<'_, SelectorsParser>::{closure#0}}, selectors::parser::Selector<SelectorImplDescriptor>, selectors::parser::SelectorParseErrorKind<'_>>` at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cssparser-0.35.0/src/parser.rs:1063:18: 1063:56
    = note: inside `cssparser::Parser::<'_, '_>::parse_until_before::<{closure@selectors::SelectorList<SelectorImplDescriptor>::parse_with_state<'_, SelectorsParser>::{closure#0}}, selectors::parser::Selector<SelectorImplDescriptor>, selectors::parser::SelectorParseErrorKind<'_>>` at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cssparser-0.35.0/src/parser.rs:801:9: 801:86
    = note: inside `selectors::SelectorList::<SelectorImplDescriptor>::parse_with_state::<'_, SelectorsParser>` at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/selectors-0.27.0/parser.rs:565:28: 573:15
    = note: inside `selectors::SelectorList::<SelectorImplDescriptor>::parse::<'_, SelectorsParser>` at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/selectors-0.27.0/parser.rs:507:9: 513:10
note: inside `SelectorsParser::parse`
   --> src/lib.rs:141:9
    |
141 |         SelectorList::parse(&Self, &mut css_parser, ParseRelative::No).map_err(drop)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `native_test`
   --> src/lib.rs:14:10
    |
14  |     drop(SelectorsParser::parse("a[href]"));
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure
   --> src/lib.rs:13:17
    |
12  | #[test]
    | ------- in this procedural macro expansion
13  | fn native_test() {

And the same does fail in WASM too:

use wasm_bindgen::prelude::*;
#[wasm_bindgen]
pub fn do_crash() {
    #[cfg(feature = "console_error_panic_hook")]
    console_error_panic_hook::set_once();
    drop(SelectorsParser::parse("a[href]"));
}
panicked at /rust/deps/dlmalloc-0.2.8/src/dlmalloc.rs:1202:13:
assertion failed: psize <= size + max_overhead

kornelski added a commit to cloudflare/lol-html that referenced this issue Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants