From 441d0eaf0d595d1cd340993926af13dee678ec31 Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Wed, 2 Jan 2013 17:22:14 +0000 Subject: [PATCH] Bug 824321 - "Assertion failure: !IsThingPoisoned(thing)," r=terrence --HG-- extra : rebase_source : e1662451f8f90af0b1f25073e7f919cebdf5d410 --- js/src/gc/Heap.h | 30 +++++++++++++++++++++++++ js/src/gc/Marking.cpp | 9 +++++++- js/src/gc/RootMarking.cpp | 31 +------------------------- js/src/jit-test/tests/gc/bug-824321.js | 3 +++ js/src/jsgc.cpp | 2 +- js/src/jsgcinlines.h | 17 -------------- 6 files changed, 43 insertions(+), 49 deletions(-) create mode 100644 js/src/jit-test/tests/gc/bug-824321.js diff --git a/js/src/gc/Heap.h b/js/src/gc/Heap.h index 722af42b1ec1..2c10ef452954 100644 --- a/js/src/gc/Heap.h +++ b/js/src/gc/Heap.h @@ -988,6 +988,36 @@ Cell::isAligned() const } #endif +inline bool +InFreeList(ArenaHeader *aheader, void *thing) +{ + if (!aheader->hasFreeThings()) + return false; + + FreeSpan firstSpan(aheader->getFirstFreeSpan()); + uintptr_t addr = reinterpret_cast(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 */ diff --git a/js/src/gc/Marking.cpp b/js/src/gc/Marking.cpp index 524c88890096..94e3e4c51146 100644 --- a/js/src/gc/Marking.cpp +++ b/js/src/gc/Marking.cpp @@ -144,7 +144,14 @@ CheckMarkedThing(JSTracer *trc, T *thing) 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 * diff --git a/js/src/gc/RootMarking.cpp b/js/src/gc/RootMarking.cpp index 271d34ada51b..54110fc9137d 100644 --- a/js/src/gc/RootMarking.cpp +++ b/js/src/gc/RootMarking.cpp @@ -87,35 +87,6 @@ MarkExactStackRoots(JSTracer *trc) } #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, @@ -240,7 +211,7 @@ MarkIfGCThingWord(JSTracer *trc, uintptr_t w) * 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); diff --git a/js/src/jit-test/tests/gc/bug-824321.js b/js/src/jit-test/tests/gc/bug-824321.js new file mode 100644 index 000000000000..424e2db0ce91 --- /dev/null +++ b/js/src/jit-test/tests/gc/bug-824321.js @@ -0,0 +1,3 @@ +x = "\udada\udada"; +gc(); + diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp index 7fa1e7222ce8..2c52d00dc41a 100644 --- a/js/src/jsgc.cpp +++ b/js/src/jsgc.cpp @@ -267,7 +267,7 @@ ArenaHeader::checkSynchronizedWithFreeList() const * 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); diff --git a/js/src/jsgcinlines.h b/js/src/jsgcinlines.h index 6a425dfdc2e0..c08e7cecd079 100644 --- a/js/src/jsgcinlines.h +++ b/js/src/jsgcinlines.h @@ -485,19 +485,6 @@ class GCCompartmentGroupIter { * in the partially initialized thing. */ -template -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(reinterpret_cast(thing) + 1); - *p = 0; -#endif -} - template inline T * NewGCThing(JSContext *cx, js::gc::AllocKind kind, size_t thingSize) @@ -534,8 +521,6 @@ NewGCThing(JSContext *cx, js::gc::AllocKind kind, size_t thingSize) comp->gcNursery.insertPointer(t); #endif - if (t) - UnpoisonThing(t); return t; } @@ -567,8 +552,6 @@ TryNewGCThing(JSContext *cx, js::gc::AllocKind kind, size_t thingSize) comp->gcNursery.insertPointer(t); #endif - if (t) - UnpoisonThing(t); return t; }