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)
Core
Networking
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)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
probably want to look at 363109 which landed yesterday and changed nsHttpTransaction::ParseHead()
Comment 2•14 years ago
|
||
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
Comment 3•14 years ago
|
||
Er, and actually reassigning, Michal, please see my previous comment.
Assignee: nobody → michal.novotny
Comment 4•14 years ago
|
||
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)
Updated•14 years ago
|
Summary: Firefox 4.0b8pre crash in [@ nsHttpPipeline::GetConnectionInfo(nsHttpConnectionInfo** → Firefox 4.0b8pre crash in [@ nsHttpPipeline::GetConnectionInfo(nsHttpConnectionInfo**) ]
Comment 6•14 years ago
|
||
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)
Updated•14 years ago
|
Whiteboard: [has patch][needs review bz/biesi]
Updated•14 years ago
|
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 7•14 years ago
|
||
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+
Updated•14 years ago
|
Attachment #495223 -
Flags: review?(cbiesinger)
Comment 8•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review bz/biesi]
Target Milestone: --- → mozilla2.0b8
Comment 9•14 years ago
|
||
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•14 years ago
|
||
> looks like this still is showing up in buildid 20101207033246 and
> 2010120703032
...with the same crash daily rate.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•14 years ago
|
||
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
Assignee | ||
Comment 12•14 years ago
|
||
- release mConnection of nsHttpPipeline after we call Close on all transactions
Attachment #496256 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•14 years ago
|
Attachment #496256 -
Attachment description: v1 → Additional patch - v1
Assignee | ||
Comment 13•14 years ago
|
||
For reference: release of the connection has been introduced at this place in bug 176919.
Comment 14•14 years ago
|
||
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•14 years ago
|
||
Boris, can you review (and assuming the changes looks good, land this) ASAP?
Comment 16•14 years ago
|
||
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+
Comment 17•14 years ago
|
||
So... people who hit this bug presumably have HTTP pipelining enabled. Interesting that we saw this at all. ;)
Reporter | ||
Comment 18•14 years ago
|
||
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
Comment 19•14 years ago
|
||
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 ago → 14 years ago
Resolution: --- → FIXED
Comment 20•14 years ago
|
||
Thanks for killing this, guys! Righteous. :)
Comment 21•14 years ago
|
||
Er, make that http://hg.mozilla.org/mozilla-central/rev/81aea2200c34 for the comment fix (silly precommit hook!).
Assignee | ||
Comment 22•14 years ago
|
||
(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.
Reporter | ||
Comment 23•14 years ago
|
||
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
Updated•13 years ago
|
Crash Signature: [@ nsHttpPipeline::GetConnectionInfo(nsHttpConnectionInfo**) ]
[@ nsHttpPipeline::GetConnectionInfo ]
You need to log in
before you can comment on or make changes to this bug.
Description
•