Bug 1328251 - Don't mark GC things owned by another runtime r=sfink a=abillings a=gchang
authorJon Coppeard <jcoppeard@mozilla.com>
Wed, 11 Jan 2017 10:33:52 +0000
changeset 462177 3f09b06331843ac1be0e13e8a0a36c845790c710
parent 462176 4eb8ecd6fe80f5f2b5467169388dd673c223389a
child 462178 5fb0834eba17ac8df15f2cbd34805ecada288c48
push id41678
push userfelipc@gmail.com
push dateMon, 16 Jan 2017 20:19:38 +0000
reviewerssfink, abillings, gchang
bugs1328251
milestone51.0
Bug 1328251 - Don't mark GC things owned by another runtime r=sfink a=abillings a=gchang
js/src/gc/Marking.cpp
js/src/jit-test/tests/gc/bug-1324512.js
js/src/jit-test/tests/gc/bug-1328251.js
js/src/jsgc.cpp
--- a/js/src/gc/Marking.cpp
+++ b/js/src/gc/Marking.cpp
@@ -170,16 +170,27 @@ template <> bool ThingIsPermanentAtomOrW
 }
 template <> bool ThingIsPermanentAtomOrWellKnownSymbol<PropertyName>(PropertyName* name) {
     return name->isPermanent();
 }
 template <> bool ThingIsPermanentAtomOrWellKnownSymbol<JS::Symbol>(JS::Symbol* sym) {
     return sym->isWellKnownSymbol();
 }
 
