1834536 - add mozilla::dom::syncedcontext::Transaction<T>::Commit to prefix list

Status: RESOLVED FIXED

(Socorro :: Signature, task)

Description

[Andrew McCreight [:mccr8]]

The signature [@ mozilla::dom::syncedcontext::Transaction<T>::Commit ] is currently bundling together separate crashes.

These crashes can be distinguished by their crash reason:

CanSet failed for field(s): OpenerPolicy
CanSet failed for field(s): CurrentInnerWindowId
CanSet failed for field(s): EmbedderInnerWindowId

It would be good if the field name could be incorporated into the signature. I'm not sure what would be the best here. Maybe the signature could just be the crash reason, or maybe the list of field names could be added to the Commit stuff?

It looks like in general there can be a comma separated list of fields, though I don't see any. There's also another caller of FormatValidationError, but I don't see any instances of that, so I think we don't need to worry about that for now.

Comment 1

[Andrew McCreight [:mccr8]]

Nika, Andreas, any thoughts on what the signature should look like for these crashes? Thanks.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrew McCreight [:mccr8] from comment #1)

Nika, Andreas, any thoughts on what the signature should look like for these crashes? Thanks.

I feel like I've seen other signatures before which are identified by their crash reason, so that seems like the easiest approach to get this information in the signature.

The other instance in CommitFromIPC will produce a quite different signature, as it'll be an IPC-Fail signature, and ideally it should never come up, because we'll have caught the CanSet failure in the original process (leading to this assertion) - it exists mostly for sec. enforcement reasons, or if there's a CanSet condition which can only be checked in the parent process.

Comment 3

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

Can I see an example crash report with what you're hoping the signature should look like?

Comment 4

[Andrew McCreight [:mccr8]]

Oops, right. Here's an example crash: bp-128e0e56-aae7-4be1-ab97-3b87b0230611

I think the signature could just be the crash reason, like "CanSet failed for field(s): OpenerPolicy". These crash reasons all start with "CanSet failed for field(s):". It looks like this is already in the public information, in MOZ_CRASH Reason (Sanitized) so you could use that to avoid weird data leaks if we get other strings in the crash reason that are similar. (The string is dynamically generated, but will only contain a static set of strings.)

Comment 5

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

I looked at a bunch of crash reports with this signature. It sure seems like if we add mozilla::dom::syncedcontext::Transaction<T>::Commit to the prefix list, the next frame differentiates between the cases.

Then you get something like this:

app@socorro:/app$ ./socorro-cmd signature 521a8bc3-d9ff-45f3-9c29-100130230615 6ee01b21-eb71-4863-aecd-e48e90230614 ed1435c3-ed21-4ea5-be93-b6c170230614 
Crash id: 521a8bc3-d9ff-45f3-9c29-100130230615
Original: mozilla::dom::syncedcontext::Transaction<T>::Commit
New:      mozilla::dom::syncedcontext::Transaction<T>::Commit | mozilla::dom::BrowsingContext::SetOpenerPolicy
Same?:    False

Crash id: 6ee01b21-eb71-4863-aecd-e48e90230614
Original: mozilla::dom::syncedcontext::Transaction<T>::Commit
New:      mozilla::dom::syncedcontext::Transaction<T>::Commit | mozilla::dom::BrowsingContext::SetCurrentInnerWindowId
Same?:    False

Crash id: ed1435c3-ed21-4ea5-be93-b6c170230614
Original: mozilla::dom::syncedcontext::Transaction<T>::Commit
New:      mozilla::dom::syncedcontext::Transaction<T>::Commit | mozilla::dom::Document::SetHeaderData
Same?:    False

Does that suffice?

Comment 6

[Andrew McCreight [:mccr8]]

Good catch. Yeah, that sounds fine to me.

Comment 7

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

Attachment: pr 6423: bug 1834536: add mozilla::dom::syncedcontext::Transaction<T>::Commit to prefix list

Comment 8

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

willkg merged PR #6423: "bug 1834536: add mozilla::dom::syncedcontext::Transaction<T>::Commit to prefix list" in c737290.

Comment 9

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

I deployed this just now with bug #1839444. Marking as FIXED.

Comment 10

[Andrew McCreight [:mccr8]]

I went through and updated signatures on existing bugs, and checked that there were no bugs to file. Thanks for fixing this. It should be a lot less gnarly now.