Bug 569422 - Miscellaneous fixes for GCed shapes (r=brendan)
authorBill McCloskey <wmccloskey@mozilla.com>
Wed, 23 Mar 2011 11:57:19 -0700
changeset 64357 a0ae696f155916d79a4b3a7a06abdf36154bed6a
parent 64356 b064d5e3e2e0f530d823b50cf07323559cfec9f8
child 64358 e6dd3dc8670c2280a32eda4c4bbb55e3e31c7e5f
push idunknown
push userunknown
push dateunknown
reviewersbrendan
bugs569422
milestone2.0b13pre
Bug 569422 - Miscellaneous fixes for GCed shapes (r=brendan)
js/src/jsdbgapi.cpp
js/src/jsgc.cpp
js/src/jsgcinlines.h
js/src/jsinterp.cpp
js/src/jsobj.cpp
js/src/jsobj.h
js/src/jsobjinlines.h
js/src/jspropertycache.cpp
js/src/jspropertycache.h
js/src/jsscope.cpp
js/src/shell/js.cpp
--- a/js/src/jsdbgapi.cpp
+++ b/js/src/jsdbgapi.cpp
@@ -578,53 +578,51 @@ struct JSWatchPoint {
     uintN               flags;
 };
 
 #define JSWP_LIVE       0x1             /* live because set and not cleared */
 #define JSWP_HELD       0x2             /* held while running handler/setter */
 
 /*
  * NB: DropWatchPointAndUnlock releases cx->runtime->debuggerLock in all cases.
+ * The sweeping parameter is true if the watchpoint and its object are about to
+ * be finalized, in which case we don't need to changeProperty.
  */
 static JSBool
-DropWatchPointAndUnlock(JSContext *cx, JSWatchPoint *wp, uintN flag)
+DropWatchPointAndUnlock(JSContext *cx, JSWatchPoint *wp, uintN flag, bool sweeping)
 {
     bool ok = true;
     JSRuntime *rt = cx->runtime;
 
     wp->flags &= ~flag;
     if (wp->flags != 0) {
         DBG_UNLOCK(rt);
         return ok;
     }
 
-    /*
-     * Switch to the same compartment as the watch point, since changeProperty, below,
-     * needs to have a compartment.
-     */
-    SwitchToCompartment sc(cx, wp->object);
-
     /* Remove wp from the list, then restore wp->shape->setter from wp. */
     ++rt->debuggerMutations;
     JS_REMOVE_LINK(&wp->links);
     DBG_UNLOCK(rt);
 
     /*
      * If the property isn't found on wp->object, then someone else must have deleted it,
      * and we don't need to change the property attributes.
      */
-    const Shape *shape = wp->shape;
-    const Shape *wprop = wp->object->nativeLookup(shape->id);
-    if (wprop &&
-        wprop->hasSetterValue() == shape->hasSetterValue() &&
-        IsWatchedProperty(cx, wprop)) {
-        shape = wp->object->changeProperty(cx, wprop, 0, wprop->attributes(),
-                                           wprop->getter(), wp->setter);
-        if (!shape)
-            ok = false;
+    if (!sweeping) {
+        const Shape *shape = wp->shape;
+        const Shape *wprop = wp->object->nativeLookup(shape->id);
+        if (wprop &&
+            wprop->hasSetterValue() == shape->hasSetterValue() &&
+            IsWatchedProperty(cx, wprop)) {
+            shape = wp->object->changeProperty(cx, wprop, 0, wprop->attributes(),
+                                               wprop->getter(), wp->setter);
+            if (!shape)
+                ok = false;
+        }
     }
 
     cx->free(wp);
     return ok;
 }
 
 /*
  * NB: js_TraceWatchPoints does not acquire cx->runtime->debuggerLock, since
@@ -680,17 +678,17 @@ js_SweepWatchPoints(JSContext *cx)
     for (wp = (JSWatchPoint *)rt->watchPointList.next;
          &wp->links != &rt->watchPointList;
          wp = next) {
         next = (JSWatchPoint *)wp->links.next;
         if (IsAboutToBeFinalized(cx, wp->object)) {
             sample = rt->debuggerMutations;
 
             /* Ignore failures. */
-            DropWatchPointAndUnlock(cx, wp, JSWP_LIVE);
+            DropWatchPointAndUnlock(cx, wp, JSWP_LIVE, true);
             DBG_LOCK(rt);
             if (rt->debuggerMutations != sample + 1)
                 next = (JSWatchPoint *)rt->watchPointList.next;
         }
     }
     DBG_UNLOCK(rt);
 }
 
