423342 - "Assertion failure: JSVAL_IS_VOID(obj->fslots[JSSLOT_ARRAY_LOOKUP_HOLDER])"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (triggers a fatal assertion when loaded)

Loading the testcase in a Mac trunk debug build of Firefox triggers:

Assertion failure: STOBJ_GET_SLOT(obj, JSSLOT_ARRAY_LOOKUP_HOLDER) == JSVAL_VOID, at /Users/jruderman/trunk/mozilla/js/src/jsarray.c:669

Seems harmless in a nightly.

This assertion is part of array_lookupProperty, which was added as part of bug 322889.

Comment 1

[Mike Shaver (:shaver emeritus)]

This means that we have a lookup being performed on a given array object while a previous one is "outstanding" (lookup has returned, but the resulting property hasn't been dropped).  It's probably clear with a stack, I'll take a look from the office.

Might want to block on this -- not clear what the effects are on the engine of violating this invariant (though I suspect them to be minor, setting up the right switcharoo here could affect aspects of property lookup that we might depend on for security).

Comment 2

[Mike Schroepfer]

--> Shaver then..

Comment 3

[Damon Sicore (:damons)]

How we doing here, Shaver?

Comment 4

[Mike Shaver (:shaver emeritus)]

I've got it in the debugger, but don't have symbols for JS for some reason, so have to rebuild -- that I'm failing in cairo makes me think I didn't have a good build anyway, so probably for the best.

Comment 5

[Mike Shaver (:shaver emeritus)]

This is because MarkSharpObjects can nest a get inside a lookup/drop, here:

            ok = OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop);
            if (!ok)
                break;
            if (!prop)
                continue;
            ok = OBJ_GET_ATTRIBUTES(cx, obj2, id, prop, &attrs);
            if (ok) {
                if (OBJ_IS_NATIVE(obj2) &&
                    (attrs & (JSPROP_GETTER | JSPROP_SETTER))) {
                    val = JSVAL_NULL;
                    if (attrs & JSPROP_GETTER)
                        val = (jsval) ((JSScopeProperty*)prop)->getter;
                    if (attrs & JSPROP_SETTER) {
                        if (val != JSVAL_NULL) {
                            /* Mark the getter, then set val to setter. */
                            ok = (MarkSharpObjects(cx, JSVAL_TO_OBJECT(val),
                                                   NULL)
                                  != NULL);
                        }
                        val = (jsval) ((JSScopeProperty*)prop)->setter;
                    }
                } else {
-->                 ok = OBJ_GET_PROPERTY(cx, obj, id, &val);
                }
            }
            OBJ_DROP_PROPERTY(cx, obj2, prop);
#else
            ok = OBJ_GET_PROPERTY(cx, obj, id, &val);
#endif
            if (!ok)
                break;


I could refactor that to drop before the GET, but I wonder if the property tree doesn't provide sufficient guarantee that the sprop won't go away even if I drop right after the OBJ_GET_ATTRIBUTES?  Pretty tired now, but I'll try it tonight if I get a chance.

Comments on that approach v. welcome!

Comment 6

[Mike Shaver (:shaver emeritus)]

De-blocking -- analysis of this case indicates that it's harmless (id is the same, and prop isn't reused after the second drop underneath OBJ_GET_PROPERTY except to drop it).

We have a handful of choices:

- move OBJ_DROP_PROPERTY up immediately after OBJ_GET_ATTRIBUTES, which could cause a dangling access in the perfect storm of MarkSharpObjects(getter) causing a last-ditch GC (which would require a callable host object with enumeration causing GCThing allocation, I believe) after losing a race with a delete on another thread

- move the OBJ_GET_ATTRIBUTES/OBJ_DROP_PROPERTY underneath the OBJ_IS_NATIVE test, since we know that native objects can handle nested lookup.

(- make arrays handle recursive lookup, at some performance cost)

(- wait until we redesign object ops and see if we still have this allocation-heavy pattern at all)

I have a patch for moving DROP_PROPERTY up, but I now think I prefer putting things under the OBJ_IS_NATIVE test, if not waiting for the objectops refactor.  Jesse: how much does this obstruct your testing?

Comment 7

[Jesse Ruderman]

This does not obstruct my testing much.  I don't think I've hit it again since filing this bug.

Comment 8

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

The testcase in comment #0 now hits:

Assertion failure: JSVAL_IS_VOID(obj->fslots[JSSLOT_ARRAY_LOOKUP_HOLDER]), at /Users/skywalker/Desktop/dom-debug-fx-trunk-A/fx-trunk-hg-debug/js/src/jsarray.cpp:666

instead on Mac trunk Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080818150509 Minefield/3.1a2pre with mallocscribble. Should the summary be changed to reflect this?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

It doesn't really matter. If it makes finding this bug easier for you, then go ahead.

Comment 10

[Brendan Eich [:brendan]]

Fixed by fix for bug 487930.

/be