Closed Bug 616591 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: Networking, defect)

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) (deleted) — 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)
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)
Status: ASSIGNED → RESOLVED
Closed: 14 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
Attached patch Additional patch - v1 (deleted) — Splinter Review
- 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: 14 years ago14 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.

Attachment

General

Created:
Updated:
Size: