624968 - Assertion failure: proto->canProvideEmptyShape

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

mozilla-central winxp/win7

1. http://www.detelefoongids.nl/bg-l/12054480-Essen%252BAuto%252527s%252BVan/vermelding/?edsacid=i-254936-30-61

2. Assertion failure: proto->canProvideEmptyShape(clasp), at 
js\src\jsobjinlines.h:919

>	mozjs.dll!JS_Assert(const char * s=0x00a246d0, const char * file=0x00a24694, int ln=919)  Line 73	C++
 	mozjs.dll!js::NewNativeClassInstance(JSContext * cx=0x08506028, js::Class * clasp=0x00bb0e60, JSObject * proto=0x09b6e820, JSObject * parent=0x09b6e0a0, js::gc::FinalizeKind kind=FINALIZE_OBJECT0)  Line 919 + 0x29 bytes	C++
 	mozjs.dll!js::NewNativeClassInstance(JSContext * cx=0x08506028, js::Class * clasp=0x00bb0e60, JSObject * proto=0x09b6e820, JSObject * parent=0x09b6e0a0)  Line 935 + 0x19 bytes	C++
 	mozjs.dll!js_ErrorToException(JSContext * cx=0x08506028, const char * message=0x0d82e4d0, JSErrorReport * reportp=0x0012c970, const JSErrorFormatString * (void *, const char *, const unsigned int)* callback=0x006634e0, void * userRef=0x00000000)  Line 1190 + 0x1b bytes	C++
 	mozjs.dll!ReportError(JSContext * cx=0x08506028, const char * message=0x0d82e4d0, JSErrorReport * reportp=0x0012c970, const JSErrorFormatString * (void *, const char *, const unsigned int)* callback=0x006634e0, void * userRef=0x00000000)  Line 1294 + 0x29 bytes	C++
 	mozjs.dll!js_ReportErrorNumberVA(JSContext * cx=0x08506028, unsigned int flags=0, const JSErrorFormatString * (void *, const char *, const unsigned int)* callback=0x006634e0, void * userRef=0x00000000, const unsigned int errorNumber=22, int charArgs=1, char * ap=0x0012c9e0)  Line 1646 + 0x19 bytes	C++
 	mozjs.dll!JS_ReportErrorFlagsAndNumber(JSContext * cx=0x08506028, unsigned int flags=0, const JSErrorFormatString * (void *, const char *, const unsigned int)* errorCallback=0x006634e0, void * userRef=0x00000000, const unsigned int errorNumber=22, ...)  Line 5655 + 0x1f bytes	C++
 	mozjs.dll!js_ReportValueErrorFlags(JSContext * cx=0x08506028, unsigned int flags=0, const unsigned int errorNumber=22, int spindex=-5, const js::Value & v={...}, JSString * fallback=0x00000000, const char * arg1=0x00000000, const char * arg2=0x00000000)  Line 1776 + 0x24 bytes	C++
 	mozjs.dll!js_ReportIsNotFunction(JSContext * cx=0x08506028, const js::Value * vp=0x05080370, unsigned int flags=0)  Line 2982 + 0x21 bytes	C++
 	mozjs.dll!js::Invoke(JSContext * cx=0x08506028, const js::CallArgs & argsRef={...}, unsigned int flags=0)  Line 675 + 0x1c bytes	C++
 	mozjs.dll!js::mjit::stubs::SlowCall(js::VMFrame & f={...}, unsigned int argc=3)  Line 197 + 0x1f bytes	C++
 	mozjs.dll!js::mjit::ic::NativeCall(js::VMFrame & f={...}, js::mjit::ic::CallICInfo * ic=0x083cf308)  Line 900	C++
 	0590a8d1()	
 	mozjs.dll!js::mjit::EnterMethodJIT(JSContext * cx=0x08506028, JSStackFrame * fp=0x05080038, void * code=0x0cbb8664, js::Value * stackLimit=0x0510ca70)  Line 748 + 0x15 bytes	C++
 	mozjs.dll!CheckStackAndEnterMethodJIT(JSContext * cx=0x08506028, JSStackFrame * fp=0x05080038, void * code=0x0cbb8664)  Line 774 + 0x15 bytes	C++
 	mozjs.dll!js::mjit::JaegerShot(JSContext * cx=0x08506028)  Line 791 + 0x19 bytes	C++
 	mozjs.dll!js::RunScript(JSContext * cx=0x08506028, JSScript * script=0x0ccd3fd0, JSStackFrame * fp=0x05080038)  Line 654 + 0x9 bytes	C++

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: testcase

reduced from the Yahoo! api.

  new ({ prototype: TypeError.prototype });

Comment 2

[Jason Orendorff [:jorendorff]]

OMG, bclary. You have outdone yourself.

  Achievement unlocked: Sharks with frickin' laser beams!
  (Reduce a testcase by 99.98% or more)

Taking.

Comment 3

[Jason Orendorff [:jorendorff]]

Oh. This is because there's some important code in js_InitClass that's missing from js_InitExceptionClasses. js::NewNativeClassInstance and I both assumed that the latter called the former; it doesn't.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v1 - non-minimal fix

It seemed better to common up code than to duplicate more of it, and as sometimes happens, the high road made for a bigger patch.

Brendan, feel free to steal review.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 505870 [details] [diff] [review]
v1 - non-minimal fix

>diff --git a/js/src/jsexn.cpp b/js/src/jsexn.cpp

>+    /* Define all error constructors. */
>+    js::Value empty = StringValue(cx->runtime->emptyString);

