Selectors cssparser upgrade

[paolobarbolini]

This takes the branch from #250 (comment) and applies some additional commits to make this work. I'm not sure if this is correct, but tests pass. The biggest change I did was making tests have [foo*=""] require attributes, which it wasn't expecting before.

Closes #250
Closes #262

[paolobarbolini]

I'd missed the additional tests that get run by the script. I'll try understanding what broke when I have time.

[kornelski]

there is a test failure. This seems like an edge case, so I'm not sure if that's an implementation problem, or the test needs updating.

<p class=\"\"><!--Replaced (p[class$=\"\"]) --></p>\n<p>This text should be green.</p>\n\n</body></html>"
<p class=\"\">This text should be green.</p>\n<p>This text should be green.</p>\n\n</body></html>"

[paolobarbolini]

I'm looking into it right now. The test is correct, so there's a regression somewhere.

[paolobarbolini]

I'm still not sure what changed but basically CSS doesn't consider an empty string to start or end by an empty string. selectors seems to check it here: https://github.com/servo/stylo/blob/0eaeea3dfd4aa0415529700353075ad1e1e47e5b/selectors/attr.rs#L121-L126. I'm still not sure how it worked before and why I had to add the check now but at least now tests seem happy.
Selectors also had this same bug at some point: servo/stylo@0c1f498

[kornelski]

So the current issue is in js-api where Node.js tests throw unreachable error from __rust_dealloc.

With console_error_panic_hook it shows:

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

I think this may be a legit use-after-free error, or at least an incompatibility of selectors with WASM.

I've tracked it down to drop(selector_str.parse::<Selector>());. The actual panic is at the end of for (selector, handlers) in element_content_handlers { block, but dropping of the Selector anywhere reproduces it.

The Selector contains SelectorList:

/// A selector list is a tagged pointer with either a single selector, or a ThinArc<()> of multiple
/// selectors.
#[derive(Clone, Eq, Debug, PartialEq)]
#[cfg_attr(feature = "to_shmem", derive(ToShmem))]
#[cfg_attr(feature = "to_shmem", shmem(no_bounds))]
pub struct SelectorList<Impl: SelectorImpl>(
    #[cfg_attr(feature = "to_shmem", shmem(field_bound))]
    ThinArcUnion<SpecificityAndFlags, Component<Impl>, (), Selector<Impl>>,
);

[kornelski]

test native_test ... error: Undefined Behavior: attempting a write access using <286855> at alloc121686[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 alloc121686[0x18], but that tag does not exist in the borrow stack for this location
    |                       this error occurs as part of an access at alloc121686[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() {
    |                 ^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

[kornelski]

servo/stylo#129

[fhanau]

🚢 Thank you for this, looks like we can get rid of the duplicate crates in downstream projects now.

[paolobarbolini]

Any chance of getting a new release out? I'd really like to get this improvement in our projects