Bug 824321 - "Assertion failure: !IsThingPoisoned(thing)," r=terrence
authorJon Coppeard <jcoppeard@mozilla.com>
Wed, 02 Jan 2013 17:22:14 +0000
changeset 123486 e3b4a698e0362c0d3346e11602607989c4c4433d
parent 123485 40b1de54022af3479292821db43ca018730f5221
child 123487 3e5257f5f4a60c7b306389533d872c0105a4e7f0
push id3129
push userakeybl@mozilla.com
push dateMon, 07 Jan 2013 22:54:45 +0000
treeherdermozilla-aurora@090bc89ff6b4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersterrence
bugs824321
milestone20.0a1
Bug 824321 - "Assertion failure: !IsThingPoisoned(thing)," r=terrence
js/src/gc/Heap.h
js/src/gc/Marking.cpp
js/src/gc/RootMarking.cpp
js/src/jit-test/tests/gc/bug-824321.js
js/src/jsgc.cpp
js/src/jsgcinlines.h
--- a/js/src/gc/Heap.h
+++ b/js/src/gc/Heap.h
@@ -983,12 +983,42 @@ Cell::compartment() const
 #ifdef DEBUG
 bool
 Cell::isAligned() const
 {
     return Arena::isAligned(address(), arenaHeader()->getThingSize());
 }
 #endif
 
+inline bool
+InFreeList(ArenaHeader *aheader, void *thing)
+{
+    if (!aheader->hasFreeThings())
+        return false;
+
+    FreeSpan firstSpan(aheader->getFirstFreeSpan());
+    uintptr_t addr = reinterpret_cast<uintptr_t>(thing);
+
+    for (const FreeSpan *span = &firstSpan;;) {
+        /* If the thing comes before the current span, it's not free. */
+        if (addr < span->first)
+            return false;
+
+        /*
+         * If we find it inside the span, it's dead. We use here "<=" and not
+         * "<" even for the last span as we know that thing is inside the
+         * arena. Thus, for the last span thing < span->end.
+         */
+        if (addr <= span->last)
+            return true;
+
+        /*
+         * The last possible empty span is an the end of the arena. Here
+         * span->end < thing < thingsEnd and so we must have more spans.
+         */
+        span = span->nextSpan();
+    }
+}
+
 } /* namespace gc */
 } /* namespace js */
 
 #endif /* gc_heap_h___ */
--- a/js/src/gc/Marking.cpp
+++ b/js/src/gc/Marking.cpp
@@ -139,17 +139,24 @@ CheckMarkedThing(JSTracer *trc, T *thing
     JS_ASSERT_IF(rt->gcStrictCompartmentChecking,
                  thing->compartment()->isCollecting() ||
                  thing->compartment() == rt->atomsCompartment);
 
     JS_ASSERT_IF(IS_GC_MARKING_TRACER(trc) && ((GCMarker *)trc)->getMarkColor() == GRAY,
                  thing->compartment()->isGCMarkingGray() ||
                  thing->compartment() == rt->atomsCompartment);
 
-    JS_ASSERT(!IsThingPoisoned(thing));
+    /*
+     * Try to assert that the thing is allocated.  This is complicated by the
+     * fact that allocated things may still contain the poison pattern if that
+     * part has not been overwritten, and that the free span list head in the
+     * ArenaHeader may not be synced with the real one in ArenaLists.
+     */
+    JS_ASSERT_IF(IsThingPoisoned(thing) && rt->isHeapBusy(),
+                 !InFreeList(thing->arenaHeader(), thing));
 }
 
 static GCMarker *
 AsGCMarker(JSTracer *trc)
 {
     JS_ASSERT(IS_GC_MARKING_TRACER(trc));
     return static_cast<GCMarker *>(trc);
 }
--- a/js/src/gc/RootMarking.cpp
+++ b/js/src/gc/RootMarking.cpp
@@ -82,45 +82,16 @@ MarkExactStackRoots(JSTracer *trc)
         for (ContextIter cx(trc->runtime); !cx.done(); cx.next()) {
             MarkExactStackRooters(trc, cx->thingGCRooters[i], ThingRootKind(i));
         }
         MarkExactStackRooters(trc, rt->mainThread.thingGCRooters[i], ThingRootKind(i));
     }
 }
 #endif /* JSGC_USE_EXACT_ROOTING */
 
