1711189 - Crash in [@ core::option::expect_none_failed | geckoservo::glue::Servo_DeclarationBlock_GetCssText]

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez [:emilio]]

Crash report: https://crash-stats.mozilla.org/report/index/9358fffe-d5a1-4524-98bd-e05e50210514

MOZ_CRASH Reason: called `Result::unwrap()` on an `Err` value: Error

Top 10 frames of crashing thread:

0 xul.dll RustMozCrash mozglue/static/rust/wrappers.cpp:16
1 xul.dll mozglue_static::panic_hook mozglue/static/rust/lib.rs:89
2 xul.dll core::ops::function::Fn::call<fn ../88f19c6dab716c6281af7602e30f413e809c5974/library/core/src/ops/function.rs:227
3 xul.dll std::panicking::rust_panic_with_hook ../88f19c6dab716c6281af7602e30f413e809c5974//library/std/src/panicking.rs:595
4 xul.dll std::panicking::begin_panic_handler::{{closure}} ../88f19c6dab716c6281af7602e30f413e809c5974//library/std/src/panicking.rs:497
5 xul.dll std::sys_common::backtrace::__rust_end_short_backtrace<closure-0, !> ../88f19c6dab716c6281af7602e30f413e809c5974//library/std/src/sys_common/backtrace.rs:141
6 xul.dll std::panicking::begin_panic_handler ../88f19c6dab716c6281af7602e30f413e809c5974//library/std/src/panicking.rs:493
7 xul.dll core::panicking::panic_fmt ../88f19c6dab716c6281af7602e30f413e809c5974//library/core/src/panicking.rs:92
8 xul.dll core::option::expect_none_failed ../88f19c6dab716c6281af7602e30f413e809c5974//library/core/src/option.rs:1329
9 xul.dll geckoservo::glue::Servo_DeclarationBlock_GetCssText servo/ports/geckolib/glue.rs:4357

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1711189 - Avoid trying to serialize 'all' with stuff that isn't variable references or CSS wide keywords.

All is special, and that'd cause an error later on.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1711189 - Avoid returning fmt::Errors for things that are not formatting errors.

They are not. The serialization code already checks if the result is
empty, which can happen for other reasons. This makes the code a bit
more resilient to misuse.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

I'll land this unreviewed to stop the crash, since they're really simple, but Daniel / Boris, can you sanity-check when you get a chance? See the test-case in the first patch for what's going on.

Before, the bug was wallpapered because those properties were part of the all shorthand but weren't returned by getComputedStyle. Either of the two patches fix the bug. The former avoids doing a bit of extra work.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Err, see the comment above.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac590dee77f5
Avoid trying to serialize 'all' with stuff that isn't variable references or CSS wide keywords.
https://hg.mozilla.org/integration/autoland/rev/72f541c3031f
Avoid returning fmt::Errors for things that are not formatting errors.

Comment 6

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28996 for changes under testing/web-platform/tests

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/ac590dee77f5
https://hg.mozilla.org/mozilla-central/rev/72f541c3031f

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

I'll land this unreviewed to stop the crash, since they're really simple, but Daniel / Boris, can you sanity-check when you get a chance? See the test-case in the first patch for what's going on.

Boris probably knows this css-internals code better than I do, so he may see something that I don't. But this seems fine to me. Thanks for jumping on this!

(I did notice one extremely-nitpicky typo -- "CSS wide" is supposed to be hyphenated -- which I left as an inline nit on part 1. If you feel like fixing that in a followup here, that might be nice, though also not a big deal.)

Comment 10

[Boris Chiou [:boris]]

(In reply to Daniel Holbert [:dholbert] from comment #9)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

I'll land this unreviewed to stop the crash, since they're really simple, but Daniel / Boris, can you sanity-check when you get a chance? See the test-case in the first patch for what's going on.

Boris probably knows this css-internals code better than I do, so he may see something that I don't. But this seems fine to me. Thanks for jumping on this!

Looks fine with me as well. Thanks for the quick look. (I'm fine with both patches.)