Closed Bug 584648 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: gkw, Assigned: jorendorff)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [compartments] fixed-in-tracemonkey)

Attachments

(1 file)

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.
Summary: JM: "Assertion failure: !entry->key.obj && entry->flags == 0," → TM: "Assertion failure: !entry->key.obj && entry->flags == 0,"
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.
blocking2.0: --- → ?
Assignee: general → jorendorff
blocking2.0: ? → beta5+
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
Attached patch v1Splinter Review
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.)
Attachment #464402 - Flags: review?(brendan)
Comment on attachment 464402 [details] [diff] [review]
v1

Retracting for the moment. A couple of tests crashed.
Attachment #464402 - Flags: review?(brendan)
Comment on attachment 464402 [details] [diff] [review]
v1

It was just bug 585686. Sorry for the noise.
Attachment #464402 - Flags: review?(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
Attachment #464402 - Flags: review?(brendan) → review+
(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
Whiteboard: [compartments]
(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
Whiteboard: [compartments] → [compartments] fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/087f2e1d4a9a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-584648.js.
Flags: in-testsuite+
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.