254394 - nsHttpHandler PR_Assert WellOrdered

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[timeless]

This is amusing since it's ABA

XPCOM has a monitor for getservice (seen twice in this stack), and in the middle
there's a monitor at nsHttpConnectionMgr::Init.

 	nspr4.dll!PR_Assert(const char * s=0x0038f9cc, const char * file=0x0038f9a4,
int ln=0x000000e8)  Line 602	C
 	xpcom.dll!WellOrdered(const void * addr1=0x015e3e60, const void *
addr2=0x003ff6d0, const void * callsite2=0x00000000, unsigned int *
index2p=0x0012f1c4, nsNamedVector * * vec1p=0x0012f1c8, nsNamedVector * *
vec2p=0x0012f1d4)  Line 232 + 0x19	C++
 	xpcom.dll!nsAutoLockBase::nsAutoLockBase(void * addr=0x003ff6d0,
nsAutoLockBase::nsAutoLockType type=eAutoMonitor)  Line 281 + 0x15	C++
 	xpcom.dll!nsAutoMonitor::nsAutoMonitor(PRMonitor * mon=0x003ff6d0)  Line 247
+ 0x10	C++
 	xpcom.dll!nsComponentManagerImpl::GetService(const nsID & aClass={...}, const
nsID & aIID={...}, void * * result=0x0012f26c)  Line 2062	C++
 	xpcom.dll!nsGetServiceByCID::operator()(const nsID & aIID={...}, void * *
aInstancePtr=0x003ff064)  Line 101 + 0x15	C++
 	necko.dll!nsCOMPtr<nsIEventTarget>::assign_from_helper(const nsCOMPtr_helper &
helper={...}, const nsID & aIID={...})  Line 1050 + 0x10	C++
 	necko.dll!nsCOMPtr<nsIEventTarget>::operator=(const nsCOMPtr_helper &
rhs={...})  Line 643	C++
>	necko.dll!nsHttpConnectionMgr::Init(unsigned short maxConns=0x0018, unsigned
short maxConnsPerHost=0x0008, unsigned short maxConnsPerProxy=0x0008, unsigned
short maxPersistConnsPerHost=0x0002, unsigned short
maxPersistConnsPerProxy=0x0004, unsigned short maxRequestDelay=0x000a, unsigned
short maxPipelinedRequests=0x0004)  Line 105 + 0x1f	C++
 	necko.dll!nsHttpHandler::InitConnectionMgr()  Line 287 + 0x2f	C++
 	necko.dll!nsHttpHandler::Init()  Line 248	C++
 	necko.dll!nsHttpHandlerConstructor(nsISupports * aOuter=0x00000000, const nsID
& aIID={...}, void * * aResult=0x0012f3a8)  Line 187 + 0x4c	C++
 	xpcom.dll!nsGenericFactory::CreateInstance(nsISupports * aOuter=0x00000000,
const nsID & aIID={...}, void * * aResult=0x0012f3a8)  Line 82 + 0xe	C++
 	xpcom.dll!nsComponentManagerImpl::CreateInstance(const nsID & aClass={...},
nsISupports * aDelegate=0x00000000, const nsID & aIID={...}, void * *
aResult=0x015ea708)  Line 1886 + 0x10	C++
 	xpcom.dll!nsComponentManagerImpl::GetService(const nsID & aClass={...}, const
nsID & aIID={...}, void * * result=0x0012f3e0)  Line 2090	C++
 	xpcom.dll!nsGetServiceByCID::operator()(const nsID & aIID={...}, void * *
aInstancePtr=0x003ff064)  Line 101 + 0x15	C++
 	gkplugin.dll!nsCOMPtr<nsIHttpProtocolHandler>::assign_from_helper(const
nsCOMPtr_helper & helper={...}, const nsID & aIID={...})  Line 1050 + 0x10	C++
 
gkplugin.dll!nsCOMPtr<nsIHttpProtocolHandler>::nsCOMPtr<nsIHttpProtocolHandler>(const
nsCOMPtr_helper & helper={...})  Line 589	C++
 	gkplugin.dll!nsPluginHostImpl::UserAgent(const char * * retstring=0x0012f6ac)
 Line 2739 + 0x22	C++
 	jpins7.dll!6d35318c() 	
 	jpinsp.dll!6d366bb4() 	

There should be a way for B to avoid holding its monitor while it gets the STS.

I'll post a suggested patch which should resolve this.

Comment 1

[timeless]

Attachment: don't getservice while monitoring

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

+    nsCOMPtr<nsIEventTarget> sts = do_GetService(kSocketTransportServiceCID, &rv);

looks like this rv is now ignored.

Comment 3

[Darin Fisher]

Comment on attachment 155247 [details] [diff] [review]
don't getservice while monitoring

like biesi said: do something with the |rv| returned from do_GetService.  maybe
add |if NS_FAILED(rv)) return rv;|

i guess we normally don't hit this because the IO service would have already
initialized the STS.

Comment 4

[Justin Wood (:Callek)]

Attachment: Patch + rv verification

Same patch with review comments added (fixed for timeless)

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 156669 [details] [diff] [review]
Patch + rv verification

please fix erroneous \t's in my added lines on checkin (damn MSVC; Knew I
shouldnt have used it to create the patch)

If you'd rather have a patch with that fixed, request it and add me to CC at
the same time.

Comment 6

[Darin Fisher]

Comment on attachment 156669 [details] [diff] [review]
Patch + rv verification

r+sr=darin

i'll fix the indentation before landing this patch.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in by timeless 2004-08-26 06:45

Comment 8

[benc]

V/lxr:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsHttpConnectionMgr.cpp&branch=&root=/cvsroot&subdir=mozilla/netwerk/protocol/http/src&command=DIFF_FRAMESET&rev1=1.8&rev2=1.9

Comment 9

[Mike Kaply [:mkaply]]

Comment on attachment 156669 [details] [diff] [review]
Patch + rv verification

This doesn't strike me as branch material. Ping me if you think otherwise.

Comment 10

[timeless]

[e-mail]: among other problems, PR_Asserts are fatal.

Comment 11

[Mike Kaply [:mkaply]]

What causes this to actually happen?

Comment 12

[timeless]

really bad luck involving startup and java (or at least that's the case in
comment 0, it could probably happen some other way too).

Comment 13

[Mike Kaply [:mkaply]]

OK, so after drivers discussion, if it didn't make the aviary branch, and it's
gecko, it's not going in 1.7.5. This will be a 1.7.6 candidate.

We want 1.7.5 to match aviary as close as possible. Even crashes.