+template <typename T>
+static inline bool
+IsOwnedByOtherRuntime(JSTracer* trc, T thing)
+{
+    bool other = thing->runtimeFromAnyThread() != trc->runtime();
+    MOZ_ASSERT_IF(other,
+                  ThingIsPermanentAtomOrWellKnownSymbol(thing) ||
+                  thing->zoneFromAnyThread()->isSelfHostingZone());
+    return other;
+}
+
 template<typename T>
 void
 js::CheckTracedThing(JSTracer* trc, T* thing)
 {
 #ifdef DEBUG
     MOZ_ASSERT(trc);
     MOZ_ASSERT(thing);
 
@@ -191,20 +202,20 @@ js::CheckTracedThing(JSTracer* trc, T* t
 
     /* This function uses data that's not available in the nursery. */
     if (IsInsideNursery(thing))
         return;
 
     MOZ_ASSERT_IF(!IsMovingTracer(trc) && !trc->isTenuringTracer(), !IsForwarded(thing));
 
     /*
-     * Permanent atoms are not associated with this runtime, but will be
-     * ignored during marking.
+     * Permanent atoms and things in the self-hosting zone are not associated
+     * with this runtime, but will be ignored during marking.
      */
-    if (ThingIsPermanentAtomOrWellKnownSymbol(thing))
+    if (IsOwnedByOtherRuntime(trc, thing))
         return;
 
     Zone* zone = thing->zoneFromAnyThread();
     JSRuntime* rt = trc->runtime();
 
     MOZ_ASSERT_IF(!IsMovingTracer(trc), CurrentThreadCanAccessZone(zone));
     MOZ_ASSERT_IF(!IsMovingTracer(trc), CurrentThreadCanAccessRuntime(rt));
 
@@ -735,67 +746,53 @@ GCMarker::markImplicitEdges(T* thing)
 {
     markImplicitEdgesHelper<typename ImplicitEdgeHolderType<T*>::Type>(thing);
 }
 
 } // namespace js
 
 template <typename T>
 static inline bool
-MustSkipMarking(T thing)
+MustSkipMarking(GCMarker* gcmarker, T thing)
 {
+    // Don't trace things that are owned by another runtime.
+    if (IsOwnedByOtherRuntime(gcmarker, thing))
+        return true;
+
     // Don't mark things outside a zone if we are in a per-zone GC.
     return !thing->zone()->isGCMarking();
 }
 
 template <>
 bool
-MustSkipMarking<JSObject*>(JSObject* obj)
+MustSkipMarking<JSObject*>(GCMarker* gcmarker, JSObject* obj)
 {
+    // Don't trace things that are owned by another runtime.
+    if (IsOwnedByOtherRuntime(gcmarker, obj))
+        return true;
+
     // We may mark a Nursery thing outside the context of the
     // MinorCollectionTracer because of a pre-barrier. The pre-barrier is not
     // needed in this case because we perform a minor collection before each
     // incremental slice.
     if (IsInsideNursery(obj))
         return true;
 
     // Don't mark things outside a zone if we are in a per-zone GC. It is
     // faster to check our own arena, which we can do since we know that
     // the object is tenured.
     return !TenuredCell::fromPointer(obj)->zone()->isGCMarking();
 }
 
-template <>
-bool
-MustSkipMarking<JSString*>(JSString* str)
-{
-    // Don't mark permanent atoms, as they may be associated with another
-    // runtime. Note that traverse() also checks this, but we need to not
-    // run the isGCMarking test from off-main-thread, so have to check it here
-    // too.
-    return str->isPermanentAtom() ||
-           !str->zone()->isGCMarking();
-}
-
-template <>
-bool
-MustSkipMarking<JS::Symbol*>(JS::Symbol* sym)
-{
-    // As for JSString, don't touch a globally owned well-known symbol from
-    // off-main-thread.
-    return sym->isWellKnownSymbol() ||
-           !sym->zone()->isGCMarking();
-}
-
 template <typename T>
 void
 DoMarking(GCMarker* gcmarker, T* thing)
 {
     // Do per-type marking precondition checks.
-    if (MustSkipMarking(thing))
+    if (MustSkipMarking(gcmarker, thing))
         return;
 
     CheckTracedThing(gcmarker, thing);
     gcmarker->traverse(thing);
 
     // Mark the compartment as live.
     SetMaybeAliveFlag(thing);
 }
@@ -812,17 +809,17 @@ DoMarking(GCMarker* gcmarker, T thing)
     DispatchTyped(DoMarkingFunctor<T>(), thing, gcmarker);
 }
 
 template <typename T>
 void
 NoteWeakEdge(GCMarker* gcmarker, T** thingp)
 {
     // Do per-type marking precondition checks.
-    if (MustSkipMarking(*thingp))
+    if (MustSkipMarking(gcmarker, *thingp))
         return;
 
     CheckTracedThing(gcmarker, *thingp);
 
     // If the target is already marked, there's no need to store the edge.
     if (IsMarkedUnbarriered(thingp))
         return;
 
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/gc/bug-1324512.js
@@ -0,0 +1,13 @@
+if (helperThreadCount() === 0)
+    quit();
+
+evalInWorker(`
+    if (!('gczeal' in this))
+        quit();
+    try {
+      gczeal(2,1);
+      throw new Error();
+    } catch (e) {
+      assertEq("" + e, "Error");
+    }
+`);
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/gc/bug-1328251.js
@@ -0,0 +1,12 @@
+if (helperThreadCount() == 0)
+    quit();
+
+evalInWorker(`
+    if (!('gczeal' in this))
+        quit();
+    gczeal(2);
+    for (let i = 0; i < 30; i++) {
+        var a = [1, 2, 3];
+        a.indexOf(1);
+        relazifyFunctions(); }
+`);
--- a/js/src/jsgc.cpp
+++ b/js/src/jsgc.cpp
@@ -2102,65 +2102,65 @@ GCRuntime::relocateArenas(Zone* zone, JS
 
     return true;
 }
 
 void
 MovingTracer::onObjectEdge(JSObject** objp)
 {
     JSObject* obj = *objp;
-    if (IsForwarded(obj))
+    if (obj->runtimeFromAnyThread() == runtime() && IsForwarded(obj))
         *objp = Forwarded(obj);
 }
 
 void
 MovingTracer::onShapeEdge(Shape** shapep)
 {
     Shape* shape = *shapep;
-    if (IsForwarded(shape))
+    if (shape->runtimeFromAnyThread() == runtime() && IsForwarded(shape))
         *shapep = Forwarded(shape);
 }
 
 void
 MovingTracer::onStringEdge(JSString** stringp)
 {
     JSString* string = *stringp;
-    if (IsForwarded(string))
+    if (string->runtimeFromAnyThread() == runtime() && IsForwarded(string))
         *stringp = Forwarded(string);
 }
 
 void
 MovingTracer::onScriptEdge(JSScript** scriptp)
 {
     JSScript* script = *scriptp;
-    if (IsForwarded(script))
+    if (script->runtimeFromAnyThread() == runtime() && IsForwarded(script))
         *scriptp = Forwarded(script);
 }
 
 void
 MovingTracer::onLazyScriptEdge(LazyScript** lazyp)
 {
     LazyScript* lazy = *lazyp;
-    if (IsForwarded(lazy))
+    if (lazy->runtimeFromAnyThread() == runtime() && IsForwarded(lazy))
         *lazyp = Forwarded(lazy);
 }
 
 void
 MovingTracer::onBaseShapeEdge(BaseShape** basep)
 {
     BaseShape* base = *basep;
-    if (IsForwarded(base))
+    if (base->runtimeFromAnyThread() == runtime() && IsForwarded(base))
         *basep = Forwarded(base);
 }
 
 void
 MovingTracer::onScopeEdge(Scope** scopep)
 {
     Scope* scope = *scopep;
-    if (IsForwarded(scope))
+    if (scope->runtimeFromAnyThread() == runtime() && IsForwarded(scope))
         *scopep = Forwarded(scope);
 }
 
 void
 Zone::prepareForCompacting()
 {
     FreeOp* fop = runtimeFromMainThread()->defaultFreeOp();
     discardJitCode(fop);