1378814 - stylo: devtools: tab crash after opening inspector tab on golem.de

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Darkspirit]

Nightly 56 x64 20170706100310 @ Debian Testing (Linux 4.9.0-3-amd64, Radeon RX480)
1. fresh profile with stylo enabled
2. visit https://www.golem.de
3. Press F12 an click on the Inspector tab
4. tab crash.

[@ mozalloc_abort | abort | core::option::expect_failed ]
bp-03718a9c-9d98-4832-b8a6-026220170706	06.07.17 17:14
bp-1976f236-5f55-4371-b42b-d55530170706	06.07.17 17:11
bp-fa1b539d-368f-412a-9469-a3af30170706	06.07.17 17:10

Comment 2

[Marcia Knous [:marcia]]

Removing the added crash signature since it seems that is another bug, Bug 1378789.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

The panic message is
> thread '<unnamed>' panicked at 'We're not probing, so we should always get a style back'

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1378814 - Don't return None in get_pseudo_style if not probing.

Review commit: https://reviewboard.mozilla.org/r/155094/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155094/

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8884172 [details]
Bug 1378814 - Don't return None in get_pseudo_style if not probing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155094/diff/1-2/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8884172 [details]
Bug 1378814 - Don't return None in get_pseudo_style if not probing.

https://reviewboard.mozilla.org/r/155094/#review160268

Arg, should've caught this on review. r=me

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8884172 [details]
Bug 1378814 - Don't return None in get_pseudo_style if not probing.

https://reviewboard.mozilla.org/r/155094/#review160270

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8884172 [details]
Bug 1378814 - Don't return None in get_pseudo_style if not probing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155094/diff/2-3/

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Servo PR: servo/servo#17636

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17abd6800fe9
Don't return None in get_pseudo_style if not probing. r=emilio

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/17abd6800fe9

Comment 14

[Darkspirit]

Nightly 56 x64 20170709100223 @ Debian Testing (Linux 4.9.0-3-amd64, Radeon RX480)
Verified fixed: No crash with opening F12>Inspector on golem.de anymore.

Comment 16

[Nicholas Nethercote [inactive]]

Questions for ted, copied from bug 1379101 comment 1:

- The "alloc::oom::default_oom_handler" makes it sound like an OOM, but it's an expect() failure. Any thoughts on how to make this cleearer?

- The crash report doesn't include the expect() argument. Would it be hard to include?

Thanks.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Nicholas Nethercote [:njn] from comment #16)
> Questions for ted, copied from bug 1379101 comment 1:
> 
> - The crash report doesn't include the expect() argument. Would it be hard
> to include?

expect() argument should be part of the panic message, so the general question is why panic message isn't always included in crash reports. I raised similar question in bug 1275780 comment 17.

We probably should file a new bug for this.

Comment 18

[(not currently active) Ted Mielczarek]

(In reply to Nicholas Nethercote [:njn] from comment #16)
> Questions for ted, copied from bug 1379101 comment 1:
> 
> - The "alloc::oom::default_oom_handler" makes it sound like an OOM, but it's
> an expect() failure. Any thoughts on how to make this cleearer?

In bug 1302078 comment 38 dmajor stated that he thought this was just identical code folding. We could probably add some Socorro signature fiddling to replace both `std::panicking::rust_panic` and `alloc::oom::default_oom_handler` with a special `rust_panic` signature.