Last Comment Bug 707662 - spdy null deref
: spdy null deref
Status: RESOLVED FIXED
: crash
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 11 Branch
: All All
: -- critical (vote)
: mozilla11
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
: 707924 (view as bug list)
Depends on:
Blocks:
  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: ---
unaffected
unaffected
unaffected


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

Description 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 Patrick McManus [:mcmanus] 2011-12-05 07:04:42 PST
This requrires the manual pref-on, so there is no regression here.
Comment 2 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 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]:
-----------------------------------------------------------------

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 4 Patrick McManus [:mcmanus] 2011-12-06 07:40:46 PST
*** Bug 707924 has been marked as a duplicate of this bug. ***
Comment 5 Patrick McManus [:mcmanus] 2011-12-06 10:26:14 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5f66d5d8e4
Comment 6 Ed Morley [:emorley] 2011-12-07 02:47:57 PST
https://hg.mozilla.org/mozilla-central/rev/0a5f66d5d8e4
Comment 7 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 :-(

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc48c0992358
Comment 8 Ed Morley [:emorley] 2011-12-14 11:14:23 PST
Relanded on mozilla-central :-)

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

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