619529 - Crash [@ ToAttributeName] or [@ NewXMLQName] or "Assertion failure: proto,"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

b = Proxy.createFunction((function () {
    return {
        enumerateOwn: function () {
            @f
        }
    }
})(), Uint32Array)
function a() {
    Object.freeze(this);
    @r
}
a()

crashes js opt shell (signature not yet known, on Windows) and asserts js debug shell at Assertion failure: proto, on TM changeset c86a79eb18b9

Checked with gdb and it seems to be a null deref but I'm not sure..

Comment 1

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

The crash signature on opt shells is ToAttributeName.

Neither -m nor -j are needed.

Comment 2

[Nicholas Nethercote [inactive]]

Stack trace:

#0  0xf7fdf430 in __kernel_vsyscall ()
#1  0xf7fae610 in raise (sig=6)
    at ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
#2  0x081bd74d in JS_Assert (s=0x834fedc "proto", 
    file=0x834ffc9 "../jsobjinlines.h", ln=806) at ../jsutil.cpp:83
#3  0x081c604a in NewNativeClassInstance (cx=0x8407ca8, clasp=0x83c2280, 
    proto=0x0, parent=0xf7502028, kind=js::gc::FINALIZE_OBJECT4)
    at ../jsobjinlines.h:806
#4  0x081c6338 in NewBuiltinClassInstance (cx=0x8407ca8, clasp=0x83c2280, 
    kind=js::gc::FINALIZE_OBJECT4) at ../jsobjinlines.h:883
#5  0x081c6375 in NewBuiltinClassInstance (cx=0x8407ca8, clasp=0x83c2280)
    at ../jsobjinlines.h:890
#6  0x081c6806 in NewBuiltinClassInstanceXML (cx=0x8407ca8, clasp=0x83c2280)
    at ../jsxml.cpp:225
#7  0x081c7244 in NewXMLQName (cx=0x8407ca8, uri=0xf7500020, 
    prefix=0xf7500020, localName=0x83a8e80, clasp=0x83c2280)
    at ../jsxml.cpp:538
#8  0x081ce3cf in ToAttributeName (cx=0x8407ca8, v=...) at ../jsxml.cpp:2813
#9  0x081db0b1 in js_ToAttributeName (cx=0x8407ca8, vp=0xffffab98)
    at ../jsxml.cpp:7278
#10 0x083004f0 in js::Interpret (cx=0x8407ca8, entryFrame=0xf77870c8, 
    inlineCallCount=0, interpMode=JSINTERP_NORMAL) at ../jsinterp.cpp:6354
#11 0x08100508 in js::RunScript (cx=0x8407ca8, script=0x841b158, 
    fp=0xf77870c8) at ../jsinterp.cpp:657
#12 0x0810096a in js::Invoke (cx=0x8407ca8, argsRef=..., flags=0)
    at ../jsinterp.cpp:737
#13 0x08100f9b in js::ExternalInvoke (cx=0x8407ca8, thisv=..., fval=..., 
    argc=0, argv=0x0, rval=0xffffb628) at ../jsinterp.cpp:858
#14 0x081652bc in ExternalInvoke (cx=0x8407ca8, obj=0xf750a048, fval=..., 
    argc=0, argv=0x0, rval=0xffffb628) at ../jsinterp.h:962
#15 0x08167229 in Trap (cx=0x8407ca8, handler=0xf750a048, fval=..., argc=0, 
    argv=0x0, rval=0xffffb628) at ../jsproxy.cpp:338
#16 0x08168b0d in js::JSScriptedProxyHandler::enumerateOwn (this=0x83c51fc, 
    cx=0x8407ca8, proxy=0xf75070d0, props=...) at ../jsproxy.cpp:635
#17 0x08169671 in js::JSProxy::enumerateOwn (cx=0x8407ca8, proxy=0xf75070d0, 
    props=...) at ../jsproxy.cpp:794
#18 0x081096c1 in Snapshot<KeyEnumeration> (cx=0x8407ca8, obj=0xf75070d0, 
    flags=8, props=0xffffb7e8) at ../jsiter.cpp:331
#19 0x081067a1 in js::GetPropertyNames (cx=0x8407ca8, obj=0xf75070d0, 
    flags=8, props=0xffffb7e8) at ../jsiter.cpp:393
#20 0x08071b28 in JS_Enumerate (cx=0x8407ca8, obj=0xf75070d0)
    at ../jsapi.cpp:3856
