635873 - Assertion failure: compartment == rt->gcCurrentCompartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

1. Install Spider http://bclary.com/projects/spider/spider/spider.xpi
2. firefox -spider -url https://www.citibank.hu/HUGCB/jba/exp/ProcessPaymentInputScreen.do -start -quit
3. Assertion failure: compartment == rt->gcCurrentCompartment on shutdown

Spider is required. SS since it involves shapes+compartments+GC.
Operating system: Windows NT
                  5.1.2600 Service Pack 3
CPU: x86
     GenuineIntel family 6 model 44 stepping 2
     1 CPU

Crash reason:  EXCEPTION_ACCESS_VIOLATION_WRITE
Crash address: 0x0

Thread 0 (crashed)
 0  mozjs.dll!JS_Assert [jsutil.cpp : 73 + 0x0]
    eip = 0x0084c2aa   esp = 0x0012abe0   ebp = 0x0012abe0   ebx = 0x00000001
    esi = 0x12b6b820   edi = 0xffff0007   eax = 0x00000000   ecx = 0x8ef81597
    edx = 0x003b3d38   efl = 0x00010212
    Found by: given as instruction pointer in context
 1  mozjs.dll!js::Shape::trace(JSTracer *) [jsscope.cpp : 1488 + 0x30]
    eip = 0x0081a045   esp = 0x0012abe8   ebp = 0x0012abfc
    Found by: call frame info
 2  mozjs.dll!js::Bindings::trace(JSTracer *) [jsscript.cpp : 294 + 0xb]
    eip = 0x0081c380   esp = 0x0012ac04   ebp = 0x0012ac10
    Found by: call frame info
 3  mozjs.dll!js_TraceScript(JSTracer *,JSScript *) [jsscript.cpp : 1709 + 0xe]
    eip = 0x0082036a   esp = 0x0012ac18   ebp = 0x0012ac34
    Found by: call frame info
 4  mozjs.dll!js_TraceStackFrame(JSTracer *,JSStackFrame *) [jsgc.cpp : 1489 + 0x11]
    eip = 0x0073d124   esp = 0x0012ac3c   ebp = 0x0012ac44
    Found by: call frame info
 5  mozjs.dll!js::StackSpace::mark(JSTracer *) [jscntxt.cpp : 242 + 0xc]
    eip = 0x006e46eb   esp = 0x0012ac4c   ebp = 0x0012ac6c
    Found by: call frame info
 6  mozjs.dll!JSThreadData::mark(JSTracer *) [jscntxt.cpp : 536 + 0xe]
    eip = 0x006e6116   esp = 0x0012ac74   ebp = 0x0012ac7c
    Found by: call frame info
 7  mozjs.dll!js::MarkRuntime(JSTracer *) [jsgc.cpp : 1739 + 0x12]
    eip = 0x0073e08b   esp = 0x0012ac84   ebp = 0x0012acd0
    Found by: call frame info
 8  mozjs.dll!MarkAndSweepCompartment [jsgc.cpp : 2292 + 0x8]
    eip = 0x007400e4   esp = 0x0012acd8   ebp = 0x0012ad68
    Found by: call frame info
 9  mozjs.dll!GCUntilDone [jsgc.cpp : 2748 + 0x10]
    eip = 0x0073fbf3   esp = 0x0012ad70   ebp = 0x0012ada8
    Found by: call frame info
10  mozjs.dll!js_GC(JSContext *,JSCompartment *,JSGCInvocationKind) [jsgc.cpp : 2819 + 0x10]
    eip = 0x0073f8fc   esp = 0x0012adb0   ebp = 0x0012add8
    Found by: call frame info
11  mozjs.dll!js::MaybeGC(JSContext *) [jsgc.cpp : 1838 + 0x20]
    eip = 0x0073e7a7   esp = 0x0012ade0   ebp = 0x0012adf4
    Found by: call frame info
12  mozjs.dll!JS_MaybeGC [jsapi.cpp : 2575 + 0x8]
    eip = 0x006a79b5   esp = 0x0012adfc   ebp = 0x0012ae00
    Found by: call frame info
13  xul.dll!nsJSContext::ScriptEvaluated(int) [nsJSEnvironment.cpp : 3173 + 0xc]
    eip = 0x10d640ef   esp = 0x0012ae08   ebp = 0x0012ae24
    Found by: call frame info
14  xul.dll!nsJSContext::ScriptExecuted() [nsJSEnvironment.cpp : 3245 + 0x28]
    eip = 0x10d6432c   esp = 0x0012ae2c   ebp = 0x0012ae30
    Found by: call frame info
15  xul.dll!AutoScriptEvaluate::~AutoScriptEvaluate() [xpcwrappedjsclass.cpp : 122 + 0x18]
    eip = 0x112a5567   esp = 0x0012ae38   ebp = 0x0012ae4c
    Found by: call frame info
