959172 - Crash Reports for mozilla::net::SpdySession31::CleanupStream

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Comment 1

[Patrick McManus [:mcmanus]]

https://crash-stats.mozilla.com/report/list?date=2014-01-12&range_unit=days&range_value=7&signature=mozilla%3A%3Anet%3A%3ASpdySession31%3A%3ACleanupStream%28mozilla%3A%3Anet%3A%3ASpdyStream31*%2C+tag_nsresult%2C+mozilla%3A%3Anet%3A%3ASpdySession31%3A%3ArstReason%29

an example

https://crash-stats.mozilla.com/report/index/af1e9fe5-62bb-4215-8160-2d3f62140111

~75 crashes across the last week

0 	xul.dll 	mozilla::net::SpdySession31::CleanupStream(mozilla::net::SpdyStream31 *,tag_nsresult,mozilla::net::SpdySession31::rstReason) 	netwerk/protocol/http/SpdySession31.cpp
1 	xul.dll 	mozilla::net::SpdySession31::WriteSegments(nsAHttpSegmentWriter *,unsigned int,unsigned int *) 	netwerk/protocol/http/SpdySession31.cpp
2 	xul.dll 	nsHttpConnection::OnSocketReadable() 	netwerk/protocol/http/nsHttpConnection.cpp
3 	xul.dll 	nsHttpConnection::OnInputStreamReady(nsIAsyncInputStream *) 	netwerk/protocol/http/nsHttpConnection.cpp
4 	xul.dll 	nsSocketInputStream::OnSocketReady(tag_nsresult) 	netwerk/base/src/nsSocketTransport2.cpp

Comment 2

[Patrick McManus [:mcmanus]]

I think we're seeing the error resulting in a double cleanup of the stream off the stream->writesegements() stack.

The pragmatic thing to do, especially for backports, is to make cleanupstream robust to being called in this circumstance.

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: double SpdySession::CleanupStream

Comment 4

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=547dd83d33e1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/135a5b7ba6d9

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/135a5b7ba6d9

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8359247 [details] [diff] [review]
double SpdySession::CleanupStream

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: continued exposure to crash bug.. about 75 crashes across all channels in the last 7 days
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): this is a null ptr check right before deref'ing the pointer. very low risk.
String or IDL/UUID changes made by this patch: none

Comment 7

[bhavana bajaj [:bajaj]]

Comment on attachment 8359247 [details] [diff] [review]
double SpdySession::CleanupStream

low risk crash fix, approving on branches.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/47b8a0b8c704
https://hg.mozilla.org/releases/mozilla-beta/rev/fd2b00b96291

Comment 9

[u279076]

FYI, I reported bug 960482 -- I'm not sure how this bug impacts that bug, if at all.

Comment 11

[u279076]

Adding topcrash keyword to this bug as per bug 960482. I'll follow up here next week with up to date data to evaluate this fix.

Comment 12

[u279076]

Additionally flagging as verifyme to confirm crashstats shows these crashes are gone across all branches next week.

Comment 13

[Ioana (away)]

Socorro shows no crashes for builds after 01/11 on any of the branches:

https://crash-stats.mozilla.com/report/list?date=2014-01-12&range_unit=days&range_value=28&signature=mozilla%3A%3Anet%3A%3ASpdySession31%3A%3ACleanupStream%28mozilla%3A%3Anet%3A%3ASpdyStream31%2A%2C+tag_nsresult%2C+mozilla%3A%3Anet%3A%3ASpdySession31%3A%3ArstReason%29