#21 0x08117fdb in MarkSharpObjects (cx=0x8407ca8, obj=0xf75070d0, idap=0x0)
    at ../jsobj.cpp:227
#22 0x0811828e in MarkSharpObjects (cx=0x8407ca8, obj=0xf7502028, 
    idap=0xffffba3c) at ../jsobj.cpp:266
#23 0x081184c3 in js_EnterSharpObject (cx=0x8407ca8, obj=0xf7502028, 
    idap=0xffffbb78, sp=0xffffbb74) at ../jsobj.cpp:336
#24 0x081189ca in obj_toSource (cx=0x8407ca8, argc=0, vp=0xf77870a8)
    at ../jsobj.cpp:498
#25 0x081040f8 in js::CallJSNative (cx=0x8407ca8, 
    native=0x81188f1 <obj_toSource>, argc=0, vp=0xf77870a8)
    at ../jscntxtinlines.h:684
#26 0x0810077c in js::Invoke (cx=0x8407ca8, argsRef=..., flags=0)
    at ../jsinterp.cpp:700
#27 0x08100f9b in js::ExternalInvoke (cx=0x8407ca8, thisv=..., fval=..., 
    argc=0, argv=0x0, rval=0xffffbd48) at ../jsinterp.cpp:858
#28 0x08116b64 in ExternalInvoke (cx=0x8407ca8, obj=0xf7502028, fval=..., 
    argc=0, argv=0x0, rval=0xffffbd48) at ../jsinterp.h:962
#29 0x08128ceb in js_TryMethod (cx=0x8407ca8, obj=0xf7502028, 
    atom=0xf75009a0, argc=0, argv=0x0, rval=0xffffbd48) at ../jsobj.cpp:6152
#30 0x081a0b6b in js_ValueToSource (cx=0x8407ca8, v=...) at ../jsstr.cpp:3807
#31 0x0814771b in js_DecompileValueGenerator (cx=0x8407ca8, spindex=0, 
    v_in=..., fallback=0x0) at ../jsopcode.cpp:5179
#32 0x0809e661 in DecompileValueGenerator (cx=0x8407ca8, spindex=0, v=..., 
    fallback=0x0) at ../jsopcode.h:493
#33 0x080a2659 in js_ReportValueErrorFlags (cx=0x8407ca8, flags=0, 
    errorNumber=239, spindex=0, v=..., fallback=0x0, arg1=0x0, arg2=0x0)
    at ../jscntxt.cpp:1807
#34 0x08126524 in JSObject::reportNotExtensible (this=0xf7502028, 
    cx=0x8407ca8, report=0) at ../jsobj.cpp:5344
#35 0x0818e97f in JSObject::addProperty (this=0xf7502028, cx=0x8407ca8, 
    id=..., getter=0x806e57a <JS_PropertyStub>, 
    setter=0x806e57a <JS_PropertyStub>, slot=95, attrs=0, flags=0, shortid=0)
    at ../jsscope.cpp:773
#36 0x081220d1 in DefineStandardSlot (cx=0x8407ca8, obj=0xf7502028, 
    key=JSProto_AttributeName, atom=0xf7501020, v=..., attrs=0, 
    named=@0xffffbfcf) at ../jsobj.cpp:3684
#37 0x08122405 in js_InitClass (cx=0x8407ca8, obj=0xf7502028, 
    parent_proto=0xf7502078, clasp=0x83c2280, 
    constructor=0x81c8192 <AttributeName>, nargs=2, ps=0x83c2760, 
    fs=0x83c23b4, static_ps=0x0, static_fs=0x0) at ../jsobj.cpp:3788
#38 0x081da91f in js_InitAttributeNameClass (cx=0x8407ca8, obj=0xf7502028)
    at ../jsxml.cpp:7085
#39 0x0812309a in js_GetClassObject (cx=0x8407ca8, obj=0xf7502028, 
    key=JSProto_AttributeName, objp=0xffffc0e4) at ../jsobj.cpp:4085
#40 0x0812334e in js_FindClassObject (cx=0x8407ca8, start=0x0, 
    protoKey=JSProto_AttributeName, vp=0xffffc120, clasp=0x83c2280)
    at ../jsobj.cpp:4150
#41 0x08128683 in js::FindClassPrototype (cx=0x8407ca8, scopeobj=0xf7502028, 
    protoKey=JSProto_AttributeName, protop=0xffffc170, clasp=0x83c2280)
    at ../jsobj.cpp:5993
