watch("e", (function () { evalcx('') })) e = function () {} asserts js debug shell on JM changeset 6347cf00d3ab with -m at Assertion failure: !entry->key.obj && entry->flags == 0, at ../jsapi.cpp:1233

This is an interpreter bug; it crashes without -m.

Verified failing against TM tip with same assert.

Thanks Sean, you're right. autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: 44269:3aaaa21012c8 user: Jason Orendorff date: Wed Jun 23 16:35:10 2010 -0500 summary: Bug 563099 - Compartments and wrappers API. r=gal.

If I understand this code correctly, js_InitFunctionAndObjectClasses assumes that if cx->resolvingTable is non-empty, we must be resolving either Function or Object. But obj_watch_handler abuses resolvingTable to prevent watchpoints from recursing. This causes cx->resolvingTable to be nonempty when we call evalcx -> JS_InitStandardClasses -> js_InitFunctionAndObjectClasses where the above assumption is now false. The new implementation of evalcx exposes this bug because it uses a single context (hence a single cx->resolvingTable) where before, each sandbox had its own context.

It would be so nice if this code read like, op = createObjectPrototype() store op in global reserved slot fp = createFunctionPrototype() store fp in global reserved slot o = createObjectConstructor() store o in global reserved slot f = createFunctionConstructor() store f in global reserved slot define methods of op define methods of fp define methods of o define methods of f define global.Object = o define global.Function = f (On error, just null out the four reserved slots and return false.) I'm thinking that's not on my todo list for today though.

Yeah, comment 5 is nicer. This old code pre-dates any reserved slots in globals for original values of Foo.prototype, but over a decade. /be

If I'm reading it correctly, the old code ignores the possibility that both obj.Function and obj.Object are in cx->resolvingTable already. Is the intent that it can't happen -- we never reenter js_InitFunctionAndObjectClasses? Anyway I tried not to change the behavior at all except to stop using cx->resolvingTable->entryCount as a bogus proxy for "are we resolving obj.Function or obj.Object right now?". (The new AutoResolvingEntry could of course be used all over. It could stash resolvingTable->generation, too. Later.)

Comment on attachment 464402 [details] [diff] [review] v1 Retracting for the moment. A couple of tests crashed.

Comment on attachment 464402 [details] [diff] [review] v1 It was just bug 585686. Sorry for the noise.

Comment on attachment 464402 [details] [diff] [review] v1 >diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp >--- a/js/src/jsapi.cpp >+++ b/js/src/jsapi.cpp >@@ -1192,116 +1192,95 @@ JS_PUBLIC_API(void) > JS_SetGlobalObject(JSContext *cx, JSObject *obj) > { > CHECK_REQUEST(cx); > > cx->globalObject = obj; > cx->compartment = obj ? obj->getCompartment(cx) : cx->runtime->defaultCompartment; > } > >+class AutoResolvingEntry { >+public: >+ AutoResolvingEntry() : entry(NULL) {} >+ >+ /* >+ * Returns false on error. But N.B. if obj[id] was already being resolved, >+ * this is a no-op, and we silently treat that as success. >+ */ >+ JSBool start(JSContext *cx, JSObject *obj, jsid id, uint32 flag) { Could use bool here? A !! might be required, or go the distance and bool-ify js_StartResolving too. >+ JS_ASSERT(!entry); >+ this->cx = cx; >+ key.obj = obj; >+ key.id = id; >+ this->flag = flag; >+ JSBool ok = js_StartResolving(cx, &key, flag, &entry); >+ if (!ok) >+ entry = NULL; Just JS_ASSERT(!ok, !entry) here, js_StarResolving sets *entryp only if it is about to return true. r=me with that, thanks. /be

(In reply to comment #5) > It would be so nice if this code read like, I filed bug 586053 but maybe it should morph to do what comment 5 says. Or if there is a simple spot-fix, we need a new bug for comment 5. /be

(In reply to comment #10) > >+ JSBool start(JSContext *cx, JSObject *obj, jsid id, uint32 flag) { > > Could use bool here? A !! might be required, or go the distance and bool-ify > js_StartResolving too. I changed this, but I've been using JSBool, in combination with a cx argument, to indicate "this return value indicates success or failure with an error reported or exception thrown (not a predicate)". If other people aren't doing that too I should drop it. I'll ask around.

Lots of code uses bool foo(JSContext *cx, ...) for fallible foo -- it's the right thing, long term. Any pure predicate should not take a cx, or try to put it in the final parameter position, to convey infallibility. /be

This was checked in to TM: http://hg.mozilla.org/tracemonkey/rev/087f2e1d4a9a

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

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