Unified resolve recursion damping (70358) and resolve-from-js_SetProperty (72354), r/sr=jband,jst,shaver.

js/src/jsapi.c

@@ -1031,7 +1031,7 @@ InitFunctionAndObjectClasses(JSContext *cx, JSObject *obj)
 {
     JSDHashTable *table;
     JSRuntime *rt;
-    JSString *idstr;
+    JSResolvingKey key;
     JSDHashEntryHdr *entry;
     JSObject *fun_proto, *obj_proto;
 
@@ -1043,17 +1043,18 @@ InitFunctionAndObjectClasses(JSContext *cx, JSObject *obj)
     table = cx->resolving;
     if (table) {
         rt = cx->runtime;
-        idstr = ATOM_TO_STRING(rt->atomState.FunctionAtom);
+        key.obj = obj;
-        entry = JS_DHashTableOperate(table, idstr, JS_DHASH_LOOKUP);
+        key.id = (jsid) rt->atomState.FunctionAtom;
+        entry = JS_DHashTableOperate(table, &key, JS_DHASH_LOOKUP);
         if (JS_DHASH_ENTRY_IS_BUSY(entry))
-            idstr = ATOM_TO_STRING(rt->atomState.ObjectAtom);
+            key.id = (jsid) rt->atomState.ObjectAtom;
 
-        entry = JS_DHashTableOperate(table, idstr, JS_DHASH_ADD);
+        entry = JS_DHashTableOperate(table, &key, JS_DHASH_ADD);
         if (!entry) {
             JS_ReportOutOfMemory(cx);
             return NULL;
         }
-        ((JSDHashEntryStub *)entry)->key = idstr;
+        ((JSResolvingEntry *)entry)->key = key;
     }
 
     /* Initialize the function class first so constructors can be made. */
@@ -1253,13 +1254,10 @@ JS_ResolveStandardClass(JSContext *cx, JSObject *obj, jsval id,
                         JSBool *resolved)
 {
     JSString *idstr;
-    JSDHashTable *table;
-    JSDHashEntryHdr *entry;
     JSRuntime *rt;
     JSAtom *atom;
     JSObjectOp init;
     uintN i;
-    JSBool ok;
 
     CHECK_REQUEST(cx);
     *resolved = JS_FALSE;
@@ -1267,12 +1265,6 @@ JS_ResolveStandardClass(JSContext *cx, JSObject *obj, jsval id,
     if (!JSVAL_IS_STRING(id))
         return JS_TRUE;
     idstr = JSVAL_TO_STRING(id);
-    table = cx->resolving;
-    if (table) {
-        entry = JS_DHashTableOperate(table, idstr, JS_DHASH_LOOKUP);
-        if (JS_DHASH_ENTRY_IS_BUSY(entry))
-            return JS_TRUE;
-    }
     rt = cx->runtime;
 
 #if JS_HAS_UNDEFINED
@@ -1321,39 +1313,12 @@ JS_ResolveStandardClass(JSContext *cx, JSObject *obj, jsval id,
         }
     }
 
-    if (!init) {
+    if (init) {
-        ok = JS_TRUE;
+        if (!init(cx, obj))
-    } else {
-        if (!table) {
-            table = JS_NewDHashTable(JS_DHashGetStubOps(),
-                                     NULL,
-                                     sizeof(JSDHashEntryStub),
-                                     JS_DHASH_MIN_SIZE);
-            if (!table)
-                goto outofmem;
-            cx->resolving = table;
-        }
-        entry = JS_DHashTableOperate(table, idstr, JS_DHASH_ADD);
-        if (!entry)
-            goto outofmem;
-        ((JSDHashEntryStub *)entry)->key = idstr;
-
-        if (init(cx, obj))
-            ok = *resolved = JS_TRUE;
-        else
-            ok = JS_FALSE;
-
-        JS_DHashTableRawRemove(table, entry);
-        if (table->entryCount == 0) {
-            JS_DHashTableDestroy(table);
-            cx->resolving = NULL;
-        }
-    }
-    return ok;
-
-outofmem:
-    JS_ReportOutOfMemory(cx);
             return JS_FALSE;
+        *resolved = JS_TRUE;
+    }
+    return JS_TRUE;
 }
 
 static JSBool

js/src/jscntxt.h

@@ -240,6 +240,22 @@ struct JSStackHeader {
 
 #define JS_STACK_SEGMENT(sh)    ((jsval *)(sh) + 2)
 
+/*
+ * Key and entry types for the JSContext.resolving hash table, typedef'd here
+ * because all consumers need to see these declarations (not just the typedef
+ * names, as would be the case for a pointer-to-typedef'd-type declaration),
+ * along with cx->resolving.
+ */
+typedef struct JSResolvingKey {
+    JSObject            *obj;
+    jsid                id;
+} JSResolvingKey;
+
+typedef struct JSResolvingEntry {
+    JSDHashEntryHdr     hdr;
+    JSResolvingKey      key;
+} JSResolvingEntry;
+
 struct JSContext {
     JSCList             links;
 

js/src/jshash.c

@@ -139,14 +139,15 @@ JS_PUBLIC_API(void)
 JS_HashTableDestroy(JSHashTable *ht)
 {
     uint32 i, n;
-    JSHashEntry *he, *next;
+    JSHashEntry *he, **hep;
     JSHashAllocOps *allocOps = ht->allocOps;
     void *allocPriv = ht->allocPriv;
 
     n = NBUCKETS(ht);
     for (i = 0; i < n; i++) {
-        for (he = ht->buckets[i]; he; he = next) {
+        hep = &ht->buckets[i];
-            next = he->next;
+        while ((he = *hep) != NULL) {
+            *hep = he->next;
             (*allocOps->freeEntry)(allocPriv, he, HT_FREE_ENTRY);
         }
     }

js/src/jsobj.c

@@ -42,6 +42,7 @@
 #include "jsarena.h" /* Added by JSIFY */
 #include "jsutil.h" /* Added by JSIFY */
 #include "jshash.h" /* Added by JSIFY */
+#include "jsdhash.h"
 #include "jsprf.h"
 #include "jsapi.h"
 #include "jsatom.h"
@@ -1981,7 +1982,7 @@ js_DefineProperty(JSContext *cx, JSObject *obj, jsid id, jsval value,
         LOCKED_OBJ_SET_SLOT(obj, sprop->slot, value);
     if (propp) {
 #ifdef JS_THREADSAFE
-        js_HoldScopeProperty(cx, scope, sprop);
+        HOLD_SCOPE_PROPERTY(scope, sprop);
 #endif
         *propp = (JSProperty *) sprop;
     } else {
@@ -1994,6 +1995,44 @@ bad:
     return JS_FALSE;
 }
 
+JS_STATIC_DLL_CALLBACK(const void *)
+resolving_GetKey(JSDHashTable *table, JSDHashEntryHdr *hdr)
+{
+    JSResolvingEntry *entry = (JSResolvingEntry *)hdr;
+
+    return &entry->key;
+}
+
+JS_STATIC_DLL_CALLBACK(JSDHashNumber)
+resolving_HashKey(JSDHashTable *table, const void *ptr)
+{
+    const JSResolvingKey *key = (const JSResolvingKey *)ptr;
+
+    return ((JSDHashNumber)key->obj >> JSVAL_TAGBITS) ^ key->id;
+}
+
+JS_PUBLIC_API(JSBool)
+resolving_MatchEntry(JSDHashTable *table,
+                     const JSDHashEntryHdr *hdr,
+                     const void *ptr)
+{
+    const JSResolvingEntry *entry = (const JSResolvingEntry *)hdr;
+    const JSResolvingKey *key = (const JSResolvingKey *)ptr;
+
+    return entry->key.obj == key->obj && entry->key.id == key->id;
+}
+
+static JSDHashTableOps resolving_dhash_ops = {
+    JS_DHashAllocTable,
+    JS_DHashFreeTable,
+    resolving_GetKey,
+    resolving_HashKey,
+    resolving_MatchEntry,
+    JS_DHashMoveEntryStub,
+    JS_DHashClearEntryStub,
+    JS_DHashFinalizeStub
+};
+
 #if defined JS_THREADSAFE && defined DEBUG
 JS_FRIEND_API(JSBool)
 _js_LookupProperty(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
@@ -2009,10 +2048,14 @@ js_LookupProperty(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
     JSSymbol *sym;
     JSClass *clasp;
     JSResolveOp resolve;
+    JSResolvingKey key;
+    JSDHashTable *table;
+    JSDHashEntryHdr *entry;
     JSNewResolveOp newresolve;
     uintN flags;
     uint32 format;
     JSObject *obj2, *proto;
+    JSBool ok;
     JSScopeProperty *sprop;
 
     /*
@@ -2033,10 +2076,43 @@ js_LookupProperty(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
             /* Shared prototype scope: try resolve before lookup. */
             sym = NULL;
         }
+
+        /* Try obj's class resolve hook if id was not found in obj's scope. */
         if (!sym) {
             clasp = LOCKED_OBJ_GET_CLASS(obj);
             resolve = clasp->resolve;
             if (resolve != JS_ResolveStub) {
+                /* Avoid recursion on (obj, id) already being resolved on cx. */
+                key.obj = obj;
+                key.id = id;
+                table = cx->resolving;
+                if (table) {
+                    entry = JS_DHashTableOperate(table, &key, JS_DHASH_LOOKUP);
+                    if (JS_DHASH_ENTRY_IS_BUSY(entry)) {
+                        JS_UNLOCK_OBJ(cx, obj);
+                        goto out;
+                    }
+                } else {
+                    table = JS_NewDHashTable(&resolving_dhash_ops,
+                                             NULL,
+                                             sizeof(JSResolvingEntry),
+                                             JS_DHASH_MIN_SIZE);
+                    if (!table)
+                        goto outofmem;
+                    cx->resolving = table;
+                }
+                entry = JS_DHashTableOperate(table, &key, JS_DHASH_ADD);
+                if (!entry) {
+            outofmem:
+                    JS_UNLOCK_OBJ(cx, obj);
+                    JS_ReportOutOfMemory(cx);
+                    return JS_FALSE;
+                }
+                ((JSResolvingEntry *)entry)->key = key;
+
+                /* Null *propp here so we can test it at cleanup: safely. */
+                *propp = NULL;
+
                 if (clasp->flags & JSCLASS_NEW_RESOLVE) {
                     newresolve = (JSNewResolveOp)resolve;
                     flags = 0;
@@ -2049,43 +2125,84 @@ js_LookupProperty(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
                     }
                     obj2 = NULL;
                     JS_UNLOCK_OBJ(cx, obj);
-                    if (!newresolve(cx, obj, js_IdToValue(id), flags, &obj2))
+                    ok = newresolve(cx, obj, js_IdToValue(id), flags, &obj2);
-                        return JS_FALSE;
+                    if (!ok)
+                        goto cleanup;
                     JS_LOCK_OBJ(cx, obj);
                     SET_OBJ_INFO(obj, file, line);
                     if (obj2) {
+                        /* Resolved: juggle locks and lookup id again. */
+                        if (obj2 != obj) {
+                            JS_UNLOCK_OBJ(cx, obj);
+                            JS_LOCK_OBJ(cx, obj2);
+                        }
                         scope = OBJ_SCOPE(obj2);
-                        if (MAP_IS_NATIVE(&scope->map) &&
+                        if (!MAP_IS_NATIVE(&scope->map)) {
-                            scope->object == obj2) {
+                            /* Whoops, newresolve handed back a foreign obj2. */
+                            JS_ASSERT(obj2 != obj);
+                            JS_UNLOCK_OBJ(cx, obj2);
+                            ok = OBJ_LOOKUP_PROPERTY(cx, obj2, id, objp, propp);
+                            if (!ok || *propp)
+                                goto cleanup;
+                            JS_LOCK_OBJ(cx, obj2);
+                        } else {
+                            /*
+                             * Require that obj2 have its own scope now, as we
+                             * do for old-style resolve.  If it doesn't, then
+                             * id was not truly resolved, and we'll find it in
+                             * the proto chain, or miss it if obj2's proto is
+                             * not on obj's proto chain.  That last case is a
+                             * "too bad!" case.
+                             */
+                            if (scope->object == obj2)
                                 sym = scope->ops->lookup(cx, scope, id, hash);
+                        }
+                        if (obj2 != obj && (!sym || !sym_property(sym))) {
+                            JS_UNLOCK_OBJ(cx, obj2);
+                            JS_LOCK_OBJ(cx, obj);
+                        }
 #ifdef DEBUG
                         /*
-                             * Set obj so we can assert that OBJ_SCOPE(obj)
+                         * Set obj so we can assert that its map points to
-                             * is scope, below, just before setting *objp to
+                         * scope, before we return scope->object in *objp.
-                             * scope->object.
                          */
                         if (sym && sym_property(sym))
                             obj = obj2;
 #endif
                     }
-                    }
                 } else {
+                    /*
+                     * Old resolve always requires id re-lookup if obj owns
+                     * its scope after resolve returns.
+                     */
                     JS_UNLOCK_OBJ(cx, obj);
-                    if (!resolve(cx, obj, js_IdToValue(id)))
+                    ok = resolve(cx, obj, js_IdToValue(id));
-                        return JS_FALSE;
+                    if (!ok)
+                        goto cleanup;
                     JS_LOCK_OBJ(cx, obj);
                     SET_OBJ_INFO(obj, file, line);
                     scope = OBJ_SCOPE(obj);
-                    if (MAP_IS_NATIVE(&scope->map) && scope->object == obj)
+                    JS_ASSERT(MAP_IS_NATIVE(&scope->map));
+                    if (scope->object == obj)
                         sym = scope->ops->lookup(cx, scope, id, hash);
                 }
+
+            cleanup:
+                JS_DHashTableRawRemove(table, entry);
+                if (table->entryCount == 0) {
+                    cx->resolving = NULL;
+                    JS_DHashTableDestroy(table);
+                }
+                if (!ok || *propp)
+                    return ok;
             }
         }
+
         if (sym && (sprop = sym_property(sym)) != NULL) {
             JS_ASSERT(OBJ_SCOPE(obj) == scope);
             *objp = scope->object;      /* XXXbe hide in jsscope.[ch] */
 #ifdef JS_THREADSAFE
-            js_HoldScopeProperty(cx, scope, sprop);
+            HOLD_SCOPE_PROPERTY(scope, sprop);
 #endif
             *propp = (JSProperty *) sprop;
             return JS_TRUE;
@@ -2098,6 +2215,8 @@ js_LookupProperty(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
             return OBJ_LOOKUP_PROPERTY(cx, proto, id, objp, propp);
         obj = proto;
     }
+
+out:
     *objp = NULL;
     *propp = NULL;
     return JS_TRUE;
@@ -2120,7 +2239,7 @@ js_FindProperty(JSContext *cx, jsid id, JSObject **objp, JSObject **pobjp,
         if (prop) {
 #ifdef JS_THREADSAFE
             JS_ASSERT(OBJ_IS_NATIVE(obj));
-            ((JSScopeProperty *)prop)->nrefs++;
+            HOLD_SCOPE_PROPERTY(OBJ_SCOPE(obj), (JSScopeProperty *)prop);
 #endif
             *objp = obj;
             *pobjp = obj;
@@ -2277,7 +2396,7 @@ js_GetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
           ? LOCKED_OBJ_GET_SLOT(obj2, slot)
           : JSVAL_VOID;
 #ifndef JS_THREADSAFE
-    sprop->nrefs++;
+    HOLD_SCOPE_PROPERTY(scope, sprop);
 #endif
     JS_UNLOCK_SCOPE(cx, scope);
     if (!SPROP_GET(cx, sprop, obj, obj2, vp)) {
@@ -2298,20 +2417,19 @@ js_GetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
 JSBool
 js_SetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
 {
+    JSObject *pobj;
+    JSScopeProperty *sprop;
     JSRuntime *rt;
     JSClass *clasp;
-    JSScope *scope;
     JSHashNumber hash;
-    JSSymbol *sym, *protosym;
+    JSScope *scope;
-    JSScopeProperty *sprop;
+    JSSymbol *sym;
-    jsval userid;
-    JSObject *proto, *tmp;
-    JSPropertyOp getter, setter;
     uintN attrs;
+    jsval userid;
+    JSPropertyOp getter, setter;
     JSBool ok;
     jsval pval;
     uint32 slot;
-    JSString *str;
 
     /*
      * Handle old bug that took empty string as zero index.  Also convert
@@ -2319,37 +2437,38 @@ js_SetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
      */
     CHECK_FOR_FUNNY_INDEX(id);
 
-    /* XXX - TEMPORARY HACK */
+    if (!js_LookupProperty(cx, obj, id, &pobj, (JSProperty **)&sprop))
-    if (!js_LookupProperty(cx, obj, id, &tmp, (JSProperty **)&sprop))
         return JS_FALSE;
-
     rt = cx->runtime;
-    JS_LOCK_OBJ(cx, obj);
+    clasp = OBJ_GET_CLASS(cx, obj);
-    clasp = LOCKED_OBJ_GET_CLASS(obj);
-    scope = OBJ_SCOPE(obj);
     hash = js_HashId(id);
 
+#if JS_HAS_OBJ_WATCHPOINT
+    if (!sprop) {
+        JS_LOCK_OBJ(cx, obj);
+        scope = OBJ_SCOPE(obj);
         sym = scope->ops->lookup(cx, scope, id, hash);
         if (sym) {
             sprop = sym_property(sym);
-#if JS_HAS_OBJ_WATCHPOINT
             if (!sprop && scope->object == obj) {
                 /*
                  * Deleted property place-holder, could have a watchpoint that
-             * holds the deleted-but-watched property.  If so, slots may have
+                 * holds the deleted-but-watched property.  If so, obj->slots
-             * shrunk, or at least freeslot may have shrunk due to the delete
+                 * may have shrunk, or at least obj->map->freeslot may have
-             * operation destroying the property.
+                 * shrunk due to a delete operation destroying the property.
                  */
                 sprop = js_FindWatchPoint(rt, obj, js_IdToValue(id));
-            if (sprop &&
+                if (sprop) {
-                (slot = sprop->slot) != SPROP_INVALID_SLOT &&
+                    slot = sprop->slot;
+                    if (slot != SPROP_INVALID_SLOT &&
                         slot >= scope->map.freeslot) {
                         if (slot >= scope->map.nslots) {
                             uint32 nslots;
                             jsval *slots;
 
                             nslots = slot + slot / 2;
-                    slots = (jsval *) JS_realloc(cx, obj->slots - 1,
+                            slots = (jsval *)
+                                JS_realloc(cx, obj->slots - 1,
                                            (nslots + 1) * sizeof(jsval));
                             if (!slots) {
                                 JS_UNLOCK_OBJ(cx, obj);
@@ -2360,94 +2479,90 @@ js_SetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
                         }
                         scope->map.freeslot = slot + 1;
                     }
+
+                    /* Reconnect sprop to its symbol as sym_property(sym). */
+                    sym->entry.value = sprop;
+                    HOLD_SCOPE_PROPERTY(scope, sprop);
+                }
+            }
+            if (sprop) {
+#ifdef JS_THREADSAFE
+                HOLD_SCOPE_PROPERTY(scope, sprop);
+#endif
+                pobj = obj;
+            }
+        }
+        if (!sprop)
+            JS_UNLOCK_OBJ(cx, obj);
     }
 #endif
-    } else {
-        sprop = NULL;
-    }
 
-    if (!sprop || (proto = scope->object) != obj) {
+    /*
-        /* Find a prototype property with the same id. */
+     * Now either sprop is null, meaning id was not found in obj or one of its
-        if (sprop) {
+     * prototypes; or sprop is non-null, meaning id was found in pobj's scope.
-            /* Already found, check for a readonly prototype property. */
+     * If JS_THREADSAFE and sprop is non-null, then scope is locked, and sprop
-            attrs = sprop->attrs;
+     * is held: it needs a js_DropScopeProperty followed by a JS_UNLOCK_OBJ (or
-            if (attrs & JSPROP_READONLY)
+     * what is equivalent to those two function calls, an OBJ_DROP_PROPERTY),
-                goto read_only;
+     * before we return.
-
+     */
-            /* Don't clone a setter or shared prototype property. */
+    if (!sprop) {
-            if (attrs & (JSPROP_SETTER | JSPROP_SHARED)) {
-                sprop->nrefs++;
-                JS_UNLOCK_SCOPE(cx, scope);
-
-                ok = SPROP_SET(cx, sprop, obj, proto, vp);
-                JS_LOCK_OBJ_VOID(cx, proto,
-                                 js_DropScopeProperty(cx, scope, sprop));
-                return ok;
-            }
-
-            /* XXXbe ECMA violation: inherit attrs, etc. */
-            userid = sprop->id;
-            getter = SPROP_GETTER_SCOPE(sprop, scope);
-            setter = SPROP_SETTER_SCOPE(sprop, scope);
-            sym = NULL;
-        } else {
-            /* Not found via a shared scope: we must follow the proto chain. */
-            proto = LOCKED_OBJ_GET_PROTO(obj);
-            sprop = NULL;
         attrs = JSPROP_ENUMERATE;
         userid = JSVAL_NULL;
         getter = clasp->getProperty;
         setter = clasp->setProperty;
-
+    } else {
-            JS_UNLOCK_OBJ(cx, obj);
-            while (proto) {
-                JS_LOCK_OBJ(cx, proto);
-                if (OBJ_IS_NATIVE(proto)) {
-                    scope = OBJ_SCOPE(proto);
-                    protosym = scope->ops->lookup(cx, scope, id, hash);
-                    if (protosym) {
-                        sprop = sym_property(protosym);
-                        if (sprop) {
-                            /*
-                             * Repeat the readonly and setter/shared code here.
-                             * It's tricky to fuse with the code above because
-                             * we must hold proto's scope-lock while loading
-                             * from sprop, and finally release that lock and
-                             * reacquire obj's scope-lock in this case (where
-                             * obj and proto are not sharing a scope).
-                             */
         attrs = sprop->attrs;
         if (attrs & JSPROP_READONLY) {
-                                JS_UNLOCK_OBJ(cx, proto);
+            /* XXXbe ECMA violation: readonly proto-property stops set cold. */
-                                goto unlocked_read_only;
+            OBJ_DROP_PROPERTY(cx, pobj, (JSProperty *)sprop);
+            if (!JSVERSION_IS_ECMA(cx->version)) {
+                JSString *str = js_DecompileValueGenerator(cx,
+                                                           JSDVG_IGNORE_STACK,
+                                                           js_IdToValue(id),
+                                                           NULL);
+                if (str) {
+                    JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
+                                         JSMSG_READ_ONLY,
+                                         JS_GetStringBytes(str));
                 }
+                return JS_FALSE;
+            }
+            return JS_TRUE;
+        }
+
+        /*
+         * Set scope for use below.  It was locked by js_LookupProperty, and
+         * we know pobj owns it (i.e., scope->object == pobj).  Therefore we
+         * optimize JS_UNLOCK_OBJ(cx, pobj) into JS_UNLOCK_SCOPE(cx, scope).
+         */
+        scope = OBJ_SCOPE(pobj);
+
+        if (pobj != obj) {
+            /* Don't clone a setter or shared prototype property. */
             if (attrs & (JSPROP_SETTER | JSPROP_SHARED)) {
-                                sprop->nrefs++;
+#ifndef JS_THREADSAFE
+                HOLD_SCOPE_PROPERTY(scope, sprop);
+#endif
                 JS_UNLOCK_SCOPE(cx, scope);
 
-                                ok = SPROP_SET(cx, sprop, obj, proto, vp);
+                ok = SPROP_SET(cx, sprop, obj, pobj, vp);
-                                JS_LOCK_OBJ_VOID(cx, proto,
+                JS_LOCK_OBJ_VOID(cx, pobj,
                                  js_DropScopeProperty(cx, scope, sprop));
                 return ok;
             }
 
-                            /* XXXbe ECMA violation: inherit attrs, etc. */
+            /* XXXbe ECMA violation: inherit attrs, getter, setter. */
             userid = sprop->id;
             getter = SPROP_GETTER_SCOPE(sprop, scope);
             setter = SPROP_SETTER_SCOPE(sprop, scope);
-                            JS_UNLOCK_OBJ(cx, proto);
+            JS_UNLOCK_SCOPE(cx, scope);
-                            break;
+            sprop = NULL;
         }
     }
-                }
-                tmp = LOCKED_OBJ_GET_PROTO(proto);
-                JS_UNLOCK_OBJ(cx, proto);
-                proto = tmp;
-            }
-            JS_LOCK_OBJ(cx, obj);
-        }
 
+    if (!sprop) {
         /* Find or make a property descriptor with the right heritage. */
+        JS_LOCK_OBJ(cx, obj);
         scope = js_MutateScope(cx, obj, id, getter, setter, attrs, &sprop);
         if (!scope) {
             JS_UNLOCK_OBJ(cx, obj);
@@ -2468,59 +2583,40 @@ js_SetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
         /* XXXbe called with obj locked */
         if (!clasp->addProperty(cx, obj, sprop->id, vp)) {
             js_DestroyScopeProperty(cx, scope, sprop);
-            JS_UNLOCK_OBJ(cx, obj);
+            JS_UNLOCK_SCOPE(cx, scope);
             return JS_FALSE;
         }
 
-        /* Initialize new properties to undefined. */
+        /* Initialize new property value (passed to setter) to undefined. */
         if (SPROP_HAS_VALID_SLOT(sprop))
             LOCKED_OBJ_SET_SLOT(obj, sprop->slot, JSVAL_VOID);
 
-        if (sym) {
-            /* Null-valued symbol left behind from a delete operation. */
-            sym->entry.value = js_HoldScopeProperty(cx, scope, sprop);
-        }
-    }
-
-    if (!sym) {
         /* Need a new symbol as well as a new property. */
         sym = scope->ops->add(cx, scope, id, sprop);
         if (!sym) {
             js_DestroyScopeProperty(cx, scope, sprop);
-            JS_UNLOCK_OBJ(cx, obj);
+            JS_UNLOCK_SCOPE(cx, scope);
             return JS_FALSE;
         }
 #if JS_BUG_AUTO_INDEX_PROPS
         if (SPROP_HAS_VALID_SLOT(sprop)) {
-            jsid id2 = (jsid) INT_TO_JSVAL(sprop->slot - JSSLOT_START);
+            jsid index_id = (jsid) INT_TO_JSVAL(sprop->slot - JSSLOT_START);
-            if (!scope->ops->add(cx, scope, id2, sprop)) {
+            if (!scope->ops->add(cx, scope, index_id, sprop)) {
                 scope->ops->remove(cx, scope, id);
-                JS_UNLOCK_OBJ(cx, obj);
+                JS_UNLOCK_SCOPE(cx, scope);
                 return JS_FALSE;
             }
-            PROPERTY_CACHE_FILL(cx, &rt->propertyCache, obj, id2,
+            PROPERTY_CACHE_FILL(cx, &rt->propertyCache, obj, index_id,
                                 (JSProperty *)sprop);
         }
 #endif
         PROPERTY_CACHE_FILL(cx, &rt->propertyCache, obj, id,
                             (JSProperty *)sprop);
-    }
 
-    /* Check for readonly now that we have sprop. */
+#ifdef JS_THREADSAFE
-    if (sprop->attrs & JSPROP_READONLY) {
+        /* Hold sprop as if it were returned from js_LookupProperty. */
-read_only:
+        HOLD_SCOPE_PROPERTY(scope, sprop);
-        JS_UNLOCK_OBJ(cx, obj);
+#endif
-
-unlocked_read_only:
-        if (JSVERSION_IS_ECMA(cx->version))
-            return JS_TRUE;
-        str = js_DecompileValueGenerator(cx, JSDVG_IGNORE_STACK,
-                                         js_IdToValue(id), NULL);
-        if (str) {
-            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
-                                 JSMSG_READ_ONLY, JS_GetStringBytes(str));
-        }
-        return JS_FALSE;
     }
 
     /* Get the current property value from its slot. */
@@ -2530,8 +2626,10 @@ unlocked_read_only:
         pval = LOCKED_OBJ_GET_SLOT(obj, slot);
     }
 
+#ifndef JS_THREADSAFE
     /* Hold sprop across setter callout, and drop after, in case of delete. */
-    sprop->nrefs++;
+    HOLD_SCOPE_PROPERTY(scope, sprop);
+#endif
 
     /* Avoid deadlock by unlocking obj while calling sprop's setter. */
     JS_UNLOCK_OBJ(cx, obj);

js/src/jsscope.c

@@ -146,13 +146,26 @@ js_hash_scope_lookup(JSContext *cx, JSScope *scope, jsid id, JSHashNumber hash)
     return sym;
 }
 
+#ifdef DEBUG
+#define REDEFINING_SYMBOL(sym)          (!(sym)->scope)
+#define START_REDEFINING_SYMBOL(sym)    ((sym)->scope = NULL)
+#define END_REDEFINING_SYMBOL(sym)      ((sym)->scope = scope)
+#else
+#define REDEFINING_SYMBOL(sym)          JS_FALSE
+#define START_REDEFINING_SYMBOL(sym)    /* nothing */
+#define END_REDEFINING_SYMBOL(sym)      /* nothing */
+#endif
+
 #define SCOPE_ADD(PRIV, CLASS_SPECIFIC_CODE)                                  \
     JS_BEGIN_MACRO                                                            \
 	if (sym) {                                                            \
 	    if (sym->entry.value == sprop)                                    \
 		return sym;                                                   \
-	    if (sym->entry.value)                                             \
+	    if (sym->entry.value) {                                           \
+                START_REDEFINING_SYMBOL(sym);                                 \
 		js_free_symbol(PRIV, &sym->entry, HT_FREE_VALUE);             \
+                END_REDEFINING_SYMBOL(sym);                                   \
+            }                                                                 \
 	} else {                                                              \
 	    CLASS_SPECIFIC_CODE                                               \
 	    sym->scope = scope;                                               \
@@ -585,6 +598,9 @@ js_DropScopeProperty(JSContext *cx, JSScope *scope, JSScopeProperty *sprop)
     if (sprop) {
 	JS_ASSERT(sprop->nrefs > 0);
 	if (--sprop->nrefs == 0) {
+            JS_ASSERT(REDEFINING_SYMBOL(sprop->symbols) ||
+                      !scope->ops->lookup(cx, scope, sym_id(sprop->symbols),
+                                          js_HashId(sym_id(sprop->symbols))));
 	    js_DestroyScopeProperty(cx, scope, sprop);
 	    sprop = NULL;
 	}

js/src/jsscope.h

@@ -191,10 +191,18 @@ js_NewScopeProperty(JSContext *cx, JSScope *scope, jsid id,
 extern void
 js_DestroyScopeProperty(JSContext *cx, JSScope *scope, JSScopeProperty *sprop);
 
+/*
+ * For bogus historical reasons, these functions cope with null sprop params.
+ * They hide details of reference or "short-term property hold" accounting, so
+ * they're worth the space.  But do use HOLD_SCOPE_PROPERTY to keep a non-null
+ * sprop in hand for a scope-locking critical section.
+ */
 extern JSScopeProperty *
 js_HoldScopeProperty(JSContext *cx, JSScope *scope, JSScopeProperty *sprop);
 
 extern JSScopeProperty *
 js_DropScopeProperty(JSContext *cx, JSScope *scope, JSScopeProperty *sprop);
 
+#define HOLD_SCOPE_PROPERTY(scope,sprop) ((sprop)->nrefs++)
+
 #endif /* jsscope_h___ */