351717 - Missing object/scope UNLOCK macro call todo with JSCLASS_GLOBAL_FLAGS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Mike Moening]

The function js_DefineNativeProperty doesn't always call JS_UNLOCK_OBJ when exiting the function.  See below:

--------
   PROPERTY_CACHE_FILL(&cx->runtime->propertyCache, obj, id, sprop);
    if (propp)
        *propp = (JSProperty *) sprop;
    else
        JS_UNLOCK_OBJ(cx, obj);
    return JS_TRUE;

bad:
    JS_UNLOCK_OBJ(cx, obj);
    return JS_FALSE;
}
--------------

Commenting out the else before JS_UNLOCK_OBJ(cx, obj);
fixes the JS_ASSERTS I'm seeing in js_lockScope:

JS_ASSERT(scope->u.count == 0);

Also take a look at the following functions which ***may*** have a mismatched pair of LOCK/UNLOCK as well:

js_CallIteratorNext()
js_SetProtoOrParent()
xml_lookupProperty()

Comment 1

[Brendan Eich [:brendan]]

Please see my further private mail in reply to yours.  These are all false alarms.

You have found something real, which has a clear workaround pointing to the true and alarming bug: removing JSCLASS_GLOBAL_FLAGS avoids the stuck u.count reentry counter.

Start bugs out by citing symptoms and facts such as that JSCLASS_GLOBAL_FLAGS workaround.  That avoids jumping to the wrong conclusion.  Morphing summary now.

/be

Comment 2

[Mike Moening]

This bug is hidden and very tricky.
I think I might be on to something...

js_LookupPropertyWithFlags() has a return which doesn't UNLOCK.
See code below:
--------------------------
        if (sprop) {
            JS_ASSERT(OBJ_SCOPE(obj) == scope);
            *objp = scope->object;      /* XXXbe hide in jsscope.[ch] */

            *propp = (JSProperty *) sprop;
            return JS_TRUE;
        }
--------------------------

This method is called from js_CheckRedeclaration() which calls OBJ_LOOKUP_PROPERTY().

js_CheckRedeclaration() utimately returns here:
------------------
	/*
         * Allow redeclaration of variables and functions, but insist that the
         * new value is not a getter if the old value was, ditto for setters --
         * unless prop is impermanent (in which case anyone could delete it and
         * redefine it, willy-nilly).
         */
         if (!(attrs & (JSPROP_GETTER | JSPROP_SETTER)))
            return JS_TRUE;
----------------
without unlocking via OBJ_DROP_PROPERTY or any other mechanism.


Just after the return the lock counter is bumped again by a call to OBJ_DEFINE_PROPERTY see below:
That leaves us with a count 1 higher than it should be.
------------------
 /*
             * Check for a const property of the same name -- or any kind
             * of property if executing with the strict option.  We check
             * here at runtime as well as at compile-time, to handle eval
             * as well as multiple HTML script tags.
             */
            parent = fp->varobj;
            SAVE_SP_AND_PC(fp);
            ok = js_CheckRedeclaration(cx, parent, id, attrs, NULL, NULL);
            if (ok) {
                ok = OBJ_DEFINE_PROPERTY(cx, parent, id, rval,
                                         (flags & JSPROP_GETTER)
                                         ? JS_EXTENSION (JSPropertyOp) obj
                                         : NULL,
                                         (flags & JSPROP_SETTER)
                                         ? JS_EXTENSION (JSPropertyOp) obj
                                         : NULL,
                                         attrs,
                                         &prop);
            }
-----------------

Comment 3

[Brendan Eich [:brendan]]

MikeM shoots, he scores!  Thanks, patch soon.

Must have regressed since JS1.[56] -- can you confirm from sources?

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix

Igor feel free to comment.  I'm a bit fried from other patches, want suggestions on minimizing control flow and out param setting/nulling in js_CheckRedeclarations (but this patch should fix the bug -- MikeM please test and confirm).

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 237211 [details] [diff] [review]
fix

Yeah, this seems like the best way to go.

Comment 6

[Brendan Eich [:brendan]]

Fixed on trunk.  MikeM, thanks for finding this -- it couldn't have been easy!

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 237211 [details] [diff] [review]
fix

This is an important fix for embedders such as MikeM, and the bug could bite Firefox with extensions such as Chatzilla (IIRC) or others that use JS and threads (nsIThread).

/be

Comment 8

[Mike Moening]

>I'm a bit fried from other patches, want
>suggestions on minimizing control flow and out param setting/nulling in
>js_CheckRedeclarations (but this patch should fix the bug -- MikeM please test
>and confirm).

Your fix works great!  
With this fix and a forthcoming patch to bug #351602, JS 1.7 appears to finally be stable now for multi-threaded embedders.

Comment 9

[Mike Schroepfer]

Comment on attachment 237211 [details] [diff] [review]
fix

a=schrep for drivers.

Comment 10

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be