616591 - Firefox 4.0b8pre crash in [@ nsHttpPipeline::GetConnectionInfo(nsHttpConnectionInfo**) ][@ nsHttpPipeline::GetConnectionInfo ]

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Marcia Knous [:marcia]]

Seen while reviewing crash stats. Started showing up in crash stats using the 2010120300 build. Low volume crash which is so far Windows only. http://tinyurl.com/34hwbmq to the crashes so far.


Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	nsHttpPipeline::GetConnectionInfo 	netwerk/protocol/http/nsHttpPipeline.cpp:231
1 	xul.dll 	nsHttpTransaction::ParseHead 	netwerk/protocol/http/nsHttpTransaction.cpp:827
2 	mozcrt19.dll 	arena_dalloc 	obj-firefox/memory/jemalloc/crtsrc/jemalloc.c:4281
3 	xul.dll 	nsHttpPipeline::Close 	netwerk/protocol/http/nsHttpPipeline.cpp:527
4 	xul.dll 	nsHttpConnection::CloseTransaction 	netwerk/protocol/http/nsHttpConnection.cpp:648
5 	xul.dll 	nsHttpConnection::OnInputStreamReady 	netwerk/protocol/http/nsHttpConnection.cpp:1000
6 	xul.dll 	nsSocketInputStream::OnSocketReady 	netwerk/base/src/nsSocketTransport2.cpp:256
7 	xul.dll 	nsSocketTransport::OnSocketReady 	netwerk/base/src/nsSocketTransport2.cpp:1526
8 	xul.dll 	nsSocketTransportService::DoPollIteration 	netwerk/base/src/nsSocketTransportService2.cpp:687
9 	xul.dll 	nsSocketTransportService::OnProcessNextEvent 	netwerk/base/src/nsSocketTransportService2.cpp:551
10 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:590
11 	xul.dll 	NS_ProcessPendingEvents_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:200
12 	xul.dll 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:250
13 	xul.dll 	nsSocketTransportService::Run 	netwerk/base/src/nsSocketTransportService2.cpp:594
14 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:626
15 	xul.dll 	nsThreadStartupEvent::Run 	xpcom/threads/nsThread.cpp:207
16 	nspr4.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:426
17 	nspr4.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:122
18 	mozcrt19.dll 	_callthreadstartex 	obj-firefox/memory/jemalloc/crtsrc/threadex.c:348
19 	mozcrt19.dll 	__dllonexit 	obj-firefox/memory/jemalloc/crtsrc/onexit.c:276
20 	mozcrt19.dll 	_threadstartex 	obj-firefox/memory/jemalloc/crtsrc/threadex.c:326
21 	kernel32.dll 	GetCodePageFileInfo

Comment 1

[Patrick McManus [:mcmanus]]

probably want to look at 363109 which landed yesterday and changed nsHttpTransaction::ParseHead()

Comment 2

[Johnny Stenback (:jst)]

Given that this is a very recent regression I think we should track this down before we ship beta8.

Michal, Patrick seems to think this might be a regression from bug 363109, so handing this to you to start with.

Comment 3

[Johnny Stenback (:jst)]

Er, and actually reassigning, Michal, please see my previous comment.

Comment 4

[(no longer active)]

Attachment: Patch (v1)

nsHttpTransaction::ParseHead may be called from Close, where mConnection could be null.  We should make sure that it's not before attempting to get its connection info.

Comment 6

[(no longer active)]

Comment on attachment 495223 [details] [diff] [review]
Patch (v1)

Asking review from biesi too in case he can get to this sooner.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 495223 [details] [diff] [review]
Patch (v1)

r=me if you s/value id/value is/ in the preexisting comment and add a comment explaining that if !ci then we can't possibly have a response body, so there's no reason to look for junk in it.

Comment 8

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/2b63604957e2

Comment 9

[chris hofmann]

looks like this still is showing up in buildid 20101207033246 and 20101207030325

like http://crash-stats.mozilla.com/report/index/4847538a-db23-4fad-9d57-c0dd02101207

and

http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A4.0b8pre&query_search=signature&query_type=exact&query=nsHttpPipeline%3A%3AGetConnectionInfo%28nsHttpConnectionInfo**%29&do_query=1&admin=&signature=nsHttpPipeline%3A%3AGetConnectionInfo%28nsHttpConnectionInfo**%29

Comment 10

[Scoobidiver (away)]

> looks like this still is showing up in buildid 20101207033246 and
> 2010120703032
...with the same crash daily rate.

Comment 11

[Honza Bambas (:mayhemer)]

The crash has never been in nsHttpTransaction, but always in nsHttpPipeline.  The Ehsan's fix is good to have but will never fix this problem.

The true issue is here [1]:

We release mConnection from nsHttpPipeline before we call Close on the nsHttpTransaction that (the connection), after bug 363109 has landed, is accessed under some circumstances in nsHttpTransaction::Close.

[1] http://hg.mozilla.org/mozilla-central/annotate/0ff6d5984287/netwerk/protocol/http/nsHttpPipeline.cpp#l505

I have a real patch for this.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: Additional patch - v1

- release mConnection of nsHttpPipeline after we call Close on all transactions

Comment 13

[Honza Bambas (:mayhemer)]

For reference: release of the connection has been introduced at this place in bug 176919.

Comment 14

[Damon Sicore (:damons)]

Unless we're going to get this in the tree in the next few hours, do we really have to block beta 8?  We've gotta ship it immediately.  We're going too slow.

Comment 15

[Johnny Stenback (:jst)]

Boris, can you review (and assuming the changes looks good, land this) ASAP?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 496256 [details] [diff] [review]
Additional patch - v1

Please add a comment at the new location saying that releasing mConnection needs to come after we close all the HTTP transactions we have in the pipeline, ok?

Comment 17

[Boris Zbarsky [:bzbarsky]]

So... people who hit this bug presumably have HTTP pipelining enabled.  Interesting that we saw this at all.  ;)

Comment 18

[Marcia Knous [:marcia]]

A little late, but I was able to repro after what bz suggested in Comment 17 with one of the crash URLS.

1. Enable pipelining via about:config
2. Load http://www.accaglobal.com/
3. Reproducible crash.

https://crash-stats.mozilla.com/report/index/bp-e7bfb645-ff5d-4733-bb8d-7fcc82101208 is my report on Win 7.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/4fccdd4bc023 and http://hg.mozilla.org/mozilla-central/rev/b0a815aedb7e for the comment fix.

Honza, please write a test for this too?

Comment 20

[Damon Sicore (:damons)]

Thanks for killing this, guys!  Righteous.  :)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Er, make that http://hg.mozilla.org/mozilla-central/rev/81aea2200c34 for the comment fix (silly precommit hook!).

Comment 22

[Honza Bambas (:mayhemer)]

(In reply to comment #19)
> Pushed http://hg.mozilla.org/mozilla-central/rev/4fccdd4bc023 and
> http://hg.mozilla.org/mozilla-central/rev/b0a815aedb7e for the comment fix.
> 
> Honza, please write a test for this too?

Thanks for pushing it, Boris.  I'll try to have a test.

Comment 23

[Marcia Knous [:marcia]]

Verified fixed using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101209 Firefox/4.0b8pre. I no longer crash following the STR In Comment 18.