1880257 - Annotate crash reports when Marionette or the Remote Agent are active

Status: RESOLVED FIXED

(Remote Protocol :: Agent, task, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Follow-up from bug 1875773.

(In reply to Gabriele Svelto [:gsvelto] from bug 1875773 comment #8)

I think that's a very good idea. We already have something similar for how the crash was submitted (see the SubmittedFrom crash annotation). You can either add to that if automation submits the crash reports on its own or add a new annotation specifically to say that it was generated in automation (but submitted using one of the existing mechanisms).

With geckodriver we do not allow automatic crash report submission. That is to prevent crash-stats from receiving too many automation related crash reports which as well are not annotated right now and would be hard to identify. By adding the annotation it would indeed become easier.

Gabriele, could you maybe give some details where the annotation needs to be done? I checked some of the crash reporter code but wasn't able to find it. Thanks!

Comment 1

[Gabriele Svelto [:gsvelto]]

First you need to add it to CrashAnnotations.yaml and request a data review for it. Then you need to add it to the JSON in the .extra file before submitting it to Socorro. How is automation submitting crashes? Is it using Firefox itself or are the automation scripts sending the crash report directly to Socorro?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Gabriele Svelto [:gsvelto] from comment #1)

Then you need to add it to the JSON in the .extra file before submitting it to Socorro.

Where is this code for the .extra file located? I assume it should be pretty straight forward once the data review has been approved.

How is automation submitting crashes? Is it using Firefox itself or are the automation scripts sending the crash report directly to Socorro?

Our automation will not send reports automatically. We only allow to enable the crash reporter for diagnostics and manual submission of reports if the minidump file cannot be retrieved. So the frequency of reports will be very low.

Comment 3

[Gabriele Svelto [:gsvelto]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1][away 02/20 - 02/26] from comment #2)

Where is this code for the .extra file located? I assume it should be pretty straight forward once the data review has been approved.

The .extra file is emitted together with the minidump (.dmp) when we generate a crash report, it contains all the annotations in the crashed process and in the main process (if the crashed process was a child). So if this annotation is added at any time during Firefox' run it will be included. You could add a parameter that causes Firefox to call CrashReporter::AnnotateCrashReport() with the new annotation on startup; this would put it in all crashes. Note this can also be called from JavaScript.

The code that writes the file itself is here for child process crashes and here for parent ones, but I don't recommend modifying it. Better stick with the public interface.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks! nsICrashReporter.annotateCrashReport() seems to be indeed the right choice here. It actually looks like that it can also be set via Services.appinfo as string values:

Services.appinfo.annotateCrashReport("%key", "%value");

We may have our own annotateCrashReport function and return early if Services.appinfo.crashReporterEnabled is false.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I'ld like to wait for bug 1882338 done first so it can be properly tested.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I had a look at this bug today and I got it working for the parent process. The annotation for Marionette and Remote Agent are included in the .extra file. But when I upload the crash report I cannot find it anywhere. Do I have to request them to show up?

Also for content processes it doesn't work. But I assume that we have to set the annotation each time a new content process is created? If that is the case, why can't the crash reporter itself not do that? Are there cases when annotations should only be present on the parent but not the child processes?

Comment 7

[Gabriele Svelto [:gsvelto]]

New crash annotations are hidden by default on Socorro. It's a security measure in case they might contain private data. If you have restricted data access you can see them anyway in the "Annotations" tab. If the annotation can be surfaced safely you can request it to be indexed and visible to all users. See bug 1723691 as a simple example on how to do that.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Gabriele. That is good to know. Given that I don't have such permissions I'm unable to verify that. Do you have access to them and if yes could check if Marionette and RemoteAgent are both listed for the following crash report?

https://crash-stats.mozilla.org/report/index/b0483ff1-ff78-49ec-9b74-fec480241230#tab-annotations

Also regarding crashes of web content processes is it required to setup the notification each time? I assume a JSProcessActor would be the right thing to do in such a case?

Comment 9

[Gabriele Svelto [:gsvelto]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #8)

Thanks Gabriele. That is good to know. Given that I don't have such permissions I'm unable to verify that. Do you have access to them and if yes could check if Marionette and RemoteAgent are both listed for the following crash report?

https://crash-stats.mozilla.org/report/index/b0483ff1-ff78-49ec-9b74-fec480241230#tab-annotations

Yes, both are present and set to 1.

Also regarding crashes of web content processes is it required to setup the notification each time? I assume a JSProcessActor would be the right thing to do in such a case?

Which notification?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Gabriele Svelto [:gsvelto] from comment #9)

https://crash-stats.mozilla.org/report/index/b0483ff1-ff78-49ec-9b74-fec480241230#tab-annotations

Yes, both are present and set to 1.

Great. Thanks for the confirmation!

Also regarding crashes of web content processes is it required to setup the notification each time? I assume a JSProcessActor would be the right thing to do in such a case?

Which notification?

Sorry, I actually wanted to ask about the annotation.

Comment 11

[Gabriele Svelto [:gsvelto]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #10)

Sorry, I actually wanted to ask about the annotation.

If you've set the annotation in the main process it will be included in all crash reports, including those for child processes. If you set it in a child process then it will be included only in the report generated when crashing that specific child process.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1880257 - Add crash annotations for Marionette and RemoteAgent enabled state.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Gabriele Svelto [:gsvelto] from comment #11)

If you've set the annotation in the main process it will be included in all crash reports, including those for child processes. If you set it in a child process then it will be included only in the report generated when crashing that specific child process.

That's definitely not working for me when testing the scenario with about:crashcontent. Whether Marionette not RemoteAgent annotations are set in the .extra file of the crash report. Do I miss something?

Also do we always have to set the annotation and not only when the components are actually enabled? Not sure if it will be fine as what I did in the attached patch, or if I have to set the this.enabled state.

Thanks!

Comment 14

[Gabriele Svelto [:gsvelto]]

Oh, I see what the problem is. The annotation is declared as a boolean and passed into nsICrashReporter.annotateCrashReport() as a boolean, but the implementation of that method is dumb and turns it into a string. This even triggers an assertion in debug builds because we expect that particular annotation to be a boolean and not a string. Let me see how I can fix this.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I was wondering about that as well and as such put the try/catch block around the call to annotateCrashReport(). So I assume this try/catch is not needed once you fixed it. Shall I maybe for now set a string or are you planning to fix that dependency short term? Also does this type issue actually prevent the parent process to set the annotation for content processes and it will be fixed as well with your patch?

Comment 16

[Gabriele Svelto [:gsvelto]]

I'm testing the patch as we speak, hopefully I'll be able to land it within 24h.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Gabriele Svelto [:gsvelto] from comment #16)

I'm testing the patch as we speak, hopefully I'll be able to land it within 24h.

Now that this patch landed I tried to update my patch and I noticed that the values that we are writing to the .extra file are still all strings. I assume that we have to keep that for compatibility and not to break external clients.

Comment 18

[Gabriele Svelto [:gsvelto]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #17)

Now that this patch landed I tried to update my patch and I noticed that the values that we are writing to the .extra file are still all strings. I assume that we have to keep that for compatibility and not to break external clients.

Yes, when serialized they all end up as strings because we need to be backwards compatible with our tooling. The typing only ensures that they're the correct type when adding them within Gecko.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1880257 - [wdspec] Improve wdspec crash tests for crash annotations.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f4f5df5486e
Add crash annotations for Marionette and RemoteAgent enabled state. r=webdriver-reviewers,gsvelto,Sasha
https://hg.mozilla.org/integration/autoland/rev/ba86455245fb
[wdspec] Improve wdspec crash tests for crash annotations. r=webdriver-reviewers,Sasha

Comment 21

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/5f4f5df5486e
https://hg.mozilla.org/mozilla-central/rev/ba86455245fb