1383708 - Crash in mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::FindTransactionHelper

Status: RESOLVED WONTFIX

(Core :: Networking: HTTP, defect, P3)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-74049244-fbba-41c6-b774-372b90170722.
=============================================================

There is 1 crash in nightly 56 with buildid 20170721030204. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1378385.

[1] https://hg.mozilla.org/mozilla-central/rev?node=996576bdc8900d15ee95e60edba77cbac8599a03

Comment 1

[Honza Bambas (:mayhemer)]

Looking at this in VS using the raw dump, looks like the pendingQ points to a released memory:


-		pendingQ	0x0000025e3de5ce70 {...}	nsTArray<RefPtr<mozilla::net::nsHttpConnectionMgr::PendingTransactionInfo> > *
-		nsTArray_Impl<RefPtr<mozilla::net::nsHttpConnectionMgr::PendingTransactionInfo>,nsTArrayInfallibleAllocator>	{...}	nsTArray_Impl<RefPtr<mozilla::net::nsHttpConnectionMgr::PendingTransactionInfo>,nsTArrayInfallibleAllocator>
-		nsTArray_base<nsTArrayInfallibleAllocator,nsTArray_CopyWithMemutils>	{mHdr=0x0000025e3de5ce90 {mLength=0x00000052 mCapacity=0x65e5e5e5 mIsAutoArray=0x00000001 } }	nsTArray_base<nsTArrayInfallibleAllocator,nsTArray_CopyWithMemutils>
-		mHdr	0x0000025e3de5ce90 {mLength=0x00000052 mCapacity=0x65e5e5e5 mIsAutoArray=0x00000001 }	nsTArrayHeader *
		mLength	0x00000052	unsigned int
		mCapacity	0x65e5e5e5	unsigned int
		mIsAutoArray	0x00000001	unsigned int


The memory address the instruction is trying to dereference is at 0xe5e5e5e5e5e5e5e5 + 10h.  Not sure why this is reported as access to memory 0xffffffffff.

Comment 2

[Dragana Damjanovic [:dragana]]

I think this is an old bug. It is happening a bit more other with TFO(I think, not sure). I have change some parts of nsHttpConnectionMgr, especially that mEnt is ref counted now, therefore signatures changed as well.

mEnt is ref counted and it is not null (there is a diagnostic assert, that I have added) and pendingTransactionInfos are ref counted...how has this happened.

We have similar crashes for a long long time..

Comment 3

[Honza Bambas (:mayhemer)]

isn't there some NS_RELEASE(mEnt) left after that work has been done?  

I remember this was as issue when we were moving some other necko parts from NS_ADDREF/NS_RELEASE(mRawPtr) to be refcounted.  There were well hidden direct calls of Release() on refptrs letf.  

Looking into nsISupportsUtils.h tells me there is no `template<refptr<t>> NS_RELEASE() static_assert(false)` thingy to detect these at compile time.

Comment 4

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> isn't there some NS_RELEASE(mEnt) left after that work has been done?  
> 
> I remember this was as issue when we were moving some other necko parts from
> NS_ADDREF/NS_RELEASE(mRawPtr) to be refcounted.  There were well hidden
> direct calls of Release() on refptrs letf.  
> 
> Looking into nsISupportsUtils.h tells me there is no `template<refptr<t>>
> NS_RELEASE() static_assert(false)` thingy to detect these at compile time.

actually nsConnectionEntry did not implement AddRef/ReleaseRef at all. My change was added in bug 1381005.

Comment 5

[Kershaw Chang [:kershaw]]

Assign this to me first, but put this in [necko-backlog].

Comment 6

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 7

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.