427185 - JS Assertion from hell with gczeal 2

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

js debug shell trunk with gczeal 2

These have failed so far, but the run is young yet.

e4x/extensions/regress-337226.js
e4x/extensions/regress-352846-01.js
e4x/extensions/regress-352846-02.js
e4x/extensions/regress-352846-03.js

Assertion failure: (jsuword) (((uint32) ((jsuword) (thing) & ((jsuword) (((JSUint32)1 << (12)) - 1))) / (uint32) (sizeof(JSGCThing)))) < (jsuword) THINGS_PER_ARENA((((JSGCArenaInfo *)(((jsuword) (thing) | ((jsuword) (((JSUint32)1 << (12)) - 1))) + 1 - sizeof(JSGCArenaInfo))))->list->thingSize), at jsgc.c:2476

marking sensitive since this just seems evil.

Comment 1

[Damon Sicore (:damons)]

Bob, need more info here if I'm to determine blocking status.  Minusing until we have more info.

Comment 2

[Bob Clary [:bc] (inactive)]

I was hoping Igor could shed some light here, but it seems to me that there is a basic assumption that is being violated. This assertion was quite common during the test run.

Comment 3

[Igor Bukanov]

I am working on this GC hazard.

Comment 4

[Igor Bukanov]

Attachment: v1

The hazard is in js_NewXMLNamespaceObject. The function can call itself recursively when js_GetXMLNamespaceObject calls js_NewObject and that triggers construction of the prototype object. That in turn collects the initial namespace.
The patch fixes that and a similar hazard in js_NewXMLQNameObject. 

In addition the patch addresses yet another hazard in JS_InitClass. This function does not root the atomized class name. That hazard can not be exposed with SpiderMonkey classes as their names are pre-atomized. But if an embedding supplies a custom class and calls JS_InitClass before js_FunctionClass is initialized, then JS_InitClass calls itself recursively to initialize js_FunctionClass during the call to js_DefineFunction. That recursive call can displace all the newborn roots with new values leading to a hazard.

Comment 5

[Igor Bukanov]

Attachment: v1b

The new version fixes typos and removes leftovers from the debugging.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 314974 [details] [diff] [review]
v1b

