Closed Bug 352604 Opened 18 years ago Closed 16 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

PowerPC
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: jruderman, Assigned: crowderbt)

References

Details

(Keywords: crash, testcase, verified1.8.1.18)

Attachments

(3 files, 8 obsolete files)

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
...
The dreaded MLM assertion!

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

/be
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.)
"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
Still happens on trunk.
Should this assertion be removed?  Is this the same as bug 352972?
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
Brendan, can you expand on what you want in comment #6?
Flags: blocking1.9?
Asked for blocking as other bugs that have blocking status have been (or should be) duped to this one.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
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
So we need a JSPROP_DELETED for things that shouldn't be restored by lazy resolution?
Or perhaps a JSClass flag to prevent it being inited multiple times?
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
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?
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.
Attached patch lazier object class init (obsolete) — Splinter Review
A proposed solution.  Perhaps the same treatment is good for InitFunctionClass, not sure.
Assignee: general → crowder
Status: NEW → ASSIGNED
Attached patch removing bogus printf (obsolete) — Splinter Review
Attachment #312997 - Attachment is obsolete: true
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
Attachment #312998 - Flags: review?(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
Attached patch hacky, but it works (obsolete) — Splinter Review
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?
Attachment #312998 - Attachment is obsolete: true
Attachment #313213 - Flags: review?(brendan)
Attachment #312998 - Flags: review?(brendan)
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.
Attachment #313213 - Attachment is obsolete: true
Attachment #313394 - Flags: review?(brendan)
Attachment #313213 - Flags: review?(brendan)
Attached patch best of both worlds? (obsolete) — Splinter Review
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!
Attachment #313394 - Attachment is obsolete: true
Attachment #313413 - Flags: review?(brendan)
Attachment #313394 - Flags: review?(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
mrbkap suspects this bug's fix will fix bug 416834 too, and I suspect he's right.

/be
Blocks: 416834
Attached patch wip (obsolete) — Splinter Review
Brendan:  I'm not clear one what sort of assertion you'd like to replace this old one with.
Attachment #313413 - Attachment is obsolete: true
Attachment #313413 - Flags: review?(brendan)
Attached patch addressing brendan's comments (obsolete) — Splinter Review
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
Attachment #314526 - Attachment is obsolete: true
Attachment #314530 - Flags: review?(brendan)
Nit-note:  Couldn't get the overflowing params to fit, but fixed the indentation.
(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 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
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.
Attachment #314530 - Attachment is obsolete: true
Attachment #315570 - Flags: review?(brendan)
Attachment #314530 - Flags: review?(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
"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 on attachment 315570 [details] [diff] [review]
addressing brendan's comments (v2)

r=me with that assertion removed.

/be
Attachment #315570 - Flags: review?(brendan) → review+
This is the one to land, hoping for approval quickly.
Attachment #315570 - Attachment is obsolete: true
Attachment #315822 - Flags: review+
Attachment #315822 - Flags: approval1.9?
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.
Attachment #315822 - Flags: approval1.9? → approval1.9+
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
crowder: One is glad to be of service.
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
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-
v 1.9.0
Status: RESOLVED → VERIFIED
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.
Flags: wanted1.8.1.x?
This is the backported patch to MOZILLA_1_8_BRANCH.
Attachment #335861 - Flags: review?(crowder)
Attachment #335863 - Attachment mime type: application/octet-stream → text/plain
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
Attachment #335861 - Flags: review?(crowder) → review+
Comment on attachment 335861 [details] [diff] [review]
patch for MOZILLA_1_8_BRANCH

Approved for 1.8.1.18, a=dveditz for release-drivers
Attachment #335861 - Flags: approval1.8.1.18? → approval1.8.1.18+
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.18+
The backported patch here needs to be landed, help is appreciated.
Keywords: checkin-needed
Attachment #315822 - Attachment description: no assert-botch → no assert-botch [Checkin: Comment 37]
Whiteboard: [c-n: 1.8.1 branch]
Target Milestone: --- → mozilla1.9
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
Whiteboard: [c-n: 1.8.1 branch]
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.
Depends on: 633741
You need to log in before you can comment on or make changes to this bug.