Closed Bug 616591 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Networking, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: marcia, Assigned: mayhemer)

References

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(2 files)

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
probably want to look at 363109 which landed yesterday and changed nsHttpTransaction::ParseHead()
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.
blocking2.0: --- → beta8+
Keywords: regression
Er, and actually reassigning, Michal, please see my previous comment.
Assignee: nobody → michal.novotny
Attached patch Patch (v1)Splinter Review
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.
Assignee: michal.novotny → ehsan
Status: NEW → ASSIGNED
Attachment #495223 - Flags: review?(bzbarsky)
Duplicate of this bug: 616676
Summary: Firefox 4.0b8pre crash in [@ nsHttpPipeline::GetConnectionInfo(nsHttpConnectionInfo** → Firefox 4.0b8pre crash in [@ nsHttpPipeline::GetConnectionInfo(nsHttpConnectionInfo**) ]
Comment on attachment 495223 [details] [diff] [review]
Patch (v1)

Asking review from biesi too in case he can get to this sooner.
Attachment #495223 - Flags: review?(cbiesinger)
Whiteboard: [has patch][needs review bz/biesi]
OS: Windows XP → All
Hardware: x86 → All
Summary: Firefox 4.0b8pre crash in [@ nsHttpPipeline::GetConnectionInfo(nsHttpConnectionInfo**) ] → Firefox 4.0b8pre crash in [@ nsHttpPipeline::GetConnectionInfo(nsHttpConnectionInfo**) ][@ nsHttpPipeline::GetConnectionInfo ]
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.
Attachment #495223 - Flags: review?(bzbarsky) → review+
Attachment #495223 - Flags: review?(cbiesinger)
http://hg.mozilla.org/mozilla-central/rev/2b63604957e2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review bz/biesi]
Target Milestone: --- → mozilla2.0b8
> looks like this still is showing up in buildid 20101207033246 and
> 2010120703032
...with the same crash daily rate.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Assignee: ehsan → honzab.moz
Status: REOPENED → ASSIGNED
- release mConnection of nsHttpPipeline after we call Close on all transactions
Attachment #496256 - Flags: review?(bzbarsky)
Attachment #496256 - Attachment description: v1 → Additional patch - v1
For reference: release of the connection has been introduced at this place in bug 176919.
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.
Boris, can you review (and assuming the changes looks good, land this) ASAP?
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?
Attachment #496256 - Flags: review?(bzbarsky) → review+
So... people who hit this bug presumably have HTTP pipelining enabled.  Interesting that we saw this at all.  ;)
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.
Keywords: reproducible
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?
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Thanks for killing this, guys!  Righteous.  :)
Er, make that http://hg.mozilla.org/mozilla-central/rev/81aea2200c34 for the comment fix (silly precommit hook!).
(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.
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.
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsHttpPipeline::GetConnectionInfo(nsHttpConnectionInfo**) ] [@ nsHttpPipeline::GetConnectionInfo ]
You need to log in before you can comment on or make changes to this bug.