1475820 - Rust crash signatures are broken: Please add some filtering on the panic symbols

Status: RESOLVED FIXED

(Socorro :: Signature, task, P1)

Description

[Darkspirit]

(Mike Hommey [:glandium] from bug 1474871 comment 21)
> (In reply to Jan Andre Ikenmeyer [:darkspirit] from bug 1474871 comment 19)
> > Thank you! Now I get bp-0ca773e7-2b08-4d1a-8fd7-67b4e0180713.
> > Could the crash signature be made as meaningful as in the past again? (e.g.
> > bp-76972dfe-f63c-4d5a-8d71-1a6a60180425)
> 
> File a Soccoro bug for some filtering on the panic symbols, but I'll note that the signature *does* contain the relevant bit (<T>::execute), although the specific rayon type is masked.

https://crash-stats.mozilla.com/topcrashers/?platform=Linux&product=Firefox&version=63.0a1&days=3&_range_type=build

bug 1452513:
> [@ mozalloc_abort | abort | panic_abort::__rust_start_panic::abort | __rust_start_panic | webrender::resource_cache::ResourceCache::update_resources ]
should be at least
> [@ mozalloc_abort | abort | webrender::resource_cache::ResourceCache::update_resources ]
again.

---

Offtopic, but maybe you could it do together?
The same crash has [@ static void webrender::resource_cache::ResourceCache::update_resources ] on Windows.
Could it be made that WebRender crash signatures look the same across all platforms?
For example in this format: 
[@ webrender::resource_cache::ResourceCache::request_image ] (bug 1390422).
Thanks.

Comment 1

[Will Kahn-Greene [:willkg] ET needinfo? me]

Grabbing this to look into today.

Comment 2

[Will Kahn-Greene [:willkg] ET needinfo? me]

The description of this bug is hard for me to parse, but I think what this bug is asking for is adding the following to the irrelevant list which gets ignored completely:

* __rust_start_panic
* panic_abort::__rust_start_panic::abort
* mozalloc_abort
* abort

Is that correct?

Comment 3

[Darkspirit]

Yes, if that looks reasonable to you. Thank you

Comment 4

[Will Kahn-Greene [:willkg] ET needinfo? me]

Let's talk about "panic_abort::" first since that's straight-forward. We have two items in the prefix list that start with "std::panicking" and we also have "core::panicking" and "std::panicking" in the irrelevant list.

I think we should nix the two "std::panicking" things from the prefix list since they're not doing anything. Then we can add "panic_abort::" to the irrelevant list. That gets us this:

"""
app@processor:/app$ socorro-cmd signature 60ebcaec-98db-48a9-a1f6-553950180717
Crash id: 60ebcaec-98db-48a9-a1f6-553950180717
Original: mozalloc_abort | abort | panic_abort::__rust_start_panic::abort | __rust_start_panic | webrender::renderer::Renderer::draw_tile_frame
New:      mozalloc_abort | abort | __rust_start_panic | webrender::renderer::Renderer::draw_tile_frame
Same?:    False
"""

"__rust_start_panic", "abort", and "mozalloc_abort" are all in the prefix list and have been for a while. When signature generation hits those frames, they will add that frame to the signature and continue to look at the next frame.

Given that those are in the prefix list already, I'm loathe to move them to the irrelevant list. Seems like mozalloc_abort shows up in a lot of crash reports, so changing that has consequences.

Is it important that these not show up in signatures at all?

Comment 5

[Darkspirit]

* I was just annoyed that crash signatures got that much longer with the regression and its fix (bug 1474871).
* WebRender crash signatures look different on across platforms because of their "prefix". MacOS often had something like ::h06673922f16d42c5 at the end (bug 1452513). If it would be consistent you had a nice overview[1] of affected operating systems and wouldn't have to add every two days the new MacOS signature to a certain bug.

[1] like this: https://crash-stats.mozilla.com/signature/?date=%3C2018-07-18T18%3A56%3A28%2B00%3A00&date=%3E%3D2018-07-11T18%3A56%3A28%2B00%3A00&product=Firefox&version=63.0a1&signature=mozilla%3A%3Adom%3A%3AContentChild%3A%3A%7EContentChild

Comment 6

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: pr 4518: bug 1475820: tweak rust panic symbols in siglists

