131640 - Mark XPCCallContext's mWrapper and veto GC during CallContext construction

Status: RESOLVED INCOMPLETE

(Core :: XPConnect, defect)

Description

[David Bradley]

This patch does three things:

1) Uses the inlined *Count accessors mentioned above
2) Moves CallContext member marking into the CallContext class and (the one
thing that might make a difference...) marks the CallContext's mWrapper member.

3) Adds code that vetos an attempt to do a GC during CallContext construction.

I'm not sure any of this will help. I have high hopes for number 2. Number 3 is

pathological, but I think there may be *some* chance that we hit this case and
are getting burned by it.

jband's comment from bug 130139.

I'm not all too confident about any of this. We need a case that triggers this
sort of problem more predicatably. Maybe a quick local hack to the JS engine to
make it GC randomly and frequently (during property accesses or something)
would help find the crasher cases with these gc'd interface set thingys.

BTW, Unless I'm completely mising it, we should change the summary of this
bug... I can't see that timeless is doing anything wrong here.

dbradley: I hope you can take this bug. I hacked a little, but I'm not
voluteering to take the bug at this point. Let me know if I can help.

Comment 1

[David Bradley]

Attachment: jband's patch from bug 130139

Comment 2

[David Bradley]

Moving out to 1.3. If this needs to be in before 1.3 please comment.

Comment 3

[David Bradley]

Moving to 1.4 Alpha

Comment 4

[David Bradley]

Moving out

Comment 5

[David Bradley]

Moving out, speak up if you believe this needs to be considered for 1.5b

Comment 6

[David Bradley]

Moving out

Comment 7

[timeless]

I applied the patch (there was one conflict, dbradley's comconnect, merging
wasn't hard) and built mozilla/js/src/xpconnect

then i ran this (in cmd) ~10 times:
start /min /low "xpcshell.dbg threads.js" xpcshell.exe
r:/mozilla/js/src/xpconnect/tests/js/old/threads.js

I got a new crash:
>	xpc3250.dll!XPCJSRuntime::GCCallback(JSContext * cx=0x00f73dc8, JSGCStatus
status=JSGC_END)  Line 525 + 0x6	C++
 	js3250.dll!js_GC(JSContext * cx=0x00f73dc8, unsigned int gcflags=0x00000000) 
Line 1420	C
 	js3250.dll!js_ForceGC(JSContext * cx=0x00f73dc8, unsigned int
gcflags=0x00000000)  Line 1000 + 0x1c	C
 	js3250.dll!js_DestroyContext(JSContext * cx=0x00f73dc8, JSGCMode
gcmode=JS_FORCE_GC)  Line 253 + 0x8	C
 	js3250.dll!JS_DestroyContext(JSContext * cx=0x00f73dc8)  Line 911 + 0xb	C
 	xpc3250.dll!XPCJSContextStack::~XPCJSContextStack()  Line 61	C++
 	xpc3250.dll!XPCJSContextStack::`scalar deleting destructor'()  + 0x8	C++
 	xpc3250.dll!XPCPerThreadData::Cleanup()  Line 388	C++
 	xpc3250.dll!XPCPerThreadData::~XPCPerThreadData()  Line 397	C++
 	xpc3250.dll!xpc_ThreadDataDtorCB(void * ptr=0x00f7ad90)  Line 429 + 0x7	C++
 	nspr4.dll!_PR_DestroyThreadPrivate(PRThread * self=0x00f846a0)  Line 260 + 0x10	C
 	nspr4.dll!_PR_CleanupThread(PRThread * thread=0x00f846a0)  Line 59 + 0x9	C
 	nspr4.dll!_PR_NativeRunThread(void * arg=0x00f846a0)  Line 463 + 0x9	C
 	nspr4.dll!pr_root(void * arg=0x00f846a0)  Line 113 + 0xd	C
 	msvcr70d.dll!_threadstartex(void * ptd=0x00f26de0)  Line 241 + 0xd	C
 	kernel32.dll!RegisterWaitForInputIdle()  + 0x43	

            case JSGC_END:
            {
                // NOTE that this event happens outside of the gc lock in
                // the js engine. So this could be sumultaneous [sp] with the
                // events above.

                // Release all the members whose JSObjects are now known
                // to be dead.

                dyingWrappedJSArray = &self->mWrappedJSToReleaseArray;
                XPCLock* lock = self->GetMainThreadOnlyGC() ?
                                nsnull : self->GetMapLock();
                while(1)
                {
                    nsXPCWrappedJS* wrapper;
                    {
                        XPCAutoLock al(lock); // lock if necessary
                        PRInt32 count = dyingWrappedJSArray->Count();
                        if(!count)
                        {
                            dyingWrappedJSArray->Compact();
                            break;
                        }
                        wrapper = NS_REINTERPRET_CAST(nsXPCWrappedJS*,
                                    dyingWrappedJSArray->ElementAt(count-1));
                        dyingWrappedJSArray->RemoveElementAt(count-1);
                    }
                    NS_RELEASE(wrapper); // Crash here
                }

-	wrapper	0x00f75c90 {mRefCnt={mValue=0xdddddddd }
_mOwningThread={mThread=0xdddddddd } mJSObj=0xdddddddd {map=??? slots=??? }
...}	nsXPCWrappedJS *

I'm not 100% certain that the this patch and this crash are related. it's just
that i'm used to a specific list of crashes for this testcase, and well, this is
new with the patch and relates to the subject.

None of the other threads is doing anything interesting anymore. hit and runs
suck. :(

Comment 8

[Andrew McCreight [:mccr8]]

I'm not too clear on what this is talking about, but I don't think we need it any more.