520890 - obj_getOwnPropertyDescriptor nests object locks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

While a property is held we call js_NewObject (not supposed to do that; it could GC) and then obj->getProperty (nesting the lock).

    JSObject *pobj;
    JSProperty *prop;
    if (!obj->lookupProperty(cx, nameidr.id(), &pobj, &prop))
        return JS_FALSE;
    MUST_FLOW_THROUGH("drop_property");

    if (!pobj->getAttributes(cx, nameidr.id(), prop, &attrs))
        goto drop_property;

    /* We have our own property, so start creating the descriptor. */
    desc = js_NewObject(cx, &js_ObjectClass, NULL, NULL);
    if (!desc)
        goto drop_property;
    *vp = OBJECT_TO_JSVAL(desc); /* Root and return. */

    if (!(attrs & (JSPROP_GETTER | JSPROP_SETTER))) {
        JSAutoTempValueRooter tvr(cx);
        if (!obj->getProperty(cx, nameidr.id(), tvr.addr()) ||
            ...

We probably shouldn't be calling a hook (like JSObjectOps::getProperty) here regardless. See bug 520882.

Here obj is either a native object, a dense array, or an XML object. I think we should special-case native objects and arrays to access the slot directly without calling a hook. For XML objects, we can throw or synthesize getter and setter functions.

Comment 1

[Jeff Walden [:Waldo]]

I'm working on a patch, and I think I have it finished actually.  However, because this error triggers no assertions in an ST shell, I'm working on adding assertions to detect this error to some extent -- and those assertions are a bit tricky given how we rely on native-property-hold/drop being equivalent to locking/unlocking the object/scope.  It's getting there -- completed patch with assertion coverage quite soon, I hope.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch

Getting the assertions done will be a long and tedious process (I've already fixed a whole lot of them in a work-in-progress patch), and this wants to be fixed rather sooner than after I finish that work.

Locking issues aside, this formulation seems better than the original anyway.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 411324 [details] [diff] [review]
Patch

Looks great.

Comment 4

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/b0fb1a71bd17

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/b0fb1a71bd17