563210 - "Assertion failure: isDenseArrayMinLenCapOk()," with gc, gczeal, Array, defineGetter

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

try {
    __defineGetter__("x", gc)
} catch (e) {}
gczeal(1)
print(x)(Array(-8))


asserts js debug shell on TM tip without -j at Assertion failure: isDenseArrayMinLenCapOk(), at ../jsobjinlines.h:195

Setting s-s because this involves gc and gczeal.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to bug 562571:

The first bad revision is:
changeset:   41517:990192b0e052
user:        Nicholas Nethercote
date:        Thu Apr 29 20:22:33 2010 -0700
summary:     Bug 562571 - TM: don't have two bounds checks for array getelem.  r=brendan.

Comment 2

[Nicholas Nethercote [inactive]]

Yeah, bug 562571 is definitely responsible, that assertion was added in that revision.  I'll take a look tomorrow.

Comment 3

[Nicholas Nethercote [inactive]]

It's similar to a problem I saw when developing the patch for bug 562571:  getDenseArrayCapacity() is called and the MINLENCAP and LENGTH slots are still set to JSVAL_VOID (the numeric value of which is 22).  In other words, it's a question of initialisation.  Here's the stack trace:

#0  in __kernel_vsyscall ()
#1  in raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
#2  in JS_Assert at ../jsutil.cpp:79
#3  in JSObject::getDenseArrayCapacity at ../jsobjinlines.h:196
#4  in array_trace at ../jsarray.cpp:1239
#5  in JS_TraceChildren at ../jsgc.cpp:1833
#6  in js_CallGCMarker at ../jsgc.cpp:2109
#7  in JS_CallTracer at ../jsapi.cpp:1882
#8  in js_TraceStackFrame at ../jsgc.cpp:2299
#9  in TraceFrameChain at ../jsgc.cpp:2344
#10 in js_TraceContext at ../jsgc.cpp:2368
#11 in js_TraceRuntime at ../jsgc.cpp:2432
#12 in GC at ../jsgc.cpp:3007
#13 in GCUntilDone at ../jsgc.cpp:3188
#14 in js_GC at ../jsgc.cpp:3518
#15 in LastDitchGC at ../jsgc.cpp:1429
#16 in RefillFinalizableFreeList at ../jsgc.cpp:1453
#17 in js_NewFinalizableGCThing at ../jsgc.cpp:1561
#18 in js_NewGCFunction at ../jsgc.h:298
#19 in NewObjectWithGivenProto at ../jsobjinlines.h:505
#20 in NewObject at ../jsobjinlines.h:588
#21 in js_NewFunction at ../jsfun.cpp:2400
#22 in js_DefineFunction at ../jsfun.cpp:2554
#23 in js_InitExceptionClasses at ../jsexn.cpp:1026
#24 in js_GetClassObject at ../jsobj.cpp:3770
#25 in js_FindClassObject at ../jsobj.cpp:3835
#26 in js_GetClassPrototype at ../jsobj.cpp:5977
#27 in js_ErrorToException at ../jsexn.cpp:1157
#28 in ReportError at ../jscntxt.cpp:1321
#29 in js_ReportErrorNumberVA at ../jscntxt.cpp:1676
#30 in JS_ReportErrorNumber at ../jsapi.cpp:5395
#31 in ValueIsLength at ../jsarray.cpp:228
#32 in js_Array at ../jsarray.cpp:3350
#33 in js_Invoke at ../jsinterp.cpp:834
#34 in js_Interpret at ../jsops.cpp:2257
#35 in js_Execute at ../jsinterp.cpp:1103
#36 in JS_ExecuteScript at ../jsapi.cpp:4818
#37 in Process at ../../shell/js.cpp:449
#38 in ProcessArgs at ../../shell/js.cpp:863
#39 in main at ../../shell/js.cpp:5045

Looks we try to construct the Array(-8) which causes an exception to be thrown due to the negative size, which somehow causes a GC during which we hit an array that hasn't had its MINLENCAP/LENGTH slots initialised yet.  Brendan, do you have any idea about this?

