Last Comment Bug 707662 - spdy null deref
: spdy null deref
: crash
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 11 Branch
: All All
-- critical (vote)
: mozilla11
Assigned To: Patrick McManus [:mcmanus]
: Patrick McManus [:mcmanus]
: 707924 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2011-12-05 07:01 PST by Patrick McManus [:mcmanus]
Modified: 2012-02-01 13:56 PST (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch 0 (2.34 KB, patch)
2011-12-05 07:51 PST, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review

Description User image Patrick McManus [:mcmanus] 2011-12-05 07:01:09 PST
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 User image Patrick McManus [:mcmanus] 2011-12-05 07:04:42 PST
This requrires the manual pref-on, so there is no regression here.
Comment 2 User image Patrick McManus [:mcmanus] 2011-12-05 07:51:29 PST
Created attachment 579068 [details] [diff] [review]
patch 0

Simple patch to move spdy server hash code to after the "is this the right transport?" check.
Comment 3 User image Honza Bambas (:mayhemer) 2011-12-06 03:07:29 PST
Comment on attachment 579068 [details] [diff] [review]
patch 0

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


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 4 User image Patrick McManus [:mcmanus] 2011-12-06 07:40:46 PST
*** Bug 707924 has been marked as a duplicate of this bug. ***
Comment 5 User image Patrick McManus [:mcmanus] 2011-12-06 10:26:14 PST
Comment 6 User image Ed Morley [:emorley] 2011-12-07 02:47:57 PST
Comment 7 User image Ed Morley [:emorley] 2011-12-10 15:34:21 PST
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 :-(
Comment 8 User image Ed Morley [:emorley] 2011-12-14 11:14:23 PST
Relanded on mozilla-central :-)

Note You need to log in before you can comment on or make changes to this bug.