1404678 - stylo: thread '<unnamed>' panicked at 'byte index 2 is not a char boundary; it is inside '−' (bytes 1..4) of `n−1`', src/libcore/str/mod.rs:2161

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Bob Clary [:bc] (inactive)]

Attachment: Linux Debug Log

1. https://www.nafdigital.no/
   Windows, Linux; Beta/57, Nightly/58

2. thread '<unnamed>' panicked at 'byte index 2 is not a char boundary; it is inside '−' (bytes 1..4) of `n−1`', /checkout/src/libcore/str/mod.rs:2161
stack backtrace:
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
   1: std::panicking::default_hook::{{closure}}
   2: std::panicking::default_hook
   3: std::panicking::rust_panic_with_hook
   4: std::panicking::begin_panic
   5: std::panicking::begin_panic_fmt
   6: core::panicking::panic_fmt
   7: core::str::slice_error_fail
   8: cssparser::nth::parse_n_dash_digits
   9: cssparser::nth::parse_nth
  10: selectors::parser::parse_functional_pseudo_class
  11: selectors::parser::parse_one_simple_selector
  12: <selectors::parser::SelectorList<Impl>>::parse
  13: <style::stylesheets::rule_parser::NestedRuleParser<'a, 'b, R>>::parse_nested_rules
  14: <style::stylesheets::rule_parser::NestedRuleParser<'a, 'b, R> as cssparser::rules_and_declarations::AtRuleParser<'i>>::parse_block
  15: Servo_StyleSheet_FromUTF8Bytes

Comment 1

[Cameron McCormack (:heycam)]

The https://www.nafdigital.no/assets/build/css/1506686622/main.css sheet is served as text/css (with no charset), but it has a |@charset "UTF-8";| rule at the top.  Looking at the raw bytes of the file, inside the :nth-of-type() selector, at offset 0x396b there are three bytes -- e2 88 92 -- that if decoded as UTF-8 would give a U+2212 MINUS SIGN (where the author should've used a normal HYPHEN-MINUS).

I wonder if this is from bug 1354989.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Should be easy to fix. Taking.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: cssparser pull request

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/autoland/rev/ddd5f526cb1f297cf8fa5bdfff616c692a641ea5
Bug 1404678 - Update cssparser to 0.21.3 and revendor.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

This patch is probably not easy to uplift because of the removal of .cargo-ok due to newer version of cargo-vendor.

We may either want to uplift bug 1403407 with this patch, or have someone still have cargo-vendor 0.1.11 installed to generate a patch for uplift.

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ddd5f526cb1f

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch for uplift

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: may crash in certain cases (e.g. comment 0)
[Is this code covered by automated tests?]: it is covered by unit test of cssparser crate
[Has the fix been verified in Nightly?]: just landed yesterday
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: cssparser 0.12.3 includes two code changes, one is the change for this, and the other is a simple lint fix. The latter is apparently not risky as far as it builds. The change for this bug is not risky either as it just avoids an unnecessary boundary check.
[String changes made/needed]: n/a

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8914574 [details] [diff] [review]
patch for uplift

Stylo related crash fix, beta57+

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dd575194b4f1

Comment 10

[Andrei Vaida [:avaida]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> [Is this code covered by automated tests?]: it is covered by unit test of
> cssparser crate
> [Has the fix been verified in Nightly?]: just landed yesterday
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Xidorn's assessment on manual testing needs and the fact that this fix has automated coverage.