890243 - Crash in EnterBaseline when using multiple JSContexts per Runtime

Status: RESOLVED DUPLICATE of bug 905926

(Core :: JavaScript Engine, defect)

Description

[Yves Gwerder]

Attachment: callstack.txt

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130620122336

Steps to reproduce:

I'm working on upgrading Spidermonkey from 1.8.5 to 24 for 0 A.D. (http://play0ad.com/). At startup we run a hardware detection script and then load the GUI.

Some additional notes:
- I mentioned the issue on IRC and did some debugging together with djvj.
http://logbot.glob.com.au/?c=mozilla%23jsapi&s=2+Jul+2013&e=4+Jul+2013#c210959
- I switched to the newest Aurora branch today and it still happens
- I'm trying to create some sample code to reproduce the crash. The problem is that if you wanted to reproduce it the same way I did you'd have to download 0 A.D. and apply my WIP patch which is already several thousand lines long and quite cumbersome to apply.





Actual results:

- It did not crash with a non-threadsafe build and with different runtimes for both scripts.
- It also doesn't crash If I don't run the hardware detection script and directly start the GUI.
- It DOES crash with a THREADSAFE build when I run the hardware detection script and then run the GUI using the same runtime but a different context.

The crash seems to happend because it tries accessing JIT-code that already got cleaned up (check the attachments).




Expected results:

It should not crash or if I used the API incorrectly somewhere it could probably assert for that earlier.

Comment 1

[Yves Gwerder]

Attachment: Debugging session with djvj (also posten on IRC)

Comment 2

[Yves Gwerder]

Attachment: Instruction dump at the location of the crash

Comment 3

[Yves Gwerder]

Attachment: It's as simple as that to reproduce...

Comment 4

[Jan de Mooij [:jandem]]

This looks a bit suspicious in js::gc::MarkRuntime:

    /* Any Ion wrappers survive until the runtime is being torn down. */
    if (rt->hasContexts())
        ion::IonRuntime::Mark(trc);

Comment 5

[Kannan Vijayan [:djvj]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> This looks a bit suspicious in js::gc::MarkRuntime:
> 
>     /* Any Ion wrappers survive until the runtime is being torn down. */
>     if (rt->hasContexts())
>         ion::IonRuntime::Mark(trc);

Yeah, and GC is clearly hit due to the following path:

JS_DestroyContext (in jsapi.cpp) calls DestroyContext(cx, DCM_FORCE_GC)
DestroyContext removes the context, and then calls GC unconditionally.

If the removed context is the last one, this will collect the Ion trampoline stubs.
I don't know if this behaviour is really a bug?

Yves: Can you try keeping an extra context alive?

I.e. Keep a minimal context (no InitStandardClasses for the global, etc.) alive all the time, not touching it, and do the Setup/Teardown with separate contexts?

Comment 6

[Yves Gwerder]

Adding such a dummy-context after creating the runtime works around the issue.
I'd call it workaround but for me it's fine for the moment.

Comment 7

[Luke Wagner [:luke]]

On a related note, bholley is hard at work removing all of Gecko's dependencies on having multiple JSContexts and, once he is finished, we'll merge JSContext and JSRuntime.  In the meantime, it'd be a good idea to only create JSContexts and JSRuntimes in 1:1 pairs.

Comment 8

[Tracy Walker [:tracy]]

enterBaseline crashers are topcrash #4 on 23, #5 on 24 and #14 on 25. See bug 861503 and bug 858032

nominating for 24 and 25

Comment 9

[Scoobidiver (away)]

(In reply to Tracy Walker [:tracy] from comment #8)
> enterBaseline crashers are topcrash #4 on 23, #5 on 24 and #14 on 25. See
> bug 861503 and bug 858032
Bug 858032 is the top crasher. For this one, crash stats tell nothing.

Comment 10

[bhavana bajaj [:bajaj]]

Tracy looks like Kairo is saying that this may be expected : https://bugzilla.mozilla.org/show_bug.cgi?id=858032#c5 , unclear why we track then. Any more data we can get here to make it actionable or track ?

Comment 11

[Scoobidiver (away)]

(In reply to bhavana bajaj [:bajaj] (on vacation until 08/14/2013) from comment #10)
> Any more data we can get here to make it actionable or track ?
Any reproducible crash that blocks a top crasher is good to track unless it's known to cause only a small ratio of crashes.

Comment 12

[bhavana bajaj [:bajaj]]

Tracking as this is a reproducible case for the pool of [EnterBaseline] crashes in 858032.

Needinfoing :naveed to help with assignee here.

Comment 13

[Jan de Mooij [:jandem]]

I will look into this, though I doubt this is responsible for these topcrashes. The EnterBaseline signature is for crashes in JIT code and it's a collection of different bugs, many of them random memory corruption elsewhere.

Apparently lots of other things will fail if we destroy the last JSContext before the runtime is destroyed, see bug 905926 comment 0.

I should have a fix for this on Monday.

Comment 14

[Jan de Mooij [:jandem]]

Bill, I think we want something other than rt->hasContexts() in MarkRuntime. I think not marking atoms in this case is also a problem for Bobby's work in bug 905926, or am I missing something?

Just removing the rt->hasContexts() will undo bug 904282, so I'm not sure how to proceed.

    if (rt->hasContexts() &&
        !trc->runtime->isHeapMinorCollecting() &&
         (!IS_GC_MARKING_TRACER(trc) || rt->atomsCompartment()->zone()->isCollecting()))
     {
         MarkAtoms(trc);
         rt->staticStrings.trace(trc);
 #ifdef JS_ION
         ion::IonRuntime::Mark(trc);
 #endif
     }

Comment 15

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

Yeah, this has been a problem for a while. We've been pretty lazy using rt->hasContexts() as a proxy for lots of somewhat unrelated things (my fault). This will have to get fixed in bug 905926, so I'll make this depend on that.

However, Firefox always has at least one context around until shutdown, so it never triggers this problem. This bug is not causing any Firefox crashes and it should not be tracked.

Comment 16

[Jan de Mooij [:jandem]]

Thank you, clearing needinfo? flag.

Comment 18

[Jan de Mooij [:jandem]]

Bug 905926 should have fixed this, we now use rt->isBeingDestroyed() instead of !rt->hasContexts().