1350917 - Killing SIGABRT from outside does not create crash report on Mac OS

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Not sure this is intentional or not.
It would be handy if we can force a hung process crash and send a crash report.

Comment 1

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

:mconley might be interested for this.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

This _used_ to work, but appears to have stopped. Attempting to get a regression range.

I've gotten this far:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d1a54ae63da7ebc4bc1eeb5b613e8ec29bfcb80a&tochange=ac39fba33c6daf95b2cda71e588ca18e2eb752ab

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I suspect bug 1069556 regressed this behaviour. Doing a build to prove, stand by...

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Yeah, the ability to intentionally trigger crash report creation with SIGABRT went away with the patches in bug 1069556.

Hey ted, do you know if that was intentional? Digging through the patch that sync'd us with breakpad upstream, I see this line that seems to be related: https://hg.mozilla.org/mozilla-central/rev/62a0d8c93da2#l92.275, but I really don't know what I'm reading. :/

Comment 5

[(not currently active) Ted Mielczarek]

This was not intentional, no. It's possible we either had a local patch that never got upstreamed, or we took an upstream change that broke this.

(In reply to Mike Conley (:mconley) from comment #4)
> Hey ted, do you know if that was intentional? Digging through the patch that
> sync'd us with breakpad upstream, I see this line that seems to be related:
> https://hg.mozilla.org/mozilla-central/rev/62a0d8c93da2#l92.275, but I
> really don't know what I'm reading. :/

FYI, that diff hunk is in src/client/linux/handler/exception_handler.cc, so it's not related. :)

On the plus side, if we figure out what broke this, we can easily fix it because we're not using upstream Breakpad for these bits anymore.

Comment 6

[(not currently active) Ted Mielczarek]

For clarity, I tested this out locally. Killing the firefox process with `kill -ABRT` still works fine:
https://crash-stats.mozilla.com/report/index/df1388db-aa47-4bc6-8c31-febd12170412

Killing plugin-container does not, I get the "tab crashed" page but not a crash report.

Oh, turns out we never upstreamed bug 1012912 for dumb reasons, so we lost that when we synced again. We just need to re-apply https://hg.mozilla.org/mozilla-central/rev/bbeb113185da on top of our current code. The patch won't apply directly because we moved the files from toolkit/crashreporter/google-breakpad/src/client to toolkit/crashreporter/breakpad-client.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Seems straight forward - lemme toss together a patch.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1350917 - Re-apply fix from bug 1012912 onto breakpad-client, since it got lost during the last sync.

Review commit: https://reviewboard.mozilla.org/r/129594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129594/

Comment 9

[Mike Conley (:mconley) (:⚙️)]

I locally confirmed that this patch fixes the issue.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8857626 [details]
Bug 1350917 - Re-apply fix from bug 1012912 onto breakpad-client, since it got lost during the last sync.

https://reviewboard.mozilla.org/r/129594/#review132194

Thanks for digging into this and fixing it!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8fbfe8c77eaa
Re-apply fix from bug 1012912 onto breakpad-client, since it got lost during the last sync. r=ted

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8fbfe8c77eaa

Comment 13

[Stephen A Pohl [:spohl]]

We're trying to uplift bug 1405151 to ESR52, which depends on the work here. Could you request uplift approval for ESR52?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8857626 [details]
Bug 1350917 - Re-apply fix from bug 1012912 onto breakpad-client, since it got lost during the last sync.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

This is something that bug 1405151 depends on, and bug 1405151 seems to be a critical stability fix.

User impact if declined: 

Users on ESR52 won't be able to benefit from greater stability from the bug 1405151 fix.

Fix Landed on Version:

Firefox 55.

Risk to taking this patch (and alternatives if risky): 

Very little risk.

String or UUID changes made by this patch: 

None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8857626 [details]
Bug 1350917 - Re-apply fix from bug 1012912 onto breakpad-client, since it got lost during the last sync.

Crash fix (plus we should get better crash data from ESR) with this and the patch from 1405151.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/c3ec222e1719