1693863 - fenix crash reports with JavaException and minidump get bad signatures

Status: NEW

(Socorro :: Signature, defect, P2)

Description

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

This Fenix crash report:

https://crash-stats.mozilla.org/report/index/41245ca2-fc75-4bf9-bf26-716f70210219

has signature:

java.lang.SecurityException: at android.os.Parcel.readException(Parcel.java)

It has both a JavaException annotation as well as a minidump.

I think signature generation generates a signature from the minidump stack, but then stomps on that with signature for the Java error.

I think we'd rather the Java error didn't stomp on the signature.

Comment 1

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

Grabbing this to look into next week.

How often does this happen?

Is the minidump-stack-based signature better than the Java-based signature? (Probably.)

Comment 2

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

If I fix the signature generation logic to prefer C++/Rust signature if there's information, then we get this:

app@socorro:/app$ socorro-cmd signature 41245ca2-fc75-4bf9-bf26-716f70210219
Crash id: 41245ca2-fc75-4bf9-bf26-716f70210219
Original: java.lang.SecurityException: at android.os.Parcel.readException(Parcel.java)
New:      mozilla::java::Clipboard::HasData
Same?:    False

In the last week, we got 2,304 crash reports for Fenix with "SecurityException" in the signature. I spot-checked 100 of them and about 25% of them have both a JavaStackTrace as well as a minidump parsed by minidump-stackwalk.

I think this is worth fixing. I'll let #stability know, too.

Comment 3

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

Here's a supersearch that shows the crash reports that will get changed:

https://crash-stats.mozilla.org/search/?signature=~java.lang&moz_crash_reason=%21__null__&product=Fenix&date=%3E%3D2021-03-03T17%3A51%3A00.000Z&date=%3C2021-03-10T17%3A51%3A00.000Z&_facets=signature&_sort=-date&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

That's 7,866 out of 471,846 in the last 7 days that are affected.

On #stability, Kevin wondered if we'd want an indication that the crash report has both JavaStackTrace and a minidump. I don't know if that helps.

What would we add as an indicator? Would it be a "this crash report has a JavaStackTrace" indicator or a "this crash report has both a minidump and a JavaStackTrace" indicator?

Who else should chime in here?

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

I don't have a strong opnion either way, I can definitely think of a few cases where this change might make things harder a little bit, but from the examples you brought up it looks like it would be a good change. I think we should do it (assuming it's not too much of a pain to roll back if we find a lot of bad examples).

Comment 5

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

Regarding rollback, we can definitely undo changes and we can reprocess existing crash reports to pick up signature generation changes. So changes aren't set in stone.

What if we prepended the Java part? With the above example, we'd have:

Original: java.lang.SecurityException: at android.os.Parcel.readException(Parcel.java)
New:      java.lang.SecurityException: at android.os.Parcel.readException(Parcel.java) | mozilla::java::Clipboard::HasData

Seems like that's best because it adds more clarity to the existing signature. Does that look good?

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

The pipe separated list seems like something worth trying.

Comment 7

[[ex-Mozilla] Agi Sferro | :agi]

Yeah that would be ideal for us!

Comment 8

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

On second thought, I don't understand what I'm looking at here or what these situations mean in the grand scheme of things. I should roll this up into the bigger "rethink Java crash signature generation" bug.

Comment 9

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

Bumping bugs off my queue because I'm not going to get to them any time soon.