> JS_InitClass(JSContext *cx, JSObject *obj, JSObject *parent_proto,
>              JSClass *clasp, JSNative constructor, uintN nargs,
>              JSPropertySpec *ps, JSFunctionSpec *fs,
>              JSPropertySpec *static_ps, JSFunctionSpec *static_fs)
> {
>     JSAtom *atom;
>     JSProtoKey key;
>     JSObject *proto, *ctor;
>+    jsval roots[2], cval, rval;
>     JSTempValueRooter tvr;
>-    jsval cval, rval;
>     JSBool named;
>     JSFunction *fun;
> 
>     CHECK_REQUEST(cx);
>-    atom = js_Atomize(cx, clasp->name, strlen(clasp->name), 0);

This is old, it was written assuming only last-ditch (keep-atoms) GCs could nest, I suppose.

>     /* Create a prototype object for this class. */
>     proto = js_NewObject(cx, clasp, parent_proto, obj, 0);
>     if (!proto)
>-        return NULL;
>+        goto out;
>+    roots[0] = OBJECT_TO_JSVAL(proto);

This was always dicey -- I remember trying to get proto connected ASAP to the constructor or (if Math, e.g.) to the global object. Better to root with a tvr, for sure, even before the reordering of js_Atomize w.r.t. this code.

>     if (!constructor) {
>         /*
>          * Lacking a constructor, name the prototype (e.g., Math) unless this
>          * class (a) is anonymous, i.e. for internal use only; (b) the class
>-         * of obj (the global object) is has a reserved slot indexed by key;
>+         * of obj (the global object) has a reserved slot indexed by key;
>          * and (c) key is not the null key.

Rewrap so "and" is on the line ending "key;"? Looks like it fits by comparison to the comment's first line.

> js_InitFunctionClass(JSContext *cx, JSObject *obj)
> {
>     JSObject *proto;
>-    JSAtom *atom;
>     JSFunction *fun;
> 
>     proto = JS_InitClass(cx, obj, NULL, &js_FunctionClass, Function, 1,
>                          function_props, function_methods, NULL, NULL);
>     if (!proto)
>         return NULL;
>-    atom = js_Atomize(cx, js_FunctionClass.name, strlen(js_FunctionClass.name),
>-                      0);
>-    if (!atom)
>-        goto bad;

Deadwood (atom is unused after this point)! How'd this code emerge over time? I have forgotton the history. Thanks for fixing. Was this involved in exposing the bug?

> typedef union JSTempValueUnion {
>     jsval               value;
>     JSObject            *object;
>     JSString            *string;
>     JSXML               *xml;
>     JSXMLQName          *qname;
>+    JSXMLNamespace      *ns;

Not the first member name that requires the macro parameter to be _-suffixied, but I wonder if it wouldn't be better (this is so a followup nit-bug, if it's anything) to use medium-length semi-canonical names for the members that do not clash with the local-variable short-names. E.g. qname for qn, object for obj, xmlobj for xml, and maybe nspace for ns?

Thanks for fixing this.

/be

Comment 7

[Igor Bukanov]

Attachment: v2

In v1 I forgot that the last ditch GC preserves atoms. Thus JS_InitClass is save. In principle, if an object creation hook can trigger a full GC, then this is indeed would be a hazard, but then it would be necessary to audit the whole code. For this is bug it is better to have a targeted fix which would require minimal porting to the branch.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 315299 [details] [diff] [review]
v2

GC safety fix, should take.

/be

Comment 9

[Igor Bukanov]

I checked in the patch from the comment 7 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1208160002&maxdate=1208160062&who=igor%25mir2.org

Comment 10

[Igor Bukanov]

The bug exists on the 1.8 branch.

Comment 11

[Igor Bukanov]

I backed out the checked in patch to investigate tinderbox orange:

Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.202; previous revision: 3.201
done
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.278; previous revision: 3.277
done
Checking in jsprvtd.h;
/cvsroot/mozilla/js/src/jsprvtd.h,v  <--  jsprvtd.h
new revision: 3.44; previous revision: 3.43
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.212; previous revision: 3.211
done

Comment 12

[Igor Bukanov]

I rechecked in the patch from the comment 7 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1208240918&maxdate=1208240942&who=igor%25mir2.org

Comment 13

[Bob Clary [:bc] (inactive)]

v 1.9.0 on fedora6

Comment 14

[Daniel Veditz [:dveditz]]

Brian: I think we need to roll this into the branch GCZeal patch.

Comment 15

[Brian Crowder]

This is an actual GC hazard, which should probably be back-ported(?), but isn't part of the overall GCZeal effort.  It is a separate bug, which GCZeal reveals.  The two patches should otherwise remain decoupled.

Comment 16

[Brian Crowder]

Attachment: simple backport to 1.8

Comment 17

[Brian Crowder]

Recording that this fix should land prior to the landing of the patch from bug 426628.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 323565 [details] [diff] [review]
simple backport to 1.8

Approved for 1.8.1.15, a=dveditz for release-drivers

Comment 19

[Brian Crowder]

Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.80.4.25; previous revision: 3.80.4.24
done
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.117.2.35; previous revision: 3.117.2.34
done
Checking in jsprvtd.h;
/cvsroot/mozilla/js/src/jsprvtd.h,v  <--  jsprvtd.h
new revision: 3.17.20.8; previous revision: 3.17.20.7
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.70; previous revision: 3.50.2.69
done

Comment 20

[Bob Clary [:bc] (inactive)]

e4x/extensions/regress-337226.js, e4x/extensions/regress-352846-01.js, e4x/extensions/regress-352846-02.js, e4x/extensions/regress-352846-03.js do not assert with a firefox 1.8.1 build and gczeal 2. I'm running a full browser based test run with gczeal=2, but it will take a while.

verified fixed linux.

Comment 21

[Alexander Sack]

afaict, not for 1.8.0. if I am missing something please ask for blocking1.8.0.15 again. Thanks!