@@ -839,17 +837,17 @@ js_watch_set(JSContext *cx, JSObject *ob
                      * default setter and therefore seems not to be a method.
                      */
                     ok = obj->methodWriteBarrier(cx, *shape, *vp) != NULL;
                 }
             }
 
         out:
             DBG_LOCK(rt);
-            return DropWatchPointAndUnlock(cx, wp, JSWP_HELD) && ok;
+            return DropWatchPointAndUnlock(cx, wp, JSWP_HELD, false) && ok;
         }
     }
     DBG_UNLOCK(rt);
     return true;
 }
 
 static JSBool
 js_watch_set_wrapper(JSContext *cx, uintN argc, Value *vp)
@@ -1125,17 +1123,17 @@ JS_SetWatchPoint(JSContext *cx, JSObject
         wp->shape = NULL;
         wp->flags = JSWP_LIVE;
 
         /* XXXbe nest in obj lock here */
         if (!UpdateWatchpointShape(cx, wp, shape)) {
             /* Self-link so DropWatchPointAndUnlock can JS_REMOVE_LINK it. */
             JS_INIT_CLIST(&wp->links);
             DBG_LOCK(rt);
-            DropWatchPointAndUnlock(cx, wp, JSWP_LIVE);
+            DropWatchPointAndUnlock(cx, wp, JSWP_LIVE, false);
             return false;
         }
 
         /*
          * Now that wp is fully initialized, append it to rt's wp list.
          * Because obj is locked we know that no other thread could have added
          * a watchpoint for (obj, propid).
          */
@@ -1171,17 +1169,17 @@ JS_ClearWatchPoint(JSContext *cx, JSObje
     for (wp = (JSWatchPoint *)rt->watchPointList.next;
          &wp->links != &rt->watchPointList;
          wp = (JSWatchPoint *)wp->links.next) {
         if (wp->object == obj && SHAPE_USERID(wp->shape) == id) {
             if (handlerp)
                 *handlerp = wp->handler;
             if (closurep)
                 *closurep = wp->closure;
-            return DropWatchPointAndUnlock(cx, wp, JSWP_LIVE);
+            return DropWatchPointAndUnlock(cx, wp, JSWP_LIVE, false);
         }
     }
     DBG_UNLOCK(rt);
     if (handlerp)
         *handlerp = NULL;
     if (closurep)
         *closurep = NULL;
     return JS_TRUE;
@@ -1199,17 +1197,17 @@ JS_ClearWatchPointsForObject(JSContext *
     rt = cx->runtime;
     DBG_LOCK(rt);
     for (wp = (JSWatchPoint *)rt->watchPointList.next;
          &wp->links != &rt->watchPointList;
          wp = next) {
         next = (JSWatchPoint *)wp->links.next;
         if (wp->object == obj) {
             sample = rt->debuggerMutations;
-            if (!DropWatchPointAndUnlock(cx, wp, JSWP_LIVE))
+            if (!DropWatchPointAndUnlock(cx, wp, JSWP_LIVE, false))
                 return JS_FALSE;
             DBG_LOCK(rt);
             if (rt->debuggerMutations != sample + 1)
                 next = (JSWatchPoint *)rt->watchPointList.next;
         }
     }
     DBG_UNLOCK(rt);
     return JS_TRUE;
@@ -1222,19 +1220,21 @@ JS_ClearAllWatchPoints(JSContext *cx)
     JSWatchPoint *wp, *next;
     uint32 sample;
 
     rt = cx->runtime;
     DBG_LOCK(rt);
     for (wp = (JSWatchPoint *)rt->watchPointList.next;
          &wp->links != &rt->watchPointList;
          wp = next) {
+        SwitchToCompartment sc(cx, wp->object);
+
         next = (JSWatchPoint *)wp->links.next;
         sample = rt->debuggerMutations;
-        if (!DropWatchPointAndUnlock(cx, wp, JSWP_LIVE))
+        if (!DropWatchPointAndUnlock(cx, wp, JSWP_LIVE, false))
             return JS_FALSE;
         DBG_LOCK(rt);
         if (rt->debuggerMutations != sample + 1)
             next = (JSWatchPoint *)rt->watchPointList.next;
     }
     DBG_UNLOCK(rt);
     return JS_TRUE;
 }
--- a/js/src/jsgc.cpp
+++ b/js/src/jsgc.cpp
@@ -1637,91 +1637,26 @@ MarkContext(JSTracer *trc, JSContext *ac
 JS_REQUIRES_STACK void
 MarkRuntime(JSTracer *trc)
 {
     JSRuntime *rt = trc->context->runtime;
 
     if (rt->state != JSRTS_LANDING)
         MarkConservativeStackRoots(trc);
 
-    /*
-     * Verify that we do not have at this point unmarked GC things stored in
-     * autorooters. To maximize test coverage we abort even in non-debug
-     * builds for now, see bug 574313.
-     */
-    JSContext *iter;
-#if 0
-    iter = NULL;
-    while (JSContext *acx = js_ContextIterator(rt, JS_TRUE, &iter)) {
-        for (AutoGCRooter *gcr = acx->autoGCRooters; gcr; gcr = gcr->down) {
-#ifdef JS_THREADSAFE
-            JS_ASSERT_IF(!acx->thread->data.requestDepth, acx->thread->suspendCount);
-#endif
-            JS_ASSERT(JS_THREAD_DATA(acx)->conservativeGC.hasStackToScan());
-            void *thing;
-            switch (gcr->tag) {
-              default:
-                continue;
-              case AutoGCRooter::JSVAL: {
-                const Value &v = static_cast<AutoValueRooter *>(gcr)->val;
-                if (!v.isMarkable())
-                    continue;
-                thing = v.toGCThing();
-                break;
-              }
-              case AutoGCRooter::XML:
-                thing = static_cast<AutoXMLRooter *>(gcr)->xml;
-                break;
-              case AutoGCRooter::OBJECT:
-                thing = static_cast<AutoObjectRooter *>(gcr)->obj;
-                if (!thing)
-                    continue;
-                break;
-              case AutoGCRooter::ID: {
-                jsid id = static_cast<AutoIdRooter *>(gcr)->id();
-                if (!JSID_IS_GCTHING(id))
-                    continue;
-                thing = JSID_TO_GCTHING(id);
-                break;
-              }
-            }
-
-            if (JSString::isGCThingStatic(thing))
-                continue;
-
-            if (!reinterpret_cast<Cell *>(thing)->isMarked()) {
-                ConservativeGCTest test = MarkIfGCThingWord(trc, reinterpret_cast<jsuword>(thing));
-                fprintf(stderr,
-                        "Conservative GC scanner has missed the root 0x%p with tag %ld"
-                        " on the stack due to %d. The root location 0x%p, distance from"
-                        " the stack base %ld, conservative gc span %ld."
-                        " Consevtaive GC status for the thread %d."
-                        " Aborting.\n",
-                        thing, (long) gcr->tag, int(test), (void *) gcr,
-                        (long) ((jsword) JS_THREAD_DATA(acx)->nativeStackBase - (jsword) gcr),
-                        (long) ((jsword) JS_THREAD_DATA(acx)->nativeStackBase -
-                                (jsword) JS_THREAD_DATA(acx)->conservativeGC.nativeStackTop),
-                        int(JS_THREAD_DATA(acx)->conservativeGC.hasStackToScan()));
-                JS_ASSERT(false);
-                abort();
-            }
-        }
-    }
-#endif
-
     for (RootRange r = rt->gcRootsHash.all(); !r.empty(); r.popFront())
         gc_root_traversal(trc, r.front());
 
     for (GCLocks::Range r = rt->gcLocksHash.all(); !r.empty(); r.popFront())
         gc_lock_traversal(r.front(), trc);
 
     js_TraceAtomState(trc);
     js_MarkTraps(trc);
 
-    iter = NULL;
+    JSContext *iter = NULL;
     while (JSContext *acx = js_ContextIterator(rt, JS_TRUE, &iter))
         MarkContext(trc, acx);
 
 #ifdef JS_TRACER
     for (JSCompartment **c = rt->compartments.begin(); c != rt->compartments.end(); ++c)
         (*c)->traceMonitor.mark(trc);
 #endif
 
@@ -2909,16 +2844,17 @@ NewCompartment(JSContext *cx, JSPrincipa
             return NULL;
         }
     }
 
     JSCompartmentCallback callback = rt->compartmentCallback;
     if (callback && !callback(cx, compartment, JSCOMPARTMENT_NEW)) {
         AutoLockGC lock(rt);
         rt->compartments.popBack();
+        js_delete(compartment);
         return NULL;
     }
     return compartment;
 }
 
 } /* namespace gc */
 
 void
--- a/js/src/jsgcinlines.h
+++ b/js/src/jsgcinlines.h
@@ -188,18 +188,20 @@ NewFinalizableGCThing(JSContext *cx, uns
 #undef METER
 #undef METER_IF
 
 inline JSObject *
 js_NewGCObject(JSContext *cx, js::gc::FinalizeKind kind)
 {
     JS_ASSERT(kind >= js::gc::FINALIZE_OBJECT0 && kind <= js::gc::FINALIZE_OBJECT_LAST);
     JSObject *obj = NewFinalizableGCThing<JSObject>(cx, kind);
-    if (obj)
+    if (obj) {
         obj->capacity = js::gc::GetGCKindSlots(kind);
+        obj->map = NULL; /* Stops obj from being scanned until initializated. */
+    }
     return obj;
 }
 
 inline JSString *
 js_NewGCString(JSContext *cx)
 {
     return NewFinalizableGCThing<JSString>(cx, js::gc::FINALIZE_STRING);    
 }
@@ -217,18 +219,20 @@ js_NewGCExternalString(JSContext *cx, ui
     JSExternalString *str = NewFinalizableGCThing<JSExternalString>(cx, js::gc::FINALIZE_EXTERNAL_STRING);
     return str;
 }
 
 inline JSFunction*
 js_NewGCFunction(JSContext *cx)
 {
     JSFunction *fun = NewFinalizableGCThing<JSFunction>(cx, js::gc::FINALIZE_FUNCTION);
-    if (fun)
+    if (fun) {
         fun->capacity = JSObject::FUN_CLASS_RESERVED_SLOTS;
+        fun->map = NULL; /* Stops fun from being scanned until initializated. */
+    }
     return fun;
 }
 
 #if JS_HAS_XML_SUPPORT
 inline JSXML *
 js_NewGCXML(JSContext *cx)
 {
     return NewFinalizableGCThing<JSXML>(cx, js::gc::FINALIZE_XML);
--- a/js/src/jsinterp.cpp
+++ b/js/src/jsinterp.cpp
@@ -562,17 +562,16 @@ js_OnUnknownMethod(JSContext *cx, Value 
             return false;
 
         /*
          * Null map to cause prompt and safe crash if this object were to
          * escape due to a bug. This will make the object appear to be a
          * stillborn instance that needs no finalization, which is sound:
          * NoSuchMethod helper objects own no manually allocated resources.
          */
-        obj->map = NULL;
         obj->init(cx, &js_NoSuchMethodClass, NULL, NULL, NULL, false);
         obj->setSlot(JSSLOT_FOUND_FUNCTION, tvr.value());
         obj->setSlot(JSSLOT_SAVED_ID, vp[0]);
         vp[0].setObject(*obj);
     }
     return true;
 }
 
@@ -2021,16 +2020,17 @@ CanIncDecWithoutOverflow(int32_t i)
     JS_END_MACRO
 
 static bool
 AssertValidPropertyCacheHit(JSContext *cx, JSScript *script, JSFrameRegs& regs,
                             ptrdiff_t pcoff, JSObject *start, JSObject *found,
                             PropertyCacheEntry *entry)
 {
     uint32 sample = cx->runtime->gcNumber;
+    PropertyCacheEntry savedEntry = *entry;
 
     JSAtom *atom;
     if (pcoff >= 0)
         GET_ATOM_FROM_BYTECODE(script, regs.pc, pcoff, atom);
     else
         atom = cx->runtime->atomState.lengthAtom;
 
     JSObject *obj, *pobj;
@@ -2041,17 +2041,17 @@ AssertValidPropertyCacheHit(JSContext *c
         ok = js_FindProperty(cx, ATOM_TO_JSID(atom), &obj, &pobj, &prop);
     } else {
         obj = start;
         ok = js_LookupProperty(cx, obj, ATOM_TO_JSID(atom), &pobj, &prop);
     }
     if (!ok)
         return false;
     if (cx->runtime->gcNumber != sample)
-        return true;
+        JS_PROPERTY_CACHE(cx).restore(&savedEntry);
     JS_ASSERT(prop);
     JS_ASSERT(pobj == found);
 
     const Shape *shape = (Shape *) prop;
     if (entry->vword.isSlot()) {
         JS_ASSERT(entry->vword.toSlot() == shape->slot);
         JS_ASSERT(!shape->isMethod());
     } else if (entry->vword.isShape()) {
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -101,17 +101,17 @@
 #include "jsobjinlines.h"
 #include "jsscriptinlines.h"
 
 #include "jsautooplen.h"
 
 using namespace js;
 using namespace js::gc;
 
-JS_FRIEND_DATA(const JSObjectMap) JSObjectMap::sharedNonNative(JSObjectMap::SHAPELESS);
+JS_FRIEND_DATA(JSObjectMap) JSObjectMap::sharedNonNative(JSObjectMap::SHAPELESS);
 
 Class js_ObjectClass = {
     js_Object_str,
     JSCLASS_HAS_CACHED_PROTO(JSProto_Object),
     PropertyStub,         /* addProperty */
     PropertyStub,         /* delProperty */
     PropertyStub,         /* getProperty */
     StrictPropertyStub,   /* setProperty */
--- a/js/src/jsobj.h
+++ b/js/src/jsobj.h
@@ -209,17 +209,17 @@ void
 MeterEntryCount(uintN count);
 
 } /* namespace js */
 
 struct JSObjectMap {
     uint32 shape;       /* shape identifier */
     uint32 slotSpan;    /* one more than maximum live slot number */
 
-    static JS_FRIEND_DATA(const JSObjectMap) sharedNonNative;
+    static JS_FRIEND_DATA(JSObjectMap) sharedNonNative;
 
     explicit JSObjectMap(uint32 shape) : shape(shape), slotSpan(0) {}
     JSObjectMap(uint32 shape, uint32 slotSpan) : shape(shape), slotSpan(slotSpan) {}
 
     enum { INVALID_SHAPE = 0x8fffffff, SHAPELESS = 0xffffffff };
 
     bool isNative() const { return this != &sharedNonNative; }
 
--- a/js/src/jsobjinlines.h
+++ b/js/src/jsobjinlines.h
@@ -803,20 +803,18 @@ JSObject::init(JSContext *cx, js::Class 
 {
     clasp = aclasp;
     flags = 0;
 
 #ifdef DEBUG
     /*
      * NB: objShape must not be set here; rather, the caller must call setMap
      * or setSharedNonNativeMap after calling init. To defend this requirement
-     * we set map to null in DEBUG builds, and set objShape to a value we then
-     * assert obj->shape() never returns.
+     * we set objShape to a value that obj->shape() is asserted never to return.
      */
-    map = NULL;
     objShape = JSObjectMap::INVALID_SHAPE;
 #endif
 
     setProto(proto);
     setParent(parent);
 
     privateData = priv;
     slots = fixedSlots();
--- a/js/src/jspropertycache.cpp
+++ b/js/src/jspropertycache.cpp
@@ -503,8 +503,19 @@ PropertyCache::purgeForScript(JSContext 
             entry->kpc = NULL;
 #ifdef DEBUG
             entry->kshape = entry->vcap = 0;
             entry->vword.setNull();
 #endif
         }
     }
 }
+
+void
+PropertyCache::restore(PropertyCacheEntry *entry)
+{
+    PropertyCacheEntry *entry2;
+
+    empty = false;
+
+    entry2 = &table[hash(entry->kpc, entry->kshape)];
+    *entry2 = *entry;
+}
--- a/js/src/jspropertycache.h
+++ b/js/src/jspropertycache.h
@@ -265,13 +265,16 @@ class PropertyCache
      * not possible.
      */
     JS_REQUIRES_STACK PropertyCacheEntry *fill(JSContext *cx, JSObject *obj, uintN scopeIndex,
                                                uintN protoIndex, JSObject *pobj,
                                                const js::Shape *shape, JSBool adding = false);
 
     void purge(JSContext *cx);
     void purgeForScript(JSContext *cx, JSScript *script);
+
+    /* Restore an entry that may have been purged during a GC. */
+    void restore(PropertyCacheEntry *entry);
 };
 
 } /* namespace js */
 
 #endif /* jspropertycache_h___ */
--- a/js/src/jsscope.cpp
+++ b/js/src/jsscope.cpp
@@ -531,38 +531,45 @@ Shape::newDictionaryShape(JSContext *cx,
 }
 
 Shape *
 Shape::newDictionaryList(JSContext *cx, Shape **listp)
 {
     Shape *shape = *listp;
     Shape *list = shape;
 
-    Shape **childp = listp;
-    *childp = NULL;
+    /*
+     * We temporarily create the dictionary shapes using a root located on the
+     * stack. This way, the GC doesn't see any intermediate state until we
+     * switch listp at the end.
+     */
+    Shape *root = NULL;
+    Shape **childp = &root;
 
     while (shape) {
         JS_ASSERT_IF(!shape->frozen(), !shape->inDictionary());
 
         Shape *dprop = Shape::newDictionaryShape(cx, *shape, childp);
         if (!dprop) {
             METER(toDictFails);
             *listp = list;
             return NULL;
         }
 
         JS_ASSERT(!dprop->hasTable());
         childp = &dprop->parent;
         shape = shape->parent;
     }
 
-    list = *listp;
-    JS_ASSERT(list->inDictionary());
-    list->hashify(cx->runtime);
-    return list;
+    *listp = root;
+    root->listp = listp;
+
+    JS_ASSERT(root->inDictionary());
+    root->hashify(cx->runtime);
+    return root;
 }
 
 bool
 JSObject::toDictionaryMode(JSContext *cx)
 {
     JS_ASSERT(!inDictionaryMode());
     if (!Shape::newDictionaryList(cx, &lastProp))
         return false;
--- a/js/src/shell/js.cpp
+++ b/js/src/shell/js.cpp
@@ -3174,16 +3174,19 @@ split_finalize(JSContext *cx, JSObject *
 
 static void
 split_trace(JSTracer *trc, JSObject *obj)
 {
     ComplexObject *cpx;
 
     cpx = (ComplexObject *) JS_GetPrivate(trc->context, obj);
 
+    if (!cpx)
+        return; /* The object is not fully constructed. */
+
     if (!cpx->isInner && cpx->inner) {
         /* Mark the inner object. */
         JS_CALL_TRACER(trc, cpx->inner, JSTRACE_OBJECT, "ComplexObject.inner");
     }
 
     if (cpx->isInner && cpx->outer) {
         /* Mark the inner object. */
         JS_CALL_TRACER(trc, cpx->outer, JSTRACE_OBJECT, "ComplexObject.outer");