JS Assertion from hell with gczeal 2

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: bc, Assigned: Igor Bukanov)

Tracking

({assertion, verified1.8.1.15})

Trunk
x86
Linux
assertion, verified1.8.1.15
Points:
---
Bug Flags:
blocking1.9 -
blocking1.8.1.15 +
blocking1.8.0.next -
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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.
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-
(Reporter)

Comment 2

10 years ago
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.
(Assignee)

Comment 3

10 years ago
I am working on this GC hazard.
Assignee: general → igor
(Assignee)

Comment 4

10 years ago
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.
Attachment #314971 - Flags: review?(brendan)
(Assignee)

Comment 5

10 years ago
Created attachment 314974 [details] [diff] [review]
v1b

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]
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
Attachment #314974 - Flags: review?(brendan) → review+
(Assignee)

Comment 7

10 years ago
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.
Attachment #314974 - Attachment is obsolete: true
Attachment #315299 - Flags: review?(brendan)
Comment on attachment 315299 [details] [diff] [review]
v2

GC safety fix, should take.

/be
Attachment #315299 - Flags: review?(brendan)
Attachment #315299 - Flags: review+
Attachment #315299 - Flags: approval1.9?

Updated

10 years ago
Attachment #315299 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 9

10 years ago
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

10 years ago
The bug exists on the 1.8 branch.
Flags: blocking1.8.1.15?
(Assignee)

Comment 11

10 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

10 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 13

10 years ago
v 1.9.0 on fedora6
Status: RESOLVED → VERIFIED
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

Comment 15

9 years ago
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

Comment 16

9 years ago
Created attachment 323565 [details] [diff] [review]
simple backport to 1.8
Attachment #323565 - Flags: review?(igor)

Comment 17

9 years ago
Recording that this fix should land prior to the landing of the patch from bug 426628.
Blocks: 426628
(Assignee)

Updated

9 years ago
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 1.8.1.15, a=dveditz for release-drivers
Attachment #323565 - Flags: approval1.8.1.15? → approval1.8.1.15+

Comment 19

9 years ago
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
Keywords: fixed1.8.1.15
(Reporter)

Comment 20

9 years ago
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.
Keywords: fixed1.8.1.15 → verified1.8.1.15

Comment 21

9 years ago
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.