707662 - spdy null deref

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

from socorro
ID: be35d598-4240-420c-9eb5-2ea142111205
Signature: nsHttpConnectionMgr::nsHalfOpenSocket::OnTransportStatus

0 	XUL 	nsHttpConnectionMgr::nsHalfOpenSocket::OnTransportStatus 	netwerk/protocol/http/nsHttpConnectionMgr.cpp:2111
1 	XUL 	nsSocketTransport::SendStatus 	netwerk/base/src/nsSocketTransport2.cpp:906
2 	XUL 	nsSocketTransport::OnSocketReady 	netwerk/base/src/nsSocketTransport2.cpp:1402
3 	XUL 	nsSocketTransportService::DoPollIteration 	netwerk/base/src/nsSocketTransportService2.cpp:759
4 	XUL 	nsSocketTransportService::Run 	netwerk/base/src/nsSocketTransportService2.cpp:642
5 	XUL 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:625
6 	XUL 	NS_ProcessNextEvent_P 	obj-firefox/x86_64/xpcom/build/nsThreadUtils.cpp:245
7 	XUL 	nsThread::ThreadFunc 	xpcom/threads/nsThread.cpp:273
8 	libnspr4.dylib 	_pt_root 	nsprpub/pr/src/pthreads/ptthread.c:187
9 	libsystem_c.dylib 	libsystem_c.dylib@0x4e8be 	
10 	libsystem_c.dylib 	libsystem_c.dylib@0x51b74 	
11 	libnspr4.dylib 	PR_JoinThread 	nsprpub/pr/src/pthreads/ptthread.c:577

mSocketTransport is null. There is an effective check for this a little later on (mSocketTransport == trans), so that should preceed the spdy hash key code.

Comment 1

[Patrick McManus [:mcmanus]]

This requrires the manual pref-on, so there is no regression here.

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Simple patch to move spdy server hash code to after the "is this the right transport?" check.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 579068 [details] [diff] [review]
patch 0

Review of attachment 579068 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

Interesting we may have mSocketTransport null while handling the notification.  We should remove nsHalfOpenSocket from callbacks of the transport on shutdown.

However, this is enough to fix this crash.  I had to catch this.

When changing this code, could you please also move status == nsISocketTransport::STATUS_CONNECTED_TO as the first condition in the list?  It could save some execution bits.

Comment 5

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5f66d5d8e4

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0a5f66d5d8e4

Comment 7

[Ed Morley [:emorley]]

Backed out (along with the rest of the SPDY landing) in order to stop us hitting the MSVC virtual address limit, so we can reopen the trees (bug 709193).

Sucks, but we don't really have any other choice here :-(

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc48c0992358

Comment 8

[Ed Morley [:emorley]]

Relanded on mozilla-central :-)

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