Should add |using namespace js;| if we don't have it already in this file, then ditch the js::.


>     for (intN i = JSEXN_ERR; i != JSEXN_LIMIT; i++) {
>-        /* Make the prototype for the current constructor name. */
>+        JSProtoKey protoKey = GetExceptionProtoKey(i);
>+        JSAtom *atom = cx->runtime->atomState.classAtoms[protoKey];
>         JSObject *proto =
>-            NewNonFunction<WithProto::Class>(cx, &js_ErrorClass, (i != JSEXN_ERR) ? error_proto : obj_proto, obj);
>+            DefineConstructorAndPrototype(cx, obj, protoKey, atom,
>+                                          (i == JSEXN_ERR) ? obj_proto : error_proto,
>+                                          &js_ErrorClass, Exception, 1,
>+                                          NULL, (i == JSEXN_ERR) ? exception_methods : NULL,
>+                                          NULL, NULL);

Change atom to errorName or something so the variable's meaning is easier to see.


>+        /* Add properties to the prototype. */
>+        JSAutoResolveFlags rf(cx, JSRESOLVE_QUALIFIED | JSRESOLVE_DECLARING);
>+        if (!js_DefineNativeProperty(cx, proto, nameId, StringValue(atom),
>+                                     NULL, NULL, JSPROP_ENUMERATE, 0, 0, NULL) ||
>+            !js_DefineNativeProperty(cx, proto, messageId, empty,
>+                                     NULL, NULL, JSPROP_ENUMERATE, 0, 0, NULL) ||
>+            !js_DefineNativeProperty(cx, proto, fileNameId, empty,
>+                                     NULL, NULL, JSPROP_ENUMERATE, 0, 0, NULL) ||
>+            !js_DefineNativeProperty(cx, proto, lineNumberId, Valueify(JSVAL_ZERO),
>+                                     NULL, NULL, JSPROP_ENUMERATE, 0, 0, NULL)) {
>             return NULL;
>         }

Use PropertyStub explicitly for these.


>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp

Noting without comment:

>+DefineConstructorAndPrototype(JSContext *cx, JSObject *obj, JSProtoKey key, JSAtom *atom,
>+                              JSObject *protoProto, Class *clasp,
                                ^^^^^^^^^^^^^^^^^^^^

>+    /*
>+     * When initializing a standard class, if no protoProto (grand-proto of
>+     * instances of the class, prototype of the class's prototype object)
>+     * is given, we must use Object.prototype if it is available.  Otherwise,
>+     * we could look up the wrong binding for a class name in obj.  Example:
>+     *
>+     *   String = Array;
>+     *   print("hi there".join);
>+     *
>+     * should print undefined, not Array.prototype.join.  This is required by
>+     * ECMA-262, alas.  It might have been better to make String readonly and
>+     * permanent in the global object, instead -- but that's too big a change
>+     * to swallow at this point.
>+     */

This comment formerly in js_InitClass -- does that even make sense any more?  It seems to me like it should just be "Prototypes for classes delegate either to a non-null protoProto or to Object.prototype."  I can't make sense of what the comment is saying.


>diff --git a/js/src/jsobj.h b/js/src/jsobj.h

>+namespace js {
>+    JSObject *
>+    DefineConstructorAndPrototype(JSContext *cx, JSObject *obj, JSProtoKey key, JSAtom *atom,
>+                                  JSObject *protoProto, Class *clasp,
>+                                  Native constructor, uintN nargs,
>+                                  JSPropertySpec *ps, JSFunctionSpec *fs,
>+                                  JSPropertySpec *static_ps, JSFunctionSpec *static_fs);
>+}

I can't remember any other declaration in a namespace indented like this, so don't indent like this.

Comment 6

[Brendan Eich [:brendan]]

grandProto?

/be

Comment 7

[Jeff Walden [:Waldo]]

|protoProto| actually feels like the right name to me, in the context of the interface -- or at least the most right name.  It just provokes a big "huh?" on first read.

I do find |protoProto| preferable to and clearer than |parent_proto|, in any case.  "parent"'s well overloaded already, and it (or "grand", I think) drags in an extra concept of ancestry that seems to me more to hinder clarity and immediate understanding more than to help.

Comment 8

[Jason Orendorff [:jorendorff]]

Thanks for the spot-on review. I changed the comment to say this:

     * All instances of the class will inherit properties from the prototype
     * object we are about to create (in DefineConstructorAndPrototype), which
     * in turn will inherit from protoProto.
     *
     * When initializing a standard class (other than Object), if protoProto is
     * null, default to the Object prototype object. The engine's internal uses
     * of js_InitClass depend on this nicety. Note that in
     * js_InitFunctionAndObjectClasses, we specially hack the resolving table
     * and then depend on js_GetClassPrototype here leaving protoProto NULL and
     * returning true.

Now that I understand this code well enough to write that, it seems unnecessarily intricate. :-) If we just changed
    if (key != JSProto_Null &&
to
    if (key > JSProto_Function &&
then we could at least do away with some of the subtle dependencies here.

Comment 9

[Brendan Eich [:brendan]]

Comment 8 has a good idea -- file?

/be

Comment 10

[Brendan Eich [:brendan]]

The comment in js_InitClass ending ", alas.  It might have been better to make String readonly and permanent in the global object, instead -- but that's too big a change to swallow at this point" has aged badly. Could lose all of that, if not more, and win.

/be

Comment 11

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

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/f727e6cd2f06
Note: not marking as fixed because fixed-in-tracemonkey is not present on the whiteboard.

Comment 12

[Christian Holler (:decoder)]

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