FWIW, in this particular case nothing bad will happen, because capacity is set correctly for the getDenseArrayCapacity() call, but it's a concern that this has been hit.

Comment 4

[Brendan Eich [:brendan]]

There's nothing mysterious. Your analysis is accurate.

Built-in classes are initialized lazily in many embeddings, including the browser and shell. The exception classes being initialized require allocation of new gc-things, gczeal runs the GC, and finds the new array object via the stack, before it has been processed by InitArrayObject called from js_Array.

Nested GC can happen from many internal APIs, including JS_ReportErrorNumber from ValueIsLength. The array needs initializing before this point, at least of its fixed fields.

/be

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch just adds an extra initialisation, which feels very hacky to me but Brendan said on IRC that it was reasonable.

BTW, Jesse, I don't think this is a security bug, the assertion is a bit aggressive, ie. the slots in question are checked before they are ever used.

Comment 6

[Andreas Gal :gal]

Comment on attachment 443033 [details] [diff] [review]
patch

We have a custom array constructor for on-trace don't we? Is that one affected?

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 443033 [details] [diff] [review]
patch

A one-line comment such as "Pre-initialize in case of GC nesting here" would be good.

/be

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: patch v2

It's mixed metaphor time:  I decided it's better to bend in the wind than play
whack-a-mole with initialisation.  This patch:

- Removes JSCLASS_CONSTRUCT_PROTOTYPE from js_ArrayClass, it's no longer
  necessary.

- Gives isDenseArrayMinLenCapOk() an argument controlling how strict it
  should be with its checking (defaults to strict checking), and makes it
  public so it can be called within jstracer.cpp when the MINLENCAP slot is
  accessed directly.

- Adds the failing test case to js/src/tests/js1_8_5/regress, please let me
  know if this is not the right place for it.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 443224 [details] [diff] [review]
patch v2

>+ *    friend.  There is a function isDenseArrayMinLenCapOk() which checks that
>+ *    it is set correctly, it should be put in an assertion at use points.

How about "The function isDenseArrayMinLenCapOk() checks that it is set correctly; it should be put in an assertion at use points" for passive voice and comma splice avoidance.

>@@ -110,23 +110,28 @@ JSObject::setPrimitiveThis(jsval pthis)
>     fslots[JSSLOT_PRIMITIVE_THIS] = pthis;
> }
> 
> inline void JSObject::staticAssertArrayLengthIsInPrivateSlot()
> {
>     JS_STATIC_ASSERT(JSSLOT_ARRAY_LENGTH == JSSLOT_PRIVATE);
> }
> 
>+inline bool JSObject::isDenseArrayMinLenCapOk(bool strictWithLength) const

These two at least need line breaks after their return types.

> {
>     JS_ASSERT(isDenseArray());
>     uint32 length = uncheckedGetArrayLength();
>     uint32 capacity = uncheckedGetDenseArrayCapacity();
>     uint32 minLenCap = uint32(fslots[JSSLOT_DENSE_ARRAY_MINLENCAP]);
>-    return minLenCap == JS_MIN(length, capacity);
>+    // This function can be called while the LENGTH and MINLENCAP slots are
>+    // still set to JSVAL_VOID and there are no dslots (ie. the capacity is
>+    // zero).  If 'strictWithLength' is false we allow this.

Nit: blank line before comment taking 1+ lines.

Also, "With" has bad meaning in JS -- suggest "About" instead.

>+    JS_ASSERT(isDenseArrayMinLenCapOk(/*strictWithLength*/false));

Suggest "/* strictWithLength = */ false" -- let it breathe.

r=me w/ nits picked.

/be

Comment 10

[Nicholas Nethercote [inactive]]

Pushed with nits fixed:

http://hg.mozilla.org/tracemonkey/rev/bd32a5d4c86d

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/bd32a5d4c86d

Comment 12

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-563210.js.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.