1213943 - Remove RELEASE_BUILD conditionality of SubjectPrincipal() MOZ_CRASH

Status: RESOLVED FIXED

Description

[Bobby Holley (:bholley)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1072150#c54

During the 45 cycle, I plan to remove the RELEASE_BUILD conditionality of the MOZ_CRASH. Once 44 hits release (and 45 is on beta), I'll check telemetry to make sure we're not seeing a bunch of these cases on release. If we are, we can back the MOZ_CRASH out on beta.

Comment 1

[Olli Pettay [:smaug] (high review load)]

Scary. Have you gone through all the SubjectPrincipal() usage manually and figured out whether
we need to change the code to use something else?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #1)
> Scary. Have you gone through all the SubjectPrincipal() usage manually and
> figured out whether
> we need to change the code to use something else?

No, that's pretty intractable, since that analysis depends on whether there is a callstack that reaches the callsite from native code. I'm open to suggestions on how to find culprits beyond the current technique of dynamic analysis (which risks crashes, but not security bugs).

Comment 3

[Bobby Holley (:bholley)]

Created attachment 8691070 [details] [diff] [review]
Remove RELEASE_BUILD conditionality. v1

Comment 4

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8691070 [details] [diff] [review]
Remove RELEASE_BUILD conditionality. v1

What is that telemetry probe showing so far?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Comment on attachment 8691070 [details] [diff] [review]
> Remove RELEASE_BUILD conditionality. v1
> 
> What is that telemetry probe showing so far?

We don't need the telemetry probe here, because this code is currently on aurora and nightly, which means that the MOZ_CRASH will take effect. Last I checked, the only remaining crash site was for bug 1222280 (at extremely low volume), which is now landed on central and will soon be uplifted to aurora.

The more interesting question is what happens when we get to beta, but we'll have to wait another cycle for that telemetry to tell us anything.

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> The more interesting question is what happens when we get to beta, but we'll have to wait
> another cycle for that telemetry to tell us anything.

OK.  Is it worth waiting that cycle to see whether this gets hit in the wild?  Seems to me like it is....

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> > The more interesting question is what happens when we get to beta, but we'll have to wait
> > another cycle for that telemetry to tell us anything.
> 
> OK.  Is it worth waiting that cycle to see whether this gets hit in the
> wild?  Seems to me like it is....

I don't understand why it would be - the Telemetry is already going out one cycle ahead of the crash (and I am watching it), so we have all the room we need to disable the release-mode crash if we see the Telemetry firing (either an aurora uplift for Telemetry activity on beta, or a beta uplift for Telemetry activity on release).

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8691070 [details] [diff] [review]
Remove RELEASE_BUILD conditionality. v1

Ah, I see.

OK, r=me assuming we plan to adjust course as needed based on the telemetry.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9689a552174c

Comment 10

[Bobby Holley (:bholley)]

According to Telemetry, we hit this case three times out of 23.55 million on beta. So I'm optimistic about going forward.

I found one new low-volume crash on crash-stats which I'll file now.

Comment 11

[Bobby Holley (:bholley)]

Note sure why this didn't get closed in comment 9.

Comment 12

[Bobby Holley (:bholley)]

I see 10 samples out of 10 million on release. I think we're good here.