Last Comment Bug 352604 - "Assertion failure: !OBJ_GET_PROTO(cx, ctor)" after deleting Function
: "Assertion failure: !OBJ_GET_PROTO(cx, ctor)" after deleting Function
Status: VERIFIED FIXED
: crash, testcase, verified1.8.1.18
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: PowerPC Mac OS X
: P1 critical (vote)
: mozilla1.9
Assigned To: Brian Crowder
:
Mentors:
: 352972 425104 (view as bug list)
Depends on: 633741
Blocks: jsfunfuzz 416834
  Show dependency treegraph
 
Reported: 2006-09-13 19:11 PDT by Jesse Ruderman
Modified: 2011-02-12 16:08 PST (History)
11 users (show)
vladimir: blocking1.9+
dveditz: blocking1.8.1.18+
dveditz: wanted1.8.1.x+
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
lazier object class init (1.42 KB, patch)
2008-04-01 16:53 PDT, Brian Crowder
no flags Details | Diff | Review
removing bogus printf (942 bytes, patch)
2008-04-01 16:55 PDT, Brian Crowder
no flags Details | Diff | Review
hacky, but it works (5.27 KB, patch)
2008-04-02 15:35 PDT, Brian Crowder
no flags Details | Diff | Review
can't have idempotence without reserved slots? (5.42 KB, patch)
2008-04-03 11:36 PDT, Brian Crowder
no flags Details | Diff | Review
best of both worlds? (5.42 KB, patch)
2008-04-03 12:50 PDT, Brian Crowder
no flags Details | Diff | Review
wip (3.75 KB, patch)
2008-04-08 23:49 PDT, Brian Crowder
no flags Details | Diff | Review
addressing brendan's comments (3.74 KB, patch)
2008-04-08 23:56 PDT, Brian Crowder
no flags Details | Diff | Review
addressing brendan's comments (v2) (2.61 KB, patch)
2008-04-14 09:24 PDT, Brian Crowder
brendan: review+
Details | Diff | Review
no assert-botch [Checkin: Comment 37] (2.58 KB, patch)
2008-04-15 13:05 PDT, Brian Crowder
crowderbt: review+
shaver: approval1.9+
Details | Diff | Review
patch for MOZILLA_1_8_BRANCH (2.60 KB, patch)
2008-08-28 03:43 PDT, Gary Kwong [:gkw] [:nth10sd]
crowderbt: review+
dveditz: approval1.8.1.18+
Details | Diff | Review
diff between trunk and 1.8 patches (600 bytes, text/plain)
2008-08-28 03:44 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details

Description Jesse Ruderman 2006-09-13 19:11:23 PDT
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] 2006-09-13 19:18:46 PDT
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 2006-09-13 19:34:33 PDT
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] 2006-09-13 20:05:08 PDT
"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 2007-09-18 17:00:39 PDT
Still happens on trunk.
Comment 5 Brian Crowder 2008-03-12 09:53:52 PDT
Should this assertion be removed?  Is this the same as bug 352972?
Comment 6 Brendan Eich [:brendan] 2008-03-12 11:13:54 PDT
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 7 Brian Crowder 2008-03-20 10:19:10 PDT
*** Bug 352972 has been marked as a duplicate of this bug. ***
Comment 8 Brian Crowder 2008-04-01 15:05:00 PDT
Brendan, can you expand on what you want in comment #6?
Comment 9 Brian Crowder 2008-04-01 15:05:30 PDT
Asked for blocking as other bugs that have blocking status have been (or should be) duped to this one.
Comment 10 Brendan Eich [:brendan] 2008-04-01 15:19:03 PDT
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 2008-04-01 15:21:25 PDT
So we need a JSPROP_DELETED for things that shouldn't be restored by lazy resolution?
Comment 12 Brian Crowder 2008-04-01 15:31:52 PDT
Or perhaps a JSClass flag to prevent it being inited multiple times?
Comment 13 Brendan Eich [:brendan] 2008-04-01 15:34:26 PDT
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 2008-04-01 16:45:55 PDT
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 2008-04-01 16:47:40 PDT
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 2008-04-01 16:53:33 PDT
Created attachment 312997 [details] [diff] [review]
lazier object class init

A proposed solution.  Perhaps the same treatment is good for InitFunctionClass, not sure.
Comment 17 Brian Crowder 2008-04-01 16:55:02 PDT
Created attachment 312998 [details] [diff] [review]
removing bogus printf
Comment 18 Brian Crowder 2008-04-01 16:58:08 PDT
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 19 Brian Crowder 2008-04-02 10:18:06 PDT
*** Bug 425104 has been marked as a duplicate of this bug. ***
Comment 20 Brendan Eich [:brendan] 2008-04-02 10:26:27 PDT
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 2008-04-02 15:35:52 PDT
Created attachment 313213 [details] [diff] [review]
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 2008-04-03 11:36:17 PDT
Created attachment 313394 [details] [diff] [review]
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 2008-04-03 12:50:29 PDT
Created attachment 313413 [details] [diff] [review]
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] 2008-04-07 19:00:22 PDT
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] 2008-04-08 23:09:22 PDT
mrbkap suspects this bug's fix will fix bug 416834 too, and I suspect he's right.

/be
Comment 26 Brian Crowder 2008-04-08 23:49:31 PDT
Created attachment 314526 [details] [diff] [review]
wip

Brendan:  I'm not clear one what sort of assertion you'd like to replace this old one with.
Comment 27 Brian Crowder 2008-04-08 23:56:06 PDT
Created attachment 314530 [details] [diff] [review]
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 2008-04-08 23:57:00 PDT
Nit-note:  Couldn't get the overflowing params to fit, but fixed the indentation.
Comment 29 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-04-09 09:10:22 PDT
(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] 2008-04-12 23:02:49 PDT
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 2008-04-14 09:24:50 PDT
Created attachment 315570 [details] [diff] [review]
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] 2008-04-15 12:28:43 PDT
(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] 2008-04-15 12:29:39 PDT
"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] 2008-04-15 12:30:11 PDT
Comment on attachment 315570 [details] [diff] [review]
addressing brendan's comments (v2)

r=me with that assertion removed.

/be
Comment 35 Brian Crowder 2008-04-15 13:05:46 PDT
Created attachment 315822 [details] [diff] [review]
no assert-botch
[Checkin: Comment 37]

This is the one to land, hoping for approval quickly.
Comment 36 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-04-15 13:07:35 PDT
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 2008-04-15 13:45:21 PDT
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:] 2008-04-16 01:21:27 PDT
crowder: One is glad to be of service.
Comment 39 Bob Clary [:bc:] 2008-04-16 07:40:14 PDT
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:] 2008-04-30 10:16:01 PDT
v 1.9.0
Comment 41 Gary Kwong [:gkw] [:nth10sd] 2008-08-26 21:54:47 PDT
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] 2008-08-28 03:43:08 PDT
Created attachment 335861 [details] [diff] [review]
patch for MOZILLA_1_8_BRANCH

This is the backported patch to MOZILLA_1_8_BRANCH.
Comment 43 Gary Kwong [:gkw] [:nth10sd] 2008-08-28 03:44:51 PDT
Created attachment 335863 [details]
diff between trunk and 1.8 patches
Comment 44 Brian Crowder 2008-08-28 09:58:52 PDT
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] 2008-09-22 11:32:43 PDT
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 2008-09-30 14:16:30 PDT
The backported patch here needs to be landed, help is appreciated.
Comment 47 Brian Crowder 2008-10-20 08:40:09 PDT
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:] 2008-10-23 12:40:47 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.