#42 0x081c6303 in NewBuiltinClassInstance (cx=0x8407ca8, clasp=0x83c2280, 
    kind=js::gc::FINALIZE_OBJECT4) at ../jsobjinlines.h:879
#43 0x081c6375 in NewBuiltinClassInstance (cx=0x8407ca8, clasp=0x83c2280)
    at ../jsobjinlines.h:890
#44 0x081c6806 in NewBuiltinClassInstanceXML (cx=0x8407ca8, clasp=0x83c2280)
    at ../jsxml.cpp:225
#45 0x081c7244 in NewXMLQName (cx=0x8407ca8, uri=0xf7500020, 
    prefix=0xf7500020, localName=0x83a8f40, clasp=0x83c2280)
    at ../jsxml.cpp:538
#46 0x081ce3cf in ToAttributeName (cx=0x8407ca8, v=...) at ../jsxml.cpp:2813
#47 0x081db0b1 in js_ToAttributeName (cx=0x8407ca8, vp=0xffffc498)
    at ../jsxml.cpp:7278
#48 0x083004f0 in js::Interpret (cx=0x8407ca8, entryFrame=0xf7787030, 
    inlineCallCount=1, interpMode=JSINTERP_NORMAL) at ../jsinterp.cpp:6354
#49 0x08100508 in js::RunScript (cx=0x8407ca8, script=0x841aab8, 
    fp=0xf7787030) at ../jsinterp.cpp:657
#50 0x081016a9 in js::Execute (cx=0x8407ca8, chain=0xf7502028, 
    script=0x841aab8, prev=0x0, flags=0, result=0x0) at ../jsinterp.cpp:1005
#51 0x08073f60 in JS_ExecuteScript (cx=0x8407ca8, obj=0xf7502028, 
    script=0x841aab8, rval=0x0) at ../jsapi.cpp:4892
#52 0x0804c552 in Process (cx=0x8407ca8, obj=0xf7502028, 
    filename=0xffffd2c5 "a.js", forceTTY=0) at ../../shell/js.cpp:453
#53 0x0804d51d in ProcessArgs (cx=0x8407ca8, obj=0xf7502028, argv=0xffffd0c8, 
    argc=1) at ../../shell/js.cpp:951
#54 0x08056b34 in Shell (cx=0x8407ca8, argc=1, argv=0xffffd0c8, 
    envp=0xffffd0d0) at ../../shell/js.cpp:5372
#55 0x08056d0f in main (argc=1, argv=0xffffd0c8, envp=0xffffd0d0)
    at ../../shell/js.cpp:5480

Comment 3

[Jesse Ruderman]

Attachment: stack trace for opt crash

Comment 4

[Jesse Ruderman]

Attachment: stack trace for debug assert

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: tentative patch (against TM 58597:dab87d48d3dd)

The problem is that NewNativeClassInstance() requires that 'proto' be
non-NULL.  But this call to FindClassPrototype() in NewBuiltinClassInstance():

    if (v.isObject()) {
        proto = &v.toObject();
        JS_ASSERT(proto->getParent() == global);
    } else {
        if (!FindClassPrototype(cx, global, protoKey, &proto, clasp))
            return NULL;
    }

can set 'proto' to NULL while also returning 'true'.

So, the question is:  is FindClassPrototype() wrong for setting *protop to
NULL and returning 'true', or is
NewBuiltinClassInstance()/NewNativeClassInstance() wrong for assuming proto
cannot be NULL?  From some experiments, I suspect the latter.

Here's a patch that just changes NewNativeClassInstance() to just return
NULL if proto is NULL.  I have no idea if it's correct but it avoids the
assertion failures, and also passes jit-tests and jstests.  If this is
acceptable I'll add the program in comment 0 to tests/js1_8_5/regress.

Comment 6

[Robert Sayre]

Comment on attachment 497946 [details] [diff] [review]
tentative patch (against TM 58597:dab87d48d3dd)

nnethercote didn't pick a reviewer, so I did it

Comment 7

[Nicholas Nethercote [inactive]]

Huh, I could have sworn I put brendan down as reviewer.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

You have to be a little careful because if you make a typo in the reviewer field and the result doesn't match anybody, bugzilla will request review from nobody.

Comment 9

[Brendan Eich [:brendan]]