Comment 7

[Will Kahn-Greene [:willkg] ET needinfo? me]

bp-e98691c3-27c4-435c-a36a-586150180511 has a signature of:

mozalloc_abort | abort | webrender::resource_cache::ResourceCache::update_resources::hb1727d001ad478db

I'll look into that ::hb172... part next.

Comment 8

[Will Kahn-Greene [:willkg] ET needinfo? me]

Actually, I don't need to. All the ::hb172... type crashes I could find are from a couple of months ago. In all cases, if we reprocess them in Socorro,t he ::hb172... part goes away in the signature.

For example, the crash report in comment #7 would now get this signature:

app@processor:/app$ socorro-cmd signature e98691c3-27c4-435c-a36a-586150180511
Crash id: e98691c3-27c4-435c-a36a-586150180511
Original: mozalloc_abort | abort | webrender::resource_cache::ResourceCache::update_resources::hb1727d001ad478db
New:      mozalloc_abort | abort | webrender::resource_cache::ResourceCache::update_resources
Same?:    False

I'm not sure what would have changed between May and now that would affect this.

Jan: Do you have any examples of crash reports with this problem from July?

Comment 9

[Darkspirit]

(In reply to Will Kahn-Greene [:willkg] ET needinfo? me from comment #8)
https://crash-stats.mozilla.com/search/?signature=%40%28.%2A%29webrender%28.%2A%29%3A%3Ah%28%5B0-9a-z%5D%7B1%2C%7D%29&product=Firefox&date=%3E%3D2018-06-05T17%3A00%3A00.000Z&date=%3C2018-07-19T17%3A00%3A20.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

Oh, sorry, it felt like it had been two weeks ago. It seems to have ended on 2018-06-06.

Comment 10

[Will Kahn-Greene [:willkg] ET needinfo? me]

Awesome!

I talked to Ted and he pointed to this:

https://doc.rust-lang.org/src/std/macros.rs.html#64-78

He mentioned making std::panicking::begin_panic and std::panicking::begin_panic_fmt sentinels. Signature generation will hit a sentinel and start the signature from that point. That way we don't get all the redundant "yes, I'm panicking" frames.

That gets us this:

app@processor:/app$ socorro-cmd signature 60ebcaec-98db-48a9-a1f6-553950180717
Crash id: 60ebcaec-98db-48a9-a1f6-553950180717
Original: mozalloc_abort | abort | panic_abort::__rust_start_panic::abort | __rust_start_panic | webrender::renderer::Renderer::draw_tile_frame
New:      webrender::renderer::Renderer::draw_tile_frame
Same?:    False

I'm going to do that.

Comment 11

[[github robot]]

Commits pushed to master at https://github.com/mozilla-services/socorro

https://github.com/mozilla-services/socorro/commit/133d4ab129421a2a2841c5c78d14f16d981f2aa9
bug 1475820: tweak rust panic symbols in siglists

* removes std::panicking::* from the prefix list because that's covered
  in the irrelevant list, so it doesn't have any effect
* add panic_abort:: to the irrelevant list so it gets skipped over

https://github.com/mozilla-services/socorro/commit/260734b4e737d0791fa12e7cdead7b15d7c2a051
fix bug 1475820: make begin_panic and begin_panic_fmt sentinels

When rust code panics, it goes through a panic! macro and the result of
that is there are a bunch of "oh, I'm panicking!" frames until we get to
the context of the actual panic.

We don't want the "oh, I'm panicking!" frames in signatures, so best to
start signature generation after those. This adds sentinels to fix that.

https://github.com/mozilla-services/socorro/commit/65b8c304cd264ab4b66f9a71e17f3e69ce6d63d0
Merge pull request #4518 from willkg/1475820-panic

bug 1475820: tweak rust panic symbols in siglists

Comment 12

[Will Kahn-Greene [:willkg] ET needinfo? me]

Those changes landed in master. We don't do deploys on fridays, so this will probably go out Monday.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Will Kahn-Greene [:willkg] ET needinfo? me from comment #8)
> Actually, I don't need to. All the ::hb172... type crashes I could find are
> from a couple of months ago.
[snip]
> 
> I'm not sure what would have changed between May and now that would affect
> this.

For the record this was bug 1440233.