Closed Bug 1213943 Opened 4 years ago Closed 4 years ago

Remove RELEASE_BUILD conditionality of SubjectPrincipal() MOZ_CRASH

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: bholley, Assigned: bholley)

Details

(Keywords: sec-other, Whiteboard: [adv-main45-][post-critsmash-triage])

Attachments

(1 file)

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.
Group: core-security → core-security-release
Scary. Have you gone through all the SubjectPrincipal() usage manually and figured out whether
we need to change the code to use something else?
(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).
Group: dom-core-security
Attachment #8691070 - Flags: review?(bzbarsky)
Comment on attachment 8691070 [details] [diff] [review]
Remove RELEASE_BUILD conditionality. v1

What is that telemetry probe showing so far?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
> 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....
Flags: needinfo?(bobbyholley)
(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).
Flags: needinfo?(bobbyholley)
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.
Attachment #8691070 - Flags: review?(bzbarsky) → review+
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.
Note sure why this didn't get closed in comment 9.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Group: dom-core-security
I see 10 samples out of 10 million on release. I think we're good here.
Whiteboard: [adv-main45-]
Whiteboard: [adv-main45-] → [adv-main45-][post-critsmash-triage]
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.