352604 - "Assertion failure: !OBJ_GET_PROTO(cx, ctor)" after deleting Function

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

js> delete Function; new Function("");
Assertion failure: !OBJ_GET_PROTO(cx, ctor), at jsapi.c:2284

#0  0x0000a47c in JS_Assert
#1  0x0001aa8c in JS_InitClass (cx=0x600180, obj=0x1804ec0, parent_proto=0x1804f10, clasp=0x12f5cc, constructor=0x51e84 <Function>, nargs=1, ps=0x12f56c, fs=0x12f614, static_ps=0x0, static_fs=0x0) at jsapi.c:2284
...

Comment 1

[Brendan Eich [:brendan]]

The dreaded MLM assertion!

Benign in release builds, AFAICT -- Jesse, can you confirm?  If so this is a debug-build-only bug.

/be

Comment 2

[Jesse Ruderman]

Seems benign in a Firefox nightly.  Somehow it manages to construct a function!  (If I set Function to 3 instead of deleting Function, I get an error instead.)

Comment 3

[Brendan Eich [:brendan]]

"Somehow" was through quite deliberate code to resolve standard classes lazily in the shell and the browser.  See bug 46703.

The assertion is due to lazy standard class initialization interacting with the fix for bug 304376.

Of course, we would rather standard class constructors were readonly and permanent but it's too late for JS1.x to change that (we thought JS2 might force the change for all versions, but it seems at least some msn sites replace String!).

/be

Comment 4

[Jesse Ruderman]

Still happens on trunk.

Comment 5

[Brian Crowder]

Should this assertion be removed?  Is this the same as bug 352972?

Comment 6

[Brendan Eich [:brendan]]

The assertion should not be removed without a better result for what the prototype chain of the new Function should be:

js> function f(){}
js> delete Function; new Function("");
function anonymous() {
}
js> function g(){}
js> g.__proto__ === f.__proto__
false
js> g.__proto__.__proto__ === f.__proto__.__proto__
false
js> g.__proto__.__proto__.__proto__ === f.__proto__.__proto__.__proto__
false
js> g.__proto__.__proto__.__proto__                                     
[object Object]
js> g.__proto__.__proto__.__proto__.__proto__
null
js> f.__proto__
function () {
}
js> f.__proto__.__proto__
[object Object]
js> f.__proto__.__proto__.__proto__
null

(optimized js shell).

You could dup, this bug has the simpler testcase.

/be

Comment 8

[Brian Crowder]

Brendan, can you expand on what you want in comment #6?

Comment 9

[Brian Crowder]

Asked for blocking as other bugs that have blocking status have been (or should be) duped to this one.

Comment 10

[Brendan Eich [:brendan]]

Crowder: what do you think the results for the script shown in comment 6 should be? The whole issue is whether lazy standard class resolution should produce any difference in prototype chain length or object identity along it, if the engine is keeping memoized objects internally.

Of course, delete Function is legal in ES1-3 and should leave a non-lazy embedding with no ('Function' in this) at top level, so the new Function("") that follows in comment 6 should throw a "ReferenceError: Function is not defined" exception.

But since we want both lazy standard class init, and we have evidence of other bugs where somehow Function or Object is being re-resolved when the internal object has been memoized, with inconsistent prototype chain results, we should try to fix the general problem.

/be

Comment 11

[Brian Crowder]

So we need a JSPROP_DELETED for things that shouldn't be restored by lazy resolution?

Comment 12

[Brian Crowder]

Or perhaps a JSClass flag to prevent it being inited multiple times?

Comment 13

[Brendan Eich [:brendan]]

No JSPROP_DELETED. That would "fix" this bug, but really, where would you store it? You'd have to keep the property around, but not detectably "in" the object. Not worth it IMHO.

This bug seems like an opportunity to fix the lazy standard class init code to be idempotent, and that seems important in view of other bugs that botch the same assertion.

Mid-air'ed. No, not a class flag. Find out why the re-initialized function proto has a one-deeper proto chain, and report that here.

/be

Comment 14

[Brian Crowder]

