Closed Bug 427185 Opened 14 years ago Closed 14 years ago

JS Assertion from hell with gczeal 2


(Core :: JavaScript Engine, defect)

Not set





(Reporter: bc, Assigned: igor)



(Keywords: assertion, verified1.8.1.15, Whiteboard: [sg:critical?])


(2 files, 2 obsolete files)

js debug shell trunk with gczeal 2

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


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.
Flags: in-testsuite+
Flags: in-litmus-
Flags: blocking1.9?
Bob, need more info here if I'm to determine blocking status.  Minusing until we have more info.
Flags: blocking1.9? → blocking1.9-
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.
I am working on this GC hazard.
Assignee: general → igor
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #314971 - Flags: review?(brendan)
Attached patch v1b (obsolete) — Splinter Review
The new version fixes typos and removes leftovers from the debugging.
Attachment #314971 - Attachment is obsolete: true
Attachment #314974 - Flags: review?(brendan)
Attachment #314971 - Flags: review?(brendan)
Comment on attachment 314974 [details] [diff] [review]

> 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,, strlen(,
>-                      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.

Attachment #314974 - Flags: review?(brendan) → review+
Attached patch v2Splinter Review
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.
Attachment #314974 - Attachment is obsolete: true
Attachment #315299 - Flags: review?(brendan)
Comment on attachment 315299 [details] [diff] [review]

GC safety fix, should take.

Attachment #315299 - Flags: review?(brendan)
Attachment #315299 - Flags: review+
Attachment #315299 - Flags: approval1.9?
Attachment #315299 - Flags: approval1.9? → approval1.9+
I checked in the patch from the comment 7 to the trunk:
Closed: 14 years ago
Resolution: --- → FIXED
The bug exists on the 1.8 branch.
Flags: blocking1.8.1.15?
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
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.278; previous revision: 3.277
Checking in jsprvtd.h;
/cvsroot/mozilla/js/src/jsprvtd.h,v  <--  jsprvtd.h
new revision: 3.44; previous revision: 3.43
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.212; previous revision: 3.211
Resolution: FIXED → ---
I rechecked in the patch from the comment 7 to the trunk:
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
v 1.9.0 on fedora6
Whiteboard: [sg:critical?]
Flags: blocking1.8.1.15? → blocking1.8.1.15+
Brian: I think we need to roll this into the branch GCZeal patch.
Blocks: 308429
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.
No longer blocks: 308429
Attachment #323565 - Flags: review?(igor)
Recording that this fix should land prior to the landing of the patch from bug 426628.
Blocks: 426628
Attachment #323565 - Flags: review?(igor)
Attachment #323565 - Flags: review+
Attachment #323565 - Flags: approval1.8.1.15?
Comment on attachment 323565 [details] [diff] [review]
simple backport to 1.8

Approved for, a=dveditz for release-drivers
Attachment #323565 - Flags: approval1.8.1.15? → approval1.8.1.15+
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision:; previous revision:
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision:; previous revision:
Checking in jsprvtd.h;
/cvsroot/mozilla/js/src/jsprvtd.h,v  <--  jsprvtd.h
new revision:; previous revision:
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision:; previous revision:
Keywords: fixed1.8.1.15
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.
afaict, not for 1.8.0. if I am missing something please ask for blocking1.8.0.15 again. Thanks!
Flags: blocking1.8.0.15-
Group: security
You need to log in before you can comment on or make changes to this bug.