For what protoKey and under what other conditions does FindClassPrototype succeed with *protop null?

/be

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> For what protoKey and under what other conditions does FindClassPrototype
> succeed with *protop null?

Here's the code:

bool
js::FindClassPrototype(JSContext *cx, JSObject *scopeobj, JSProtoKey protoKey,
                       JSObject **protop, Class *clasp)
{
    Value v;
    if (!js_FindClassObject(cx, scopeobj, protoKey, &v, clasp))
        return false;

    if (IsFunctionObject(v)) {
        JSObject *ctor = &v.toObject();
        if (!ctor->getProperty(cx, ATOM_TO_JSID(cx->runtime->atomState.classProt
otypeAtom), &v))
            return false;
    }

    *protop = v.isObject() ? &v.toObject() : NULL;
    return true;
}

Some quick assertery/debugging shows it occurs often with protoKey equal to JSProto_Function, JSProto_Null, JSProto_Object, JSProto_JSON, and probably others.  But it looks like |v.isUndefined()| is always true when it occurs.  Looks like this is related to the case in js_FindClassObject() where no global object is found, and |vp->setUndefined()| is called?

Comment 11

[Brendan Eich [:brendan]]

No need to quote code, the dynamics are what count and they are hard to figure out statically. But I should have looked at the test harder.

Blake, what is it about attribute names that leads to no global?

/be

Comment 12

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

A variation also crashes opt shell at NewXMLQName and asserts dbg shell similarly, but unfortunately the testcase was lost.

Comment 13

[Igor Bukanov]

Comment on attachment 497946 [details] [diff] [review]
tentative patch (against TM 58597:dab87d48d3dd)

This is a wrong fix. We should find a real culprit of the assert and fix it, not add a silent failure return path. I suppose the issue is somewhere in the lazy initialization of some properties that no longer works after Object.freeze(). 

This is especially visible with a simplified test case:

var b = Proxy.create({ enumerateOwn: function () { @f; }});

Object.freeze(this);

@r

Comment 14

[Igor Bukanov]

I consider the short example:

var b = Proxy.create({ enumerateOwn: function () { @f; }});
Object.freeze(this);
@r

The reason for the bug is that @r triggers initialization of AttributeName class. That fails when DefineStandardSlot tries to add AttributeName constructor to the frozen global at http://hg.mozilla.org/tracemonkey/file/e30d605097be/js/src/jsobj.cpp#l3720 . That leads to error reporting in JSObject::addName which eventually calls ValueToSource leading to enumeration of global properties recursively and a call to Proxy::getOwnProperties. That in turn triggers a call to enumerateOwn handler and evaluation of @f.

This evaluation leads to recursive AttributeName class initialization. Such recursion is detected in js_GetClassObject leading to an early successful return with NULL prototype. That NULL is propagated and eventually leads to the NewBuiltinClassInstance call with NULL prototype.

So we have several bugs here:

1. Object.freeze(global_object) does not initialize the lazily initialized names.

2. DefineStandardSlot leaves the standard slot for the created constructor initialized even if JSObject::addProperty fails.

3. js_GetClassObject assumes that recursion with class initialization is OK and returns NULL if so.

I will try to fix 2 and 3. As for 1 do we have a bug already failed about it?

Comment 15

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   54300:7ef107ab081e
user:        Brendan Eich
date:        Thu Sep 16 11:56:54 2010 -0700
summary:     Fix shape vs. slot management under putProperty, plus related layering and error reporting fixes (596805, r=jorendorff).

Comment 16

[Igor Bukanov]

Attachment: v1

The patch turns AnyName and AttributeName into anonymous classes without any prototypes stored in the global proto slots. This removes the need for the corresponding InitClass functions and allows to construct instances of the classes even for a global that was frozen before XML classes were initialized.

The consequence of the patch is that AttributeName is no longer defined in the global scope so this may break scripts that relied on that. I suppose this is unlikely. According to E4X specs AttributeName is invisible to scripts and an existence of AttributeName was an implementation detail of SpiderMonkey that was not advertised.

Still if some scrips relied on it it would be easy to add this global constructor just for the sake of compatibility.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 501836 [details] [diff] [review]
v1

NewXMLAttribiteName is misspelled. Looks good otherwise, thanks!

/be

Comment 18

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/1e5925b72c51

Comment 19

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/1e5925b72c51

Comment 20

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/e4x/QName/regress-619529.js.

Comment 21

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

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