This seems to be caused by js_InitFunctionAndObjectClasses, which inits both function AND object classes with new prototypes, whether they need it or not.

    /* Initialize the function class first so constructors can be made. */
    fun_proto = js_InitFunctionClass(cx, obj);
    if (!fun_proto)
        goto out;

    /* Initialize the object class next so Object.prototype works. */
    obj_proto = js_InitObjectClass(cx, obj);
    if (!obj_proto) {
        fun_proto = NULL;
        goto out;
    }

It seems as though this code should conditionally detect whether or not the class in question has been initialized before calling js_InitFunctionClass or js_InitObjectClass, OR those Init routines should check for a pre-existing proto object, and early-out returning it?

Comment 15

[Brian Crowder]

Note to self: the code just shortly following the snippet I've pasted in comment #14 is how the __proto__ linkage gets setup the way it is.  The chain doesn't grow longer than this with repeated deletes because the the global object always has Object.prototype as its proto.

Comment 16

[Brian Crowder]

Attachment: lazier object class init

A proposed solution.  Perhaps the same treatment is good for InitFunctionClass, not sure.

Comment 17

[Brian Crowder]

Attachment: removing bogus printf

Comment 18

[Brian Crowder]

This does not fix the assertion from comment 0.

However:
crowdermac:src crowder$ ./Darwin_DBG.OBJ/js
js> function f(){}
js> delete Function; new Function("");
function anonymous() {
}
js> function g(){}
js> g.__proto__ === f.__proto__
false
js> 
js> g.__proto__.__proto__ === f.__proto__.__proto__
true

Comment 20

[Brendan Eich [:brendan]]

Suggest conditioning all this on JSCLASS_IS_GLOBAL, and if that flag bit is set, calling js_GetClassObject with the appropriate JSProtoKey. If you find a cached class object (constructor), JS_DefineProperty it (the property isn't there or you would not be in resolve -- in this bug's testcase it was indeed deleted -- but the cached class object is hiding in a reserved slot in the global object).

/be

Comment 21

[Brian Crowder]

Attachment: hacky, but it works

js> function f() {}
js> delete Function; g = new Function("")
function anonymous() {
}
js> f.__proto__ === g.__proto__
true


This section of code
        obj_proto = OBJ_GET_PROTO(cx, JSVAL_TO_OBJECT(v));
        obj_proto = OBJ_GET_PROTO(cx, obj_proto);
bothers me.  Was is the Right Way to do this?

Comment 22

[Brian Crowder]

Attachment: can't have idempotence without reserved slots?

Lack of reserved slots defeats the last patch.  This is demonstrated by using evalcx("") in the shell, which will complain during function and object initialization, without the checks I've added.

Comment 23

[Brian Crowder]

Attachment: best of both worlds?

if we have reserved slots, f.__proto__ === g.__proto__ and all is right with the world:

js> f = new Function(""); delete Function; g = new Function("")
function anonymous() {
}
js> f.__proto__ === g.__proto__
true


If we don't have reserved slots, then the proto chain does not grow, but the protos don't match.  Still better than before:

js> sandbox = evalcx("")
[object sandbox]
js> sandbox.lazy = true
true
js> evalcx('f = new Function(""); delete Function; g = new Function("")', sandbox)
function anonymous() {
}
js> evalcx('f.__proto__ === g.__proto__', sandbox) // had to build a new proto
false
js> evalcx('g.__proto__.__proto__ === f.__proto__.__proto__', sandbox)
true

Victory!

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 313413 [details] [diff] [review]
best of both worlds?

