From 65d353413bee22c001d596ff8a20dde210dcb15c Mon Sep 17 00:00:00 2001 From: Jan de Mooij Date: Thu, 7 Jun 2018 12:18:43 +0200 Subject: [PATCH] Bug 1466458 part 2 - Refactor Realm::enterRealmDepth_ to account for Realms entered from JIT code. r=luke --- js/src/gc/GC.cpp | 29 +++++++++------- js/src/vm/Debugger.cpp | 4 +-- js/src/vm/JSCompartment.cpp | 2 ++ js/src/vm/JSCompartment.h | 67 +++++++++++++++++++++++++++++-------- js/src/vm/JSContext-inl.h | 4 +-- js/src/vm/JSFunction.cpp | 10 ++++-- js/src/vm/Stopwatch.cpp | 4 +-- 7 files changed, 84 insertions(+), 36 deletions(-) diff --git a/js/src/gc/GC.cpp b/js/src/gc/GC.cpp index da30a20f8dc8..e1bb86a3f348 100644 --- a/js/src/gc/GC.cpp +++ b/js/src/gc/GC.cpp @@ -4229,7 +4229,7 @@ ShouldCollectZone(Zone* zone, JS::gcreason::Reason reason) // been collected, then only collect zones containing those compartments. if (reason == JS::gcreason::COMPARTMENT_REVIVED) { for (CompartmentsInZoneIter comp(zone); !comp.done(); comp.next()) { - if (comp->scheduledForDestruction) + if (comp->gcState.scheduledForDestruction) return true; } @@ -4298,14 +4298,17 @@ GCRuntime::prepareZonesForCollection(JS::gcreason::Reason reason, bool* isFullOu bool canAllocateMoreCode = jit::CanLikelyAllocateMoreExecutableMemory(); for (CompartmentsIter c(rt); !c.done(); c.next()) { - c->scheduledForDestruction = false; - c->maybeAlive = false; + c->gcState.scheduledForDestruction = false; + c->gcState.maybeAlive = false; + c->gcState.hasEnteredRealm = false; for (RealmsInCompartmentIter r(c); !r.done(); r.next()) { r->unmark(); if (r->shouldTraceGlobal() || !r->zone()->isGCScheduled()) - c->maybeAlive = true; + c->gcState.maybeAlive = true; if (shouldPreserveJITCode(r, currentTime, reason, canAllocateMoreCode)) r->zone()->setPreservingCode(true); + if (r->hasBeenEnteredIgnoringJit()) + c->gcState.hasEnteredRealm = true; } } @@ -4525,7 +4528,7 @@ GCRuntime::markCompartments() Vector workList; for (CompartmentsIter comp(rt); !comp.done(); comp.next()) { - if (comp->maybeAlive) { + if (comp->gcState.maybeAlive) { if (!workList.append(comp)) return; } @@ -4535,8 +4538,8 @@ GCRuntime::markCompartments() JSCompartment* comp = workList.popCopy(); for (JSCompartment::NonStringWrapperEnum e(comp); !e.empty(); e.popFront()) { JSCompartment* dest = e.front().mutableKey().compartment(); - if (dest && !dest->maybeAlive) { - dest->maybeAlive = true; + if (dest && !dest->gcState.maybeAlive) { + dest->gcState.maybeAlive = true; if (!workList.append(dest)) return; } @@ -4546,9 +4549,9 @@ GCRuntime::markCompartments() /* Set scheduleForDestruction based on maybeAlive. */ for (GCCompartmentsIter comp(rt); !comp.done(); comp.next()) { - MOZ_ASSERT(!comp->scheduledForDestruction); - if (!comp->maybeAlive && !comp->zone()->isAtomsZone()) - comp->scheduledForDestruction = true; + MOZ_ASSERT(!comp->gcState.scheduledForDestruction); + if (!comp->gcState.maybeAlive && !comp->zone()->isAtomsZone()) + comp->gcState.scheduledForDestruction = true; } } @@ -6940,7 +6943,7 @@ GCRuntime::resetIncrementalGC(gc::AbortReason reason, AutoTraceSession& session) marker.reset(); for (CompartmentsIter c(rt); !c.done(); c.next()) - c->scheduledForDestruction = false; + c->gcState.scheduledForDestruction = false; /* Finish sweeping the current sweep group, then abort. */ abortSweepAfterCurrentGroup = true; @@ -7668,7 +7671,7 @@ GCRuntime::shouldRepeatForDeadZone(JS::gcreason::Reason reason) return false; for (CompartmentsIter c(rt); !c.done(); c.next()) { - if (c->scheduledForDestruction) + if (c->gcState.scheduledForDestruction) return true; } @@ -8097,7 +8100,7 @@ GCRuntime::mergeRealms(Realm* source, Realm* target) MOZ_ASSERT(source->creationOptions().mergeable()); MOZ_ASSERT(source->creationOptions().invisibleToDebugger()); - MOZ_ASSERT(!source->hasBeenEntered()); + MOZ_ASSERT(!source->hasBeenEnteredIgnoringJit()); MOZ_ASSERT(source->zone()->compartments().length() == 1); JSContext* cx = rt->mainContextFromOwnThread(); diff --git a/js/src/vm/Debugger.cpp b/js/src/vm/Debugger.cpp index 1e72d6683f04..83eed7c3d2ad 100644 --- a/js/src/vm/Debugger.cpp +++ b/js/src/vm/Debugger.cpp @@ -3752,7 +3752,7 @@ Debugger::addAllGlobalsAsDebuggees(JSContext* cx, unsigned argc, Value* vp) for (RealmsInZoneIter r(zone); !r.done(); r.next()) { if (r == dbg->object->realm() || r->creationOptions().invisibleToDebugger()) continue; - r->compartment()->scheduledForDestruction = false; + r->compartment()->gcState.scheduledForDestruction = false; GlobalObject* global = r->maybeGlobal(); if (global) { Rooted rg(cx, global); @@ -4991,7 +4991,7 @@ Debugger::findAllGlobals(JSContext* cx, unsigned argc, Value* vp) if (r->creationOptions().invisibleToDebugger()) continue; - r->compartment()->scheduledForDestruction = false; + r->compartment()->gcState.scheduledForDestruction = false; GlobalObject* global = r->maybeGlobal(); diff --git a/js/src/vm/JSCompartment.cpp b/js/src/vm/JSCompartment.cpp index 5f694a684463..4403cf30fe4d 100644 --- a/js/src/vm/JSCompartment.cpp +++ b/js/src/vm/JSCompartment.cpp @@ -75,6 +75,8 @@ Realm::Realm(JSCompartment* comp, const JS::RealmOptions& options) Realm::~Realm() { + MOZ_ASSERT(!hasBeenEnteredIgnoringJit()); + // Write the code coverage information in a file. JSRuntime* rt = runtimeFromMainThread(); if (rt->lcovOutput().isEnabled()) diff --git a/js/src/vm/JSCompartment.h b/js/src/vm/JSCompartment.h index 58fc18578fab..f3446e3216fa 100644 --- a/js/src/vm/JSCompartment.h +++ b/js/src/vm/JSCompartment.h @@ -573,12 +573,23 @@ struct JSCompartment void* data = nullptr; - // These flags help us to discover if a compartment that shouldn't be alive - // manages to outlive a GC. Note that these flags have to be on the - // compartment, not the realm, because same-compartment realms can have - // cross-realm pointers without wrappers. - bool scheduledForDestruction = false; - bool maybeAlive = true; + // Fields set and used by the GC. Be careful, may be stale after we return + // to the mutator. + struct { + // These flags help us to discover if a compartment that shouldn't be + // alive manages to outlive a GC. Note that these flags have to be on + // the compartment, not the realm, because same-compartment realms can + // have cross-realm pointers without wrappers. + bool scheduledForDestruction = false; + bool maybeAlive = true; + + // During GC, we may set this to |true| if we entered a realm in this + // compartment. Note that (without a stack walk) we don't know exactly + // *which* realms, because Realm::enterRealmDepthIgnoringJit_ does not + // account for cross-Realm calls in JIT code updating cx->realm_. See + // also the enterRealmDepthIgnoringJit_ comment. + bool hasEnteredRealm = false; + } gcState; JS::Zone* zone() { return zone_; } const JS::Zone* zone() const { return zone_; } @@ -815,7 +826,21 @@ class JS::Realm : public JS::shadow::Realm js::ReadBarriered unmappedArgumentsTemplate_ { nullptr }; js::ReadBarriered iterResultTemplate_ { nullptr }; - unsigned enterRealmDepth_ = 0; + // There are two ways to enter a realm: + // + // (1) AutoRealm (and JSAutoRealm, JS::EnterRealm) + // (2) When calling a cross-realm (but same-compartment) function in JIT + // code. + // + // This field only accounts for (1), to keep the JIT code as simple as + // possible. + // + // An important invariant is that the JIT can only switch to a different + // realm within the same compartment, so whenever that happens there must + // always be a same-compartment realm with enterRealmDepthIgnoringJit_ > 0. + // This lets us set JSCompartment::hasEnteredRealm without walking the + // stack. + unsigned enterRealmDepthIgnoringJit_ = 0; enum { IsDebuggee = 1 << 0, @@ -1019,16 +1044,20 @@ class JS::Realm : public JS::shadow::Realm } void enter() { - enterRealmDepth_++; + enterRealmDepthIgnoringJit_++; } void leave() { - enterRealmDepth_--; + MOZ_ASSERT(enterRealmDepthIgnoringJit_ > 0); + enterRealmDepthIgnoringJit_--; } - bool hasBeenEntered() const { - return enterRealmDepth_ > 0; + bool hasBeenEnteredIgnoringJit() const { + return enterRealmDepthIgnoringJit_ > 0; } bool shouldTraceGlobal() const { - return hasBeenEntered(); + // If we entered this realm in JIT code, there must be a script and + // function on the stack for this realm, so the global will definitely + // be traced and it's safe to return false here. + return hasBeenEnteredIgnoringJit(); } bool hasAllocationMetadataBuilder() const { @@ -1305,8 +1334,18 @@ namespace js { // scheduledForDestruction will be set on the compartment, which will cause // some extra GC activity to try to free the compartment. template inline void SetMaybeAliveFlag(T* thing) {} -template<> inline void SetMaybeAliveFlag(JSObject* thing) {thing->compartment()->maybeAlive = true;} -template<> inline void SetMaybeAliveFlag(JSScript* thing) {thing->compartment()->maybeAlive = true;} + +template<> inline void +SetMaybeAliveFlag(JSObject* thing) +{ + thing->compartment()->gcState.maybeAlive = true; +} + +template<> inline void +SetMaybeAliveFlag(JSScript* thing) +{ + thing->compartment()->gcState.maybeAlive = true; +} } // namespace js diff --git a/js/src/vm/JSContext-inl.h b/js/src/vm/JSContext-inl.h index cefa89650d0e..65eeebad18b3 100644 --- a/js/src/vm/JSContext-inl.h +++ b/js/src/vm/JSContext-inl.h @@ -517,8 +517,8 @@ JSContext::setRealm(JS::Realm* realm) { // Both the current and the new realm should be properly marked as // entered at this point. - MOZ_ASSERT_IF(realm_, realm_->hasBeenEntered()); - MOZ_ASSERT_IF(realm, realm->hasBeenEntered()); + MOZ_ASSERT_IF(realm_, realm_->hasBeenEnteredIgnoringJit()); + MOZ_ASSERT_IF(realm, realm->hasBeenEnteredIgnoringJit()); // This thread must have exclusive access to the zone. MOZ_ASSERT_IF(realm, CurrentThreadCanAccessZone(realm->zone())); diff --git a/js/src/vm/JSFunction.cpp b/js/src/vm/JSFunction.cpp index c22b6721689d..9062c724e858 100644 --- a/js/src/vm/JSFunction.cpp +++ b/js/src/vm/JSFunction.cpp @@ -1679,10 +1679,14 @@ JSFunction::maybeRelazify(JSRuntime* rt) if (!hasScript() || !u.scripted.s.script_) return; - // Don't relazify functions in realms that are active. + // Don't relazify functions in compartments that are active. Realm* realm = this->realm(); - if (realm->hasBeenEntered() && !rt->allowRelazificationForTesting) - return; + if (!rt->allowRelazificationForTesting) { + if (realm->compartment()->gcState.hasEnteredRealm) + return; + + MOZ_ASSERT(!realm->hasBeenEnteredIgnoringJit()); + } // The caller should have checked we're not in the self-hosting zone (it's // shared with worker runtimes so relazifying functions in it will race). diff --git a/js/src/vm/Stopwatch.cpp b/js/src/vm/Stopwatch.cpp index 3d7a7aad695a..96432f407b6f 100644 --- a/js/src/vm/Stopwatch.cpp +++ b/js/src/vm/Stopwatch.cpp @@ -237,7 +237,7 @@ AutoStopwatch::AutoStopwatch(JSContext* cx MOZ_GUARD_OBJECT_NOTIFIER_PARAM_IN_IM MOZ_GUARD_OBJECT_NOTIFIER_INIT; JSCompartment* compartment = cx_->compartment(); - if (MOZ_UNLIKELY(compartment->scheduledForDestruction)) + if (MOZ_UNLIKELY(compartment->gcState.scheduledForDestruction)) return; JSRuntime* runtime = cx_->runtime(); @@ -276,7 +276,7 @@ AutoStopwatch::~AutoStopwatch() } JSCompartment* compartment = cx_->compartment(); - if (MOZ_UNLIKELY(compartment->scheduledForDestruction)) + if (MOZ_UNLIKELY(compartment->gcState.scheduledForDestruction)) return; JSRuntime* runtime = cx_->runtime();