1502534 - Crash pings don't have sessionIds for child process crashes

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement)

Description

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

The `payload.sessionId` field of a crash ping seems to be missing when the processType is not "main". It would be useful to report sessionIds for child process crashes if they're available.

The instant use case is that we want to exclude all crashes from the first telemetry session after a user enrolls in the WebRender experiment, since WebRender won't be deactivated for the "disabled" branch until the browser is restarted.

Comment 1

[William Lachance (:wlach)]

Was hoping this would have an obvious answer that I could dig up myself, but my brief spelunking in the crashreporter code didn't turn up anything.

It doesn't seem like the property is explicitly blacklisted as an annotation for this crash type, which makes me think that maybe the telemetry session id just isn't available inside the content process?

https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/toolkit/crashreporter/CrashAnnotations.yaml#698

:gsvelto, do you know the answer to :tdsmith's question?

Comment 2

[Gabriele Svelto [:gsvelto]]

I had a quick look and we do put the session ID in every crash ping:

https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/toolkit/components/crashes/CrashManager.jsm#663

However, if it's not among the crash annotations in the first place it won't be added to it. For main process crashes we add it manually within the exception handler:

https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/toolkit/crashreporter/nsExceptionHandler.cpp#1079

My guess is that we don't do it for content crashes. I'll have a look at what's needed to add it. Fortunately it doesn't matter if it's not accessible in the content process because the ping will be sent from the main process which has access to it.

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1502534 - Add the telemetry session ID to content crash pings

Comment 4

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

Awesome! Thanks for looking into this. Could we do this for GPU process crashes as well?

Comment 5

[Gabriele Svelto [:gsvelto]]

(In reply to Tim Smith 👨‍🔬 [:tdsmith] from comment #4)
> Awesome! Thanks for looking into this. Could we do this for GPU process
> crashes as well?

I haven't yet seen how GPU process crashes are handled, I'll have a look tomorrow. Chances are this change is sufficient for them too.

Comment 6

[Gabriele Svelto [:gsvelto]]

I just tested this on Windows and I can confirm that this patch also adds the telemetry session ID entry for GPU process crashes.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41cbf7beff10
Add the telemetry session ID to content crash pings r=ted

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/41cbf7beff10