Last Comment Bug 427185 - JS Assertion from hell with gczeal 2
: JS Assertion from hell with gczeal 2
Status: VERIFIED FIXED
[sg:critical?]
: assertion, verified1.8.1.15
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 426628
  Show dependency treegraph
 
Reported: 2008-04-04 22:27 PDT by Bob Clary [:bc:]
Modified: 2008-07-02 02:14 PDT (History)
6 users (show)
dsicore: blocking1.9-
dveditz: blocking1.8.1.15+
asac: blocking1.8.0.next-
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (9.90 KB, patch)
2008-04-10 15:28 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
v1b (9.29 KB, patch)
2008-04-10 15:44 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
v2 (6.43 KB, patch)
2008-04-12 14:01 PDT, Igor Bukanov
brendan: review+
mtschrep: approval1.9+
Details | Diff | Splinter Review
simple backport to 1.8 (6.27 KB, patch)
2008-06-03 08:28 PDT, Brian Crowder
igor: review+
dveditz: approval1.8.1.15+
Details | Diff | Splinter Review

Description Bob Clary [:bc:] 2008-04-04 22:27:44 PDT
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) 2008-04-08 17:51:06 PDT
Bob, need more info here if I'm to determine blocking status.  Minusing until we have more info.
Comment 2 Bob Clary [:bc:] 2008-04-08 18:09:39 PDT
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 2008-04-10 13:40:54 PDT
I am working on this GC hazard.
Comment 4 Igor Bukanov 2008-04-10 15:28:07 PDT
Created attachment 314971 [details] [diff] [review]
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 2008-04-10 15:44:30 PDT
Created attachment 314974 [details] [diff] [review]
v1b

The new version fixes typos and removes leftovers from the debugging.
Comment 6 Brendan Eich [:brendan] 2008-04-12 11:03:01 PDT
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 2008-04-12 14:01:59 PDT
Created attachment 315299 [details] [diff] [review]
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] 2008-04-12 21:43:19 PDT
Comment on attachment 315299 [details] [diff] [review]
v2

GC safety fix, should take.

/be
Comment 10 Igor Bukanov 2008-04-14 01:09:04 PDT
The bug exists on the 1.8 branch.
Comment 11 Igor Bukanov 2008-04-14 02:34:06 PDT
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 13 Bob Clary [:bc:] 2008-04-19 06:49:29 PDT
v 1.9.0 on fedora6
Comment 14 Daniel Veditz [:dveditz] 2008-06-02 11:51:48 PDT
Brian: I think we need to roll this into the branch GCZeal patch.
Comment 15 Brian Crowder 2008-06-02 12:10:11 PDT
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 2008-06-03 08:28:02 PDT
Created attachment 323565 [details] [diff] [review]
simple backport to 1.8
Comment 17 Brian Crowder 2008-06-03 08:28:46 PDT
Recording that this fix should land prior to the landing of the patch from bug 426628.
Comment 18 Daniel Veditz [:dveditz] 2008-06-04 11:29:35 PDT
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 2008-06-04 11:49:24 PDT
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:] 2008-06-11 07:08:06 PDT
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 2008-06-16 09:57:17 PDT
afaict, not for 1.8.0. if I am missing something please ask for blocking1.8.0.15 again. Thanks!

Note You need to log in before you can comment on or make changes to this bug.