>     /* Initialize the function class first so constructors can be made. */
>-    fun_proto = js_InitFunctionClass(cx, obj);
>+    if (!js_GetClassPrototype(cx, obj, INT_TO_JSID(JSProto_Function),
>+        &fun_proto) || !fun_proto) {

Nit: the overflowing actual param should be indented to underhang the first actual, so starting in the next column after the one the ( is in -- and the || would then be at end of line to put the !fun_proto on its own line.

Non-nit: if !js_GetClassPrototype(...), there was an exception thrown and still pending -- so you must fail by going to out with null fun_proto (you'll have to set it in this event, just before the goto). This nicely avoids the overlong if condition.

Non-nit: don't change js_InitFunctionClass at all, since you've forked its entire contents. Instead, put the code you added to it in the else clause below:

>+        fun_proto = js_InitFunctionClass(cx, obj, NULL);
>+    } else {
>+        fun_proto = js_InitFunctionClass(cx, obj, fun_proto);
>+    }
>     if (!fun_proto)
>         goto out;

This may have to move into the then and else clauses, maybe not.

>     /* Initialize the object class next so Object.prototype works. */
>-    obj_proto = js_InitObjectClass(cx, obj);
>+    if (!js_GetClassPrototype(cx, obj, INT_TO_JSID(JSProto_Object),
>+        &obj_proto) || !obj_proto) {

Same nit and non-nit about overflow line indentation and error/exception propagation.

>         /* Bootstrap Function.prototype (see also JS_InitStandardClasses). */
>         if (OBJ_GET_CLASS(cx, ctor) == clasp) {
>-            JS_ASSERT(!OBJ_GET_PROTO(cx, ctor));

Can we keep this and strengthen it somehow to cope in both JSCLASS_GLOBAL_FLAGS and no reserved slots cases?

>+    } else {
>+        jsval v;
>+
>+        proto = old_proto;
>+        OBJ_GET_PROPERTY(cx, proto,
>+            ATOM_TO_JSID(cx->runtime->atomState.constructorAtom), &v);

Use JS_GetConstructor instead.

>+        OBJ_DEFINE_PROPERTY(cx, obj, 
>+            ATOM_TO_JSID(cx->runtime->atomState.classAtoms[JSProto_Function]),

Use CLASS_ATOM(cx, Function) here.

/be

Comment 25

[Brendan Eich [:brendan]]

mrbkap suspects this bug's fix will fix bug 416834 too, and I suspect he's right.

/be

Comment 26

[Brian Crowder]

Attachment: wip

Brendan:  I'm not clear one what sort of assertion you'd like to replace this old one with.

Comment 27

[Brian Crowder]

Attachment: addressing brendan's comments

This patch does not save us from the testcase in bug 416834 comment 0:

js> this.__proto__.x = eval;
function eval() {
    [native code]
}
js> for (i = 0; i < 16; ++i) delete eval;
true
js> (function w() { x = 1; })();
Assertion failure: entry->kpc == ((PCVCAP_TAG(entry->vcap) > 1) ? (jsbytecode *) JSID_TO_ATOM(id) : cx->fp->regs->pc), at jsobj.c:3491

But it does improve the situation as described in comment #23

Comment 28

[Brian Crowder]

Nit-note:  Couldn't get the overflowing params to fit, but fixed the indentation.

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #25)
> mrbkap suspects this bug's fix will fix bug 416834 too, and I suspect he's
> right.

Except that this patch makes js_InitFunctionAndObjectClasses idempotent, not js_InitObjectClass, which is the culprit in bug 416834, so probably not.

Comment 30

[Brendan Eich [:brendan]]

Comment on attachment 314530 [details] [diff] [review]
addressing brendan's comments

>+    } else {
>+        JSObject *ctor;
>+
>+        ctor = JS_GetConstructor(cx, fun_proto);
>+        JS_ASSERT(ctor);

Stuff happens (bug-caused security error, e.g.), so this should really check and error if null is returned. Null return means error.

(Also we don't assert non-null, preferring a clear NPE crash -- but here we want to defend.)

>+        OBJ_DEFINE_PROPERTY(cx, obj, ATOM_TO_JSID(CLASS_ATOM(cx, Function)),
>+                            OBJECT_TO_JSVAL(ctor), 0, 0, 0, NULL);
>+    }
> 
>     /* Initialize the object class next so Object.prototype works. */
>-    obj_proto = js_InitObjectClass(cx, obj);
>+    if (!js_GetClassPrototype(cx, obj, INT_TO_JSID(JSProto_Object),
>+                              &obj_proto)) {
>+        fun_proto = NULL;
>+        obj_proto = NULL;

There's no reason to null obj_proto, since it is not used at out: and anyway, goto out instances above do not null obj_proto.

>+        goto out;
>+    }
>+    if (!obj_proto) {
>+        obj_proto = js_InitObjectClass(cx, obj);
>+    }

Nit: don't overbrace single-line then clauses whose if condition fits on one line.

>@@ -1971,27 +1971,22 @@ Function(JSContext *cx, JSObject *obj, u

Igor got this in one of his pending patches, r+me now, so you'll probably be able to cvs up and find this already committed.

/be

Comment 31

[Brian Crowder]

Attachment: addressing brendan's comments (v2)

Brendan:  I think the ctor assertion here is still righteous.  This condition should never happen, and it's one that we shouldn't silently recover from in debug builds.

Comment 32

[Brendan Eich [:brendan]]

(In reply to comment #31)
> Created an attachment (id=315570) [details]
> addressing brendan's comments (v2)
> 
> Brendan:  I think the ctor assertion here is still righteous.  This condition
> should never happen, and it's one that we shouldn't silently recover from in
> debug builds.

Who is "we"? Assertions should not botch. If they do, the person they bite should be working to remove the botch or add runtime code to cope. If you have runtime code to cope then you don't need an assertion. If you want a warning, then do not use an assertion (which crashes the program when it botches) or you will hurt third parties.

Why do you want a warning here in DEBUG builds?

/be

Comment 33

[Brendan Eich [:brendan]]

"Assertions should not botch" means "never". Given the fallible JS_GetConstructor API, we know failure is an option. So we should not assert what might be false.

/be

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 315570 [details] [diff] [review]
addressing brendan's comments (v2)

r=me with that assertion removed.

/be

Comment 35

[Brian Crowder]

Attachment: no assert-botch [Checkin: Comment 37]

This is the one to land, hoping for approval quickly.

Comment 36

[Mike Shaver (:shaver emeritus)]

Comment on attachment 315822 [details] [diff] [review]
no assert-botch
[Checkin: Comment 37]

a=shaver, let's get those shell tests in the tree too.

Comment 37

[Brian Crowder]

jsapi.c: 3.444

bclary: your help would be much appreciated in taking some of these proto and sandboxed proto tests from below, and testifying them.  If you haven't got the time, let me know.

Comment 38

[Bob Clary [:bc] (inactive)]

crowder: One is glad to be of service.

Comment 39

[Bob Clary [:bc] (inactive)]

RCS file: /cvsroot/mozilla/js/tests/js1_5/Regress/regress-352604.js,v

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-352604.js,v  <--  regress-352604.js
initial revision: 1.1

Checking in js1_5/extensions/regress-352604.js;
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-352604.js,v  <--  regress-352604.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/public-failures.txt,v  <--  public-failures.txt
new revision: 1.64; previous revision: 1.63

Comment 40

[Bob Clary [:bc] (inactive)]

v 1.9.0

Comment 41

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

As per discussion with Jesse, this bug should be ported over to the 1.8 branch to fix some of the issues jsfunfuzz is hitting.

I should have a backported patch in the next day(s) or so.

Comment 42

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

Attachment: patch for MOZILLA_1_8_BRANCH

This is the backported patch to MOZILLA_1_8_BRANCH.

Comment 43

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

Attachment: diff between trunk and 1.8 patches

Comment 44

[Brian Crowder]

Comment on attachment 335861 [details] [diff] [review]
patch for MOZILLA_1_8_BRANCH

[STAMP]ed with the assumption that it passes js/tests and Mochitests

Comment 45

[Daniel Veditz [:dveditz]]

Comment on attachment 335861 [details] [diff] [review]
patch for MOZILLA_1_8_BRANCH

Approved for 1.8.1.18, a=dveditz for release-drivers

Comment 46

[Brian Crowder]

The backported patch here needs to be landed, help is appreciated.

Comment 47

[Brian Crowder]

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.214.2.44; previous revision: 3.214.2.43
done

Comment 48

[Bob Clary [:bc] (inactive)]

assertion verified fixed 1.8.1.18.
js1_5/extensions/regress-352604.js
Expected value 'function f() {NL}', Actual value 'function () {NL}'
I'll add that to the known 1.8.1 failures.