Closed Bug 1286664 Opened 3 years ago Closed 3 years ago

mozilla::net::TLSFilterTransaction::WriteSegments crash

Categories

(Core :: Networking: HTTP, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed
firefox-esr45 --- affected
firefox50 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

(Keywords: csectype-nullptr, Whiteboard: [spdy][necko-active][sg:dos])

Crash Data

Attachments

(1 file)

Connection() is null - probably due to a Close() of the sesison

this is a lesser problem correlated with bug 1283823, zenmate, and https/h2 based proxying.

It effects back to 38, but an increase in h2 based proxying has made it run up the crash charts.

https://bugzilla.mozilla.org/show_bug.cgi?id=1283823#c25 indicates it is ~1.5 of the crashes in 48b7
Whiteboard: [spdy] → [spdy][necko-active]
Crash Signature: [@ mozilla::net::TLSFilterTransaction::WriteSegments ]
Attachment #8770963 - Flags: review?(hurley) → review+
https://hg.mozilla.org/mozilla-central/rev/aeec04b71527
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment on attachment 8770963 [details] [diff] [review]
TLSFilterTransaction::WriteSegments null Connection()

Same approval? comments as from 
https://bugzilla.mozilla.org/show_bug.cgi?id=1283823#c14
as this covers an additional case from that bug.

Approval Request Comment
[Feature/regressing bug #]: https proxy - 378637 - gecko 38
[User impact if declined]: crashes with some https proxies - zenmate is gaining market traction which is why this crash rate has gone up
[Describe test coverage new/current, TreeHerder]: some treeherder, fix manually verified by reporter
[Risks and why]: medium low - very targeted patch with well understood stack trace
[String/UUID change made/needed]: none


[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: likely a high crash rate just like beta 48 1.5% crashes due to changes in the way firefox is used with vpns/https proxies
Fix Landed on Version: 50
Risk to taking this patch (and alternatives if risky): medium low.. could disable feature as alternative but that would regress some installs
String or UUID changes made by this patch: none
Attachment #8770963 - Flags: approval-mozilla-esr45?
Attachment #8770963 - Flags: approval-mozilla-beta?
Attachment #8770963 - Flags: approval-mozilla-aurora?
Comment on attachment 8770963 [details] [diff] [review]
TLSFilterTransaction::WriteSegments null Connection()

Too late in the beta cycle to take chances.
For ESR, it doesn't match the esr criteria (only sec-high & sec-critical issues) and we don't have enough crash reports to care about.
Attachment #8770963 - Flags: approval-mozilla-esr45?
Attachment #8770963 - Flags: approval-mozilla-esr45-
Attachment #8770963 - Flags: approval-mozilla-beta?
Attachment #8770963 - Flags: approval-mozilla-beta-
Attachment #8770963 - Flags: approval-mozilla-aurora?
Attachment #8770963 - Flags: approval-mozilla-aurora+
hi, could i ask you to reconsider the denied uplift approval for beta (maybe after it sits for 2-3 days on aurora)? 

the uplift approval message seems to have been copied from bug 1283823, but the patch here is just a simple one-line null-check on top of the already uplifted and otherwise working patch, so i'm not sure if the risk assessment ("medium low") applies here as well. 
the benefits may outweigh the (small) risk as the mentioned zenmate addon (https://addons.mozilla.org/firefox/addon/zenmate-security-privacy-vpn/) has over 300.000 users who could be affected by this crash (its volume is at 1.4% on beta & 0.4% on release currently).
Flags: needinfo?(sledru)
Patrick, can you confirm the risk evaluation? Thanks
Flags: needinfo?(sledru) → needinfo?(mcmanus)
:philipp has it right. This is just a null check - its biggest risk is that it doesn't fix the whole problem (as with 1283823 - which only solved ~75% of the isuse) or shifts it to somewhere less desirable. But I'm very comfortable with its safety.
Flags: needinfo?(mcmanus)
Comment on attachment 8770963 [details] [diff] [review]
TLSFilterTransaction::WriteSegments null Connection()

OK, let's take it then. Thanks
Attachment #8770963 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
Whiteboard: [spdy][necko-active] → [spdy][necko-active][sg:dos]
You need to log in before you can comment on or make changes to this bug.