Closed
Bug 1213943
Opened 7 years ago
Closed 7 years ago
Remove RELEASE_BUILD conditionality of SubjectPrincipal() MOZ_CRASH
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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)
1.18 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•7 years ago
|
Group: core-security → core-security-release
Comment 1•7 years ago
|
||
Scary. Have you gone through all the SubjectPrincipal() usage manually and figured out whether we need to change the code to use something else?
Assignee | ||
Comment 2•7 years ago
|
||
(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).
Updated•7 years ago
|
Group: dom-core-security
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8691070 -
Flags: review?(bzbarsky)
![]() |
||
Comment 4•7 years ago
|
||
Comment on attachment 8691070 [details] [diff] [review] Remove RELEASE_BUILD conditionality. v1 What is that telemetry probe showing so far?
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 5•7 years ago
|
||
(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)
![]() |
||
Comment 6•7 years ago
|
||
> 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)
Assignee | ||
Comment 7•7 years ago
|
||
(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 8•7 years ago
|
||
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+
Comment 9•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9689a552174c
status-firefox45:
--- → fixed
Target Milestone: --- → mozilla45
Assignee | ||
Comment 10•7 years ago
|
||
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.
Assignee | ||
Comment 11•7 years ago
|
||
Note sure why this didn't get closed in comment 9.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Group: dom-core-security
Assignee | ||
Comment 12•7 years ago
|
||
I see 10 samples out of 10 million on release. I think we're good here.
Updated•6 years ago
|
Whiteboard: [adv-main45-]
Updated•6 years ago
|
Whiteboard: [adv-main45-] → [adv-main45-][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
Updated•3 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•