Bug 1292529 - Fix assertion used by GCPtr destructor to not assume referent is still allocated r=terrence
authorJon Coppeard <jcoppeard@mozilla.com>
Tue, 09 Aug 2016 10:26:00 +0100
changeset 308817 2b81703d0d0ead8d9d4afcfaeb04b7d7b853ae08
parent 308816 5df7daac885a736f269f831673d8dfe88e7894ff
child 308818 5d5d3ef04f3f77bb95616f56c129256a89f57831
push id20279
push usercbook@mozilla.com
push dateWed, 10 Aug 2016 14:04:43 +0000
treeherderfx-team@531100c1d950 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersterrence
bugs1292529
milestone51.0a1
Bug 1292529 - Fix assertion used by GCPtr destructor to not assume referent is still allocated r=terrence
js/src/gc/Barrier.cpp
js/src/gc/Barrier.h
js/src/gc/GCInternals.h
js/src/vm/Runtime.h
js/src/vm/TaggedProto.h
--- a/js/src/gc/Barrier.cpp
+++ b/js/src/gc/Barrier.cpp
@@ -76,23 +76,16 @@ CurrentThreadIsIonCompilingSafeForMinorG
 }
 
 bool
 CurrentThreadIsGCSweeping()
 {
     return TlsPerThreadData.get()->gcSweeping;
 }
 
-bool
-CurrentThreadCanSkipPostBarrier(bool inNursery)
-{
-    bool onMainThread = TlsPerThreadData.get()->runtimeIfOnOwnerThread() != nullptr;
-    return !onMainThread && !inNursery;
-}
-
 #endif // DEBUG
 
 template <typename S>
 template <typename T>
 void
 ReadBarrierFunctor<S>::operator()(T* t)
 {
     InternalBarrierMethods<T*>::readBarrier(t);
--- a/js/src/gc/Barrier.h
+++ b/js/src/gc/Barrier.h
@@ -236,19 +236,16 @@ class JitCode;
 bool
 CurrentThreadIsIonCompiling();
 
 bool
 CurrentThreadIsIonCompilingSafeForMinorGC();
 
 bool
 CurrentThreadIsGCSweeping();
-
-bool
-CurrentThreadCanSkipPostBarrier(bool inNursery);
 #endif
 
 namespace gc {
 
 // Marking.h depends on these barrier definitions, so we need a separate
 // entry point for marking to implement the pre-barrier.
 void MarkValueForBarrier(JSTracer* trc, Value* v, const char* name);
 void MarkIdForBarrier(JSTracer* trc, jsid* idp, const char* name);
@@ -265,18 +262,16 @@ struct InternalBarrierMethods<T*>
 
     static bool isMarkableTaggedPointer(T* v) { return !IsNullTaggedPointer(v); }
 
     static void preBarrier(T* v) { T::writeBarrierPre(v); }
 
     static void postBarrier(T** vp, T* prev, T* next) { T::writeBarrierPost(vp, prev, next); }
 
     static void readBarrier(T* v) { T::readBarrier(v); }
-
-    static bool isInsideNursery(T* v) { return IsInsideNursery(v); }
 };
 
 template <typename S> struct PreBarrierFunctor : public VoidDefaultAdaptor<S> {
     template <typename T> void operator()(T* t);
 };
 
 template <typename S> struct ReadBarrierFunctor : public VoidDefaultAdaptor<S> {
     template <typename T> void operator()(T* t);
@@ -311,32 +306,26 @@ struct InternalBarrierMethods<Value>
         // Remove the prev entry if the new value does not need it.
         if (prev.isObject() && (sb = reinterpret_cast<gc::Cell*>(&prev.toObject())->storeBuffer()))
             sb->unputValue(vp);
     }
 
     static void readBarrier(const Value& v) {
         DispatchTyped(ReadBarrierFunctor<Value>(), v);
     }
-
-    static bool isInsideNursery(const Value& v) {
-        return v.isMarkable() && IsInsideNursery(v.toGCThing());
-    }
 };
 
 template <>
 struct InternalBarrierMethods<jsid>
 {
-    static bool isMarkable(jsid id) { return JSID_IS_STRING(id) || JSID_IS_SYMBOL(id); }
+    static bool isMarkable(jsid id) { return JSID_IS_GCTHING(id); }
     static bool isMarkableTaggedPointer(jsid id) { return isMarkable(id); }
 
     static void preBarrier(jsid id) { DispatchTyped(PreBarrierFunctor<jsid>(), id); }
     static void postBarrier(jsid* idp, jsid prev, jsid next) {}
-
-    static bool isInsideNursery(jsid id) { return false; }
 };
 
 // Barrier classes can use Mixins to add methods to a set of barrier
 // instantiations, to make the barriered thing look and feel more like the
 // thing itself.
 template <typename T>
 class BarrieredBaseMixins {};
 
