1286664 - mozilla::net::TLSFilterTransaction::WriteSegments crash

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

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

Comment 1

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08a40c5a18bc

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: TLSFilterTransaction::WriteSegments null Connection()

Comment 3

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aeec04b71527b443c169099cc125288746cc0926
Bug 1286664 - TLSFilterTransaction::WriteSegments null Connection() r=hurley

Comment 4

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

https://hg.mozilla.org/mozilla-central/rev/aeec04b71527

Comment 5

[Patrick McManus [:mcmanus]]

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

Comment 6

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e59e20a2479e

Comment 8

[[:philipp]]

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).

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Patrick, can you confirm the risk evaluation? Thanks

Comment 10

[Patrick McManus [: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.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8770963 [details] [diff] [review]
TLSFilterTransaction::WriteSegments null Connection()

OK, let's take it then. Thanks

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f4257230dff