584648 - TM: "Assertion failure: !entry->key.obj && entry->flags == 0,"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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

Comment 1

[Sean Stangl [:sstangl]]

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

Comment 2

[Sean Stangl [:sstangl]]

Verified failing against TM tip with same assert.

Comment 3

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

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.

Comment 4

[Jason Orendorff [:jorendorff]]

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.

Comment 5

[Jason Orendorff [:jorendorff]]

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.

Comment 6

[Brendan Eich [:brendan]]

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

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v1

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 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 464402 [details] [diff] [review]
v1

Retracting for the moment. A couple of tests crashed.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 464402 [details] [diff] [review]
v1

It was just bug 585686. Sorry for the noise.

Comment 10

[Brendan Eich [:brendan]]

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

Comment 11

[Brendan Eich [:brendan]]

(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

Comment 12

[Jason Orendorff [:jorendorff]]

(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.

Comment 13

[Brendan Eich [:brendan]]

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

Comment 14

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

This was checked in to TM:

http://hg.mozilla.org/tracemonkey/rev/087f2e1d4a9a

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/087f2e1d4a9a

Comment 16

[Christian Holler (:decoder)]

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

Comment 17

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

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