@@ -446,20 +435,19 @@ class GCPtr : public WriteBarrieredBase<
         this->post(JS::GCPolicy<T>::initial(), v);
     }
     explicit GCPtr(const GCPtr<T>& v) : WriteBarrieredBase<T>(v) {
         this->post(JS::GCPolicy<T>::initial(), v);
     }
 #ifdef DEBUG
     ~GCPtr() {
         // No prebarrier necessary as this only happens when we are sweeping or
-        // after we have just collected the nursery.
-        bool inNursery = InternalBarrierMethods<T>::isInsideNursery(this->value);
-        MOZ_ASSERT(CurrentThreadIsGCSweeping() ||
-                   CurrentThreadCanSkipPostBarrier(inNursery));
+        // after we have just collected the nursery.  Note that the wrapped
+        // pointer may already have been freed by this point.
+        MOZ_ASSERT(CurrentThreadIsGCSweeping());
         Poison(this, JS_FREED_HEAP_PTR_PATTERN, sizeof(*this));
     }
 #endif
 
     void init(T v) {
         this->value = v;
         this->post(JS::GCPolicy<T>::initial(), v);
     }
--- a/js/src/gc/GCInternals.h
+++ b/js/src/gc/GCInternals.h
@@ -141,39 +141,16 @@ struct MovingTracer : JS::CallbackTracer
         MOZ_ASSERT(!RelocationOverlay::isCellForwarded(thing.asCell()));
     }
 
 #ifdef DEBUG
     TracerKind getTracerKind() const override { return TracerKind::Moving; }
 #endif
 };
 
-// In debug builds, set/unset the GC sweeping flag for the current thread.
-struct MOZ_RAII AutoSetThreadIsSweeping
-{
-#ifdef DEBUG
-    AutoSetThreadIsSweeping()
-      : threadData_(js::TlsPerThreadData.get())
-    {
-        MOZ_ASSERT(!threadData_->gcSweeping);
-        threadData_->gcSweeping = true;
-    }
-
-    ~AutoSetThreadIsSweeping() {
-        MOZ_ASSERT(threadData_->gcSweeping);
-        threadData_->gcSweeping = false;
-    }
-
-  private:
-    PerThreadData* threadData_;
-#else
-    AutoSetThreadIsSweeping() {}
-#endif
-};
-
 // Structure for counting how many times objects in a particular group have
 // been tenured during a minor collection.
 struct TenureCount
 {
     ObjectGroup* group;
     int count;
 };
 
--- a/js/src/vm/Runtime.h
+++ b/js/src/vm/Runtime.h
@@ -1688,36 +1688,69 @@ class MOZ_RAII AutoEnterIonCompilation
         pt->ionCompiling = false;
         pt->ionCompilingSafeForMinorGC = false;
 #endif
     }
 
     MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
 };
 
+namespace gc {
+
+// In debug builds, set/unset the GC sweeping flag for the current thread.
+struct MOZ_RAII AutoSetThreadIsSweeping
+{
+#ifdef DEBUG
+    AutoSetThreadIsSweeping()
+      : threadData_(js::TlsPerThreadData.get())
+    {
+        MOZ_ASSERT(!threadData_->gcSweeping);
+        threadData_->gcSweeping = true;
+    }
+
+    ~AutoSetThreadIsSweeping() {
+        MOZ_ASSERT(threadData_->gcSweeping);
+        threadData_->gcSweeping = false;
+    }
+
+  private:
+    PerThreadData* threadData_;
+#else
+    AutoSetThreadIsSweeping() {}
+#endif
+};
+
+} // namespace gc
+
 /*
  * Provides a delete policy that can be used for objects which have their
  * lifetime managed by the GC and can only safely be destroyed while the nursery
  * is empty.
  *
  * This is necessary when initializing such an object may fail after the initial
  * allocation.  The partially-initialized object must be destroyed, but it may
  * not be safe to do so at the current time.  This policy puts the object on a
  * queue to be destroyed at a safe time.
  */
 template <typename T>
 struct GCManagedDeletePolicy
 {
     void operator()(const T* ptr) {
         if (ptr) {
             JSRuntime* rt = TlsPerThreadData.get()->runtimeIfOnOwnerThread();
-            if (rt)
+            if (rt) {
+                // The object may contain nursery pointers and must only be
+                // destroyed after a minor GC.
                 rt->gc.callAfterMinorGC(deletePtr, const_cast<T*>(ptr));
-            else
+            } else {
+                // The object cannot contain nursery pointers so can be
+                // destroyed immediately.
+                gc::AutoSetThreadIsSweeping threadIsSweeping;
                 js_delete(const_cast<T*>(ptr));
+            }
         }
     }
 
   private:
     static void deletePtr(void* data) {
         js_delete(reinterpret_cast<T*>(data));
     }
 };
--- a/js/src/vm/TaggedProto.h
+++ b/js/src/vm/TaggedProto.h
@@ -70,21 +70,16 @@ struct InternalBarrierMethods<TaggedProt
 
     static bool isMarkableTaggedPointer(TaggedProto proto) {
         return proto.isObject();
     }
 
     static bool isMarkable(TaggedProto proto) {
         return proto.isObject();
     }
-
-    static bool isInsideNursery(TaggedProto proto) {
-        return proto.isObject() &&
-            gc::IsInsideNursery(reinterpret_cast<gc::Cell*>(proto.toObject()));
-    }
 };
 
 template<class Outer>
 class TaggedProtoOperations
 {
     const TaggedProto& value() const {
         return static_cast<const Outer*>(this)->get();
     }