16  xul.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *,unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) [xpcwrappedjsclass.cpp : 1911 + 0x29]
    eip = 0x112aa834   esp = 0x0012ae54   ebp = 0x0012b28c
    Found by: call frame info
17  xul.dll!nsXPCWrappedJS::CallMethod(unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) [xpcwrappedjs.cpp : 588 + 0x29]
    eip = 0x112a4413   esp = 0x0012b294   ebp = 0x0012b2cc
    Found by: call frame info
18  xul.dll!PrepareAndDispatch [xptcstubs.cpp : 114 + 0x20]
    eip = 0x11581306   esp = 0x0012b2d4   ebp = 0x0012b3a0
    Found by: call frame info
19  xul.dll!SharedStub [xptcstubs.cpp : 141 + 0x4]
    eip = 0x11580fd6   esp = 0x0012b3a8   ebp = 0x0012b3bc
    Found by: call frame info
20  xul.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct *,nsIDOMEventListener *,nsIDOMEvent *,nsPIDOMEventTarget *,unsigned int,nsCxPusher *) [nsEventListenerManager.cpp : 1127 + 0x11]
    eip = 0x10be1cef   esp = 0x0012b3c4   ebp = 0x0012b3bc
    Found by: call frame info with scanning

Comment 1

[Andreas Gal :gal]

This might be a bad bug. Bill can you please investigate?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

So far it doesn't look like this can cause anything bad to happen. Right now, the only code that stops us from crossing compartments while marking are some checks in the marking code for GC things (objects, strings, etc.). In the stack in comment 0, we don't pass through any of those checks. That's why the assertion triggers.

The danger when marking outside your compartment is that, if the mark bit isn't cleared at the end of the GC, then the next time around you'll mark too little and crash. Right now, the mark bits are not being reset at the end of GC, which is bad. Looking back at the patch history in bug 609104, I accidentally took this code out while fixing some review comments.

However, the marking code for shapes doesn't check if they're already marked; it scans them unconditionally. So, at worst, we may mark a few extra shapes. But we won't crash.

I'll post a patch in a bit. I don't think this should block, but we might want to take the patch anyway. I'll try to make it real safe.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix

This removes the assertion and puts back the code to clear the mark bits after a per-compartment GC. I also fixes some post-checkin nits that Brendan had in bug 609104 comment 67.

It's too bad to lose the assertion. However, when we start GC allocating shapes, this will all change for the better. Until then, I think this fix is the simplest.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 514228 [details] [diff] [review]
fix

>+        /*
>+         * Unmarked compartments containing marked objects don't get deleted,
>+         * except LAST_CONTEXT GC is performed.
>+         */

Need "when" after "except".

>         if ((!compartment->isMarked() && compartment->arenaListsAreEmpty())
>             || gckind == GC_LAST_CONTEXT)

Nit: || and && go at end of line.

>+    /*
>+     * Unmark all shapes. Even a per-compartment GC can mark shapes in other
>+     * compartments, and we need to clear these bits.
>+    */

Which other compartments? Sorry if I'm missing something obvious. A cross-compartment object-to-shape reference seems like a bug to me.

/be

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to comment #4)
> Which other compartments? Sorry if I'm missing something obvious. A
> cross-compartment object-to-shape reference seems like a bug to me.

As far as I can tell, there are no cross-compartment object-to-shape references here (although I haven't been able to reproduce the crash myself, so I don't know for sure).

When we do a per-compartment GC, we start tracing through the entire runtime; if we hit an object in the wrong compartment, we don't bother to trace through it. The problem here is that there aren't enough checks to ensure that we don't trace out of the compartment. You can see on the stack that we're going through a script and then a bindings object.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

http://hg.mozilla.org/tracemonkey/rev/0c398bad4c99

Also, there's no need for this to be security-sensitive.

Comment 7

[Gregor Wagner [:gwagner]]

Why are we performing a per-compartment GC during shutdown?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to comment #7)
> Why are we performing a per-compartment GC during shutdown?

Hopefully this was just a GC that happened around when we were about to shut down. The stack seems to say that it wasn't the last GC.

Comment 9

[Gregor Wagner [:gwagner]]

(In reply to comment #8)
> (In reply to comment #7)
> > Why are we performing a per-compartment GC during shutdown?
> 
> Hopefully this was just a GC that happened around when we were about to shut
> down. The stack seems to say that it wasn't the last GC.

Yeah you are right. That is a normal per-compartment GC.

It seems the marked shape comes from js_TraceScript. We should avoid marking things there if the script is from another compartment and we perform per-compartment GC. Another bug for this?

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0c398bad4c99