80619 - js_NewContext can leave us with garbage atoms

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[John Bandhauer]

This is a regression from the fix to bug 72043. This 1 -> 0 -> 1 JSContext count 
transition is *still* not quite right :(

The issue here is the code:

    if (first) {
        ok = (rt->atomState.liveAtoms == 0)
             ? js_InitAtomState(cx, &rt->atomState)
             : JS_TRUE;

It fails to properly take into account that we could have one liveAtom left over 
from an intern'd string. So it skips the work done in js_InitAtomState and 
leaves previously collected (garbage) atoms in that set.

I see this in xpcshell by just starting it up and then doing a quit(). The 
current code path in xpcshell ends up creating a JSContext during xpconnect 
shutdown and hits this case.

This can be demonstated with a simple C program (this can be easily built by 
replacing the contents of js.c with the code below)...

/*****************************************************************************/

#include "jsapi.h"

static JSBool
global_resolve(JSContext *cx, JSObject *obj, jsval id)
{
    JSBool resolved;
    printf("resolver called\n");
    return JS_ResolveStandardClass(cx, obj, id, &resolved);
}

static JSClass global_class = {
    "global", 0,
    JS_PropertyStub,  JS_PropertyStub,  JS_PropertyStub,  JS_PropertyStub,
    JS_EnumerateStub, global_resolve,   JS_ConvertStub,   JS_FinalizeStub
};

int main()
{
    JSRuntime *rt;
    JSContext *cx;
    JSObject *glob;
    JSObject *foo;

    rt = JS_NewRuntime(8L * 1024L * 1024L);
    if (!rt)
        return 1;

    cx = JS_NewContext(rt, 4048);
    if (!cx)
        return 1;

    /* This will make rt->atomState.liveAtoms != 0 */
    (void) JS_InternString(cx, "foo");

    /* transition JSContext count from 1 to 0 */
    JS_DestroyContext(cx);

    cx = JS_NewContext(rt, 4048);
    if (!cx)
        return 1;

    glob = JS_NewObject(cx, &global_class, NULL, NULL);
    if (!glob)
        return 1;

    /* This will crash when a GetProperty lookup uses a garbage atom */
    foo = JS_NewObject(cx, NULL, glob, NULL);

    printf("ran w/o failure\n");    
    return 0;
}

/*****************************************************************************/

This crashes like...

js_GetProperty(JSContext * 0x00301c10, JSObject * 0x002fb200, long 0x00302b40, 
long * 0x0012ff1c) line 2208 + 43 bytes
js_NewObject(JSContext * 0x00301c10, JSClass * 0x100a1238 _js_ObjectClass, 
JSObject * 0x002fb200, JSObject * 0x00000000) line 1616 + 36 bytes
JS_NewObject(JSContext * 0x00301c10, JSClass * 0x100a1238 _js_ObjectClass, 
JSObject * 0x002fb200, JSObject * 0x00000000) line 1988 + 21 bytes
main() line 47 + 18 bytes

The CHECK_FOR_FUNNY_INDEX(id) macro crashes because the string for 
cx->runtime->atomState.constructorAtom is null. The case I see in xpcshell 
actually hits a garbage atom (evalAtom) when it gets into 
JS_ResolveStandardClasses. Tracing in that case shows the atom had been 
collected and the debug free routine had set the bits to 0xdddddddd.

I'm not clear on the best fix here. It seems to me that we want to always go 
into js_InitAtomState, but only init those atoms that are not already init'd. 
This implies that we'd need to be sure that atom table entries are set to some 
known 'invalid' state initially (which it appears is already the case?) And, 
when collected we'd need to set them back to that state. I'm not seeing the 
clearly best way to do this. Ideas?

Comment 1

[Brendan Eich [:brendan]]

The problem is the js_UnpinPinnedAtoms call in js_DestroyContext, in the "last"
case.  It leaves all the pinned rt->atomState.FooAtom pointers likely to dangle.
Missed that last time, in my haste.  I want off this ride!

Patch coming up.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix, please help me test and review

Comment 3

[John Bandhauer]

r/sr=jband

Doh! I had it in my mind that re-atomizing anything in that table would cause 
potential leaks of unreachable things. But, of course, these are just pointers 
to the gcthing atoms in the real atom table.

Factoring out js_InitPinnedAtoms looks just right. It works in my test case 
above and fixes the problem I saw in xpcshell.

> I want off this ride!

I've been with you every step of the way and I was amazed each time we 
discovered we didn't quite have it right. It sure looks right this time! Thanks.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

sr=shaver, and I agree with jband's sentiments: this code seems to somehow
resist predictive analysis.  Shame, really. =)

Comment 5

[Brendan Eich [:brendan]]

Fix checked in.  I hope that does it!

/be

Comment 6

[John Bandhauer]

*** Bug 82246 has been marked as a duplicate of this bug. ***