-static inline bool
-InFreeList(ArenaHeader *aheader, uintptr_t addr)
-{
-    if (!aheader->hasFreeThings())
-        return false;
-
-    FreeSpan firstSpan(aheader->getFirstFreeSpan());
-
-    for (const FreeSpan *span = &firstSpan;;) {
-        /* If the thing comes fore the current span, it's not free. */
-        if (addr < span->first)
-            return false;
-
-        /*
-         * If we find it inside the span, it's dead. We use here "<=" and not
-         * "<" even for the last span as we know that thing is inside the
-         * arena. Thus for the last span thing < span->end.
-         */
-        if (addr <= span->last)
-            return true;
-
-        /*
-         * The last possible empty span is an the end of the arena. Here
-         * span->end < thing < thingsEnd and so we must have more spans.
-         */
-        span = span->nextSpan();
-    }
-}
-
 enum ConservativeGCTest
 {
     CGCT_VALID,
     CGCT_LOWBITSET, /* excluded because one of the low bits was set */
     CGCT_NOTARENA,  /* not within arena range in a chunk */
     CGCT_OTHERCOMPARTMENT,  /* in another compartment */
     CGCT_NOTCHUNK,  /* not within a valid chunk */
     CGCT_FREEARENA, /* within arena containing only free things */
@@ -235,17 +206,17 @@ MarkIfGCThingWord(JSTracer *trc, uintptr
     if (status != CGCT_VALID)
         return status;
 
     /*
      * Check if the thing is free. We must use the list of free spans as at
      * this point we no longer have the mark bits from the previous GC run and
      * we must account for newly allocated things.
      */
-    if (InFreeList(aheader, uintptr_t(thing)))
+    if (InFreeList(aheader, thing))
         return CGCT_NOTLIVE;
 
     JSGCTraceKind traceKind = MapAllocToTraceKind(thingKind);
 #ifdef DEBUG
     const char pattern[] = "machine_stack %p";
     char nameBuf[sizeof(pattern) - 2 + sizeof(thing) * 2];
     JS_snprintf(nameBuf, sizeof(nameBuf), pattern, thing);
     JS_SET_TRACING_NAME(trc, nameBuf);
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/gc/bug-824321.js
@@ -0,0 +1,3 @@
+x = "\udada\udada";
+gc();
+
--- a/js/src/jsgc.cpp
+++ b/js/src/jsgc.cpp
@@ -262,17 +262,17 @@ ArenaHeader::checkSynchronizedWithFreeLi
      */
     JS_ASSERT(allocated());
 
     /*
      * We can be called from the background finalization thread when the free
      * list in the compartment can mutate at any moment. We cannot do any
      * checks in this case.
      */
-    if (!compartment->rt->isHeapBusy())
+    if (IsBackgroundFinalized(getAllocKind()) && !compartment->rt->isHeapBusy())
         return;
 
     FreeSpan firstSpan = FreeSpan::decodeOffsets(arenaAddress(), firstFreeSpanOffsets);
     if (firstSpan.isEmpty())
         return;
     const FreeSpan *list = compartment->arenas.getFreeList(getAllocKind());
     if (list->isEmpty() || firstSpan.arenaAddress() != list->arenaAddress())
         return;
--- a/js/src/jsgcinlines.h
+++ b/js/src/jsgcinlines.h
@@ -480,29 +480,16 @@ class GCCompartmentGroupIter {
 
 /*
  * Allocates a new GC thing. After a successful allocation the caller must
  * fully initialize the thing before calling any function that can potentially
  * trigger GC. This will ensure that GC tracing never sees junk values stored
  * in the partially initialized thing.
  */
 
-template<typename T>
-static inline void
-UnpoisonThing(T *thing)
-{
-#ifdef DEBUG
-    /* Change the contents of memory slightly so that IsThingPoisoned returns false. */
-    JS_STATIC_ASSERT(sizeof(T) >= sizeof(FreeSpan) + sizeof(uint8_t));
-    uint8_t *p =
-        reinterpret_cast<uint8_t *>(reinterpret_cast<FreeSpan *>(thing) + 1);
-    *p = 0;
-#endif
-}
-
 template <typename T>
 inline T *
 NewGCThing(JSContext *cx, js::gc::AllocKind kind, size_t thingSize)
 {
     AssertCanGC();
     JS_ASSERT(thingSize == js::gc::Arena::thingSize(kind));
     JS_ASSERT_IF(cx->compartment == cx->runtime->atomsCompartment,
                  kind == FINALIZE_STRING ||
@@ -529,18 +516,16 @@ NewGCThing(JSContext *cx, js::gc::AllocK
     JS_ASSERT_IF(t && comp->wasGCStarted() && (comp->isGCMarking() || comp->isGCSweeping()),
                  t->arenaHeader()->allocatedDuringIncremental);
 
 #if defined(JSGC_GENERATIONAL) && defined(JS_GC_ZEAL)
     if (cx->runtime->gcVerifyPostData && IsNurseryAllocable(kind) && !IsAtomsCompartment(comp))
         comp->gcNursery.insertPointer(t);
 #endif
 
-    if (t)
-        UnpoisonThing(t);
     return t;
 }
 
 /* Alternate form which allocates a GC thing if doing so cannot trigger a GC. */
 template <typename T>
 inline T *
 TryNewGCThing(JSContext *cx, js::gc::AllocKind kind, size_t thingSize)
 {
@@ -562,18 +547,16 @@ TryNewGCThing(JSContext *cx, js::gc::All
                  t->arenaHeader()->allocatedDuringIncremental);
 
 #if defined(JSGC_GENERATIONAL) && defined(JS_GC_ZEAL)
     JSCompartment *comp = cx->compartment;
     if (cx->runtime->gcVerifyPostData && IsNurseryAllocable(kind) && !IsAtomsCompartment(comp))
         comp->gcNursery.insertPointer(t);
 #endif
 
-    if (t)
-        UnpoisonThing(t);
     return t;
 }
 
 } /* namespace gc */
 } /* namespace js */
 
 inline JSObject *
 js_NewGCObject(JSContext *cx, js::gc::AllocKind kind)