diff --git a/dom/bindings/CallbackObject.cpp b/dom/bindings/CallbackObject.cpp index 02a5db0cdec..e907c38c5f0 100644 --- a/dom/bindings/CallbackObject.cpp +++ b/dom/bindings/CallbackObject.cpp @@ -81,7 +81,10 @@ CallbackObject::CallSetup::CallSetup(CallbackObject* aCallback, nsIGlobalObject* globalObject = nullptr; { - JS::AutoAssertNoGC nogc; + // Bug 955660: we cannot do "proper" rooting here because we need the + // global to get a context. Everything here is simple getters that cannot + // GC, so just paper over the necessary dataflow inversion. + JS::AutoSuppressGCAnalysis nogc; if (mIsMainThread) { // Now get the global and JSContext for this callback. nsGlobalWindow* win = xpc::WindowGlobalOrNull(realCallback); diff --git a/js/public/GCAPI.h b/js/public/GCAPI.h index 9abc1f0cab7..fb948ea7311 100644 --- a/js/public/GCAPI.h +++ b/js/public/GCAPI.h @@ -369,9 +369,27 @@ extern JS_FRIEND_API(void) ShrinkGCBuffers(JSRuntime *rt); /* - * Assert if any GC occured while this guard object was live. This is most - * useful to help the exact rooting hazard analysis in complex regions, since - * it cannot understand dataflow. + * Assert if a GC occurs while this class is live. This class does not disable + * the static rooting hazard analysis. + */ +class JS_PUBLIC_API(AutoAssertOnGC) +{ + JSRuntime *runtime; + size_t gcNumber; + + public: + AutoAssertOnGC(); + explicit AutoAssertOnGC(JSRuntime *rt); + ~AutoAssertOnGC(); + + static void VerifyIsSafeToGC(JSRuntime *rt); +}; + +/* + * Disable the static rooting hazard analysis in the live region, but assert if + * any GC occurs while this guard object is live. This is most useful to help + * the exact rooting hazard analysis in complex regions, since it cannot + * understand dataflow. * * Note: GC behavior is unpredictable even when deterministice and is generally * non-deterministic in practice. The fact that this guard has not @@ -381,22 +399,28 @@ ShrinkGCBuffers(JSRuntime *rt); * that the hazard analysis is correct for that code, rather than relying * on this class. */ -class JS_PUBLIC_API(AutoAssertNoGC) +class JS_PUBLIC_API(AutoSuppressGCAnalysis) : public AutoAssertOnGC { -#ifdef JS_DEBUG - JSRuntime *runtime; - size_t gcNumber; + public: + AutoSuppressGCAnalysis() : AutoAssertOnGC() {} + explicit AutoSuppressGCAnalysis(JSRuntime *rt) : AutoAssertOnGC(rt) {} +}; +/* + * Place AutoCheckCannotGC in scopes that you believe can never GC. These + * annotations will be verified both dynamically via AutoAssertOnGC, and + * statically with the rooting hazard analysis (implemented by making the + * analysis consider AutoCheckCannotGC to be a GC pointer, and therefore + * complain if it is live across a GC call.) It is useful when dealing with + * internal pointers to GC things where the GC thing itself may not be present + * for the static analysis: e.g. acquiring inline chars from a JSString* on the + * heap. + */ +class JS_PUBLIC_API(AutoCheckCannotGC) : public AutoAssertOnGC +{ public: - AutoAssertNoGC(); - AutoAssertNoGC(JSRuntime *rt); - ~AutoAssertNoGC(); -#else - public: - /* Prevent unreferenced local warnings in opt builds. */ - AutoAssertNoGC() {} - explicit AutoAssertNoGC(JSRuntime *) {} -#endif + AutoCheckCannotGC() : AutoAssertOnGC() {} + explicit AutoCheckCannotGC(JSRuntime *rt) : AutoAssertOnGC(rt) {} }; class JS_PUBLIC_API(ObjectPtr) diff --git a/js/src/devtools/rootAnalysis/annotations.js b/js/src/devtools/rootAnalysis/annotations.js index 62affcbf136..4daf208b924 100644 --- a/js/src/devtools/rootAnalysis/annotations.js +++ b/js/src/devtools/rootAnalysis/annotations.js @@ -138,7 +138,7 @@ var ignoreFunctions = { "PR_ExplodeTime" : true, "PR_ErrorInstallTable" : true, "PR_SetThreadPrivate" : true, - "JSObject* js::GetWeakmapKeyDelegate(JSObject*)" : true, // FIXME: mark with AutoAssertNoGC instead + "JSObject* js::GetWeakmapKeyDelegate(JSObject*)" : true, // FIXME: mark with AutoSuppressGCAnalysis instead "uint8 NS_IsMainThread()" : true, // FIXME! @@ -161,7 +161,7 @@ var ignoreFunctions = { // Bug 948646 - the only thing AutoJSContext's constructor calls // is an Init() routine whose entire body is covered with an - // AutoAssertNoGC. AutoSafeJSContext is the same thing, just with + // AutoSuppressGCAnalysis. AutoSafeJSContext is the same thing, just with // a different value for the 'aSafe' parameter. "void mozilla::AutoJSContext::AutoJSContext(mozilla::detail::GuardObjectNotifier*)" : true, "void mozilla::AutoSafeJSContext::~AutoSafeJSContext(int32)" : true, @@ -230,7 +230,7 @@ function isSuppressConstructor(name) { return name.indexOf("::AutoSuppressGC") != -1 || name.indexOf("::AutoEnterAnalysis") != -1 - || name.indexOf("::AutoAssertNoGC") != -1 + || name.indexOf("::AutoSuppressGCAnalysis") != -1 || name.indexOf("::AutoIgnoreRootingHazards") != -1; } diff --git a/js/src/devtools/rootAnalysis/computeGCTypes.js b/js/src/devtools/rootAnalysis/computeGCTypes.js index f1f540d6e44..018ca628cb2 100644 --- a/js/src/devtools/rootAnalysis/computeGCTypes.js +++ b/js/src/devtools/rootAnalysis/computeGCTypes.js @@ -133,6 +133,7 @@ addGCType('js::LazyScript'); addGCType('js::ion::IonCode'); addGCPointer('JS::Value'); addGCPointer('jsid'); +addGCPointer('JS::AutoCheckCannotGC'); function explain(csu, indent, seen) { if (!seen) diff --git a/js/src/gc/GCRuntime.h b/js/src/gc/GCRuntime.h index 71cc84be07a..0b6e918a908 100644 --- a/js/src/gc/GCRuntime.h +++ b/js/src/gc/GCRuntime.h @@ -537,6 +537,14 @@ class GCRuntime /* Strong references on scripts held for PCCount profiling API. */ js::ScriptAndCountsVector *scriptAndCountsVector; + /* + * Some regions of code are hard for the static rooting hazard analysis to + * understand. In those cases, we trade the static analysis for a dynamic + * analysis. When this is non-zero, we should assert if we trigger, or + * might trigger, a GC. + */ + int inUnsafeRegion; + private: /* Always preserve JIT code during GCs, for testing. */ bool alwaysPreserveCode; diff --git a/js/src/jsapi-tests/testGCExactRooting.cpp b/js/src/jsapi-tests/testGCExactRooting.cpp index 24920d4f2a4..569d84332b0 100644 --- a/js/src/jsapi-tests/testGCExactRooting.cpp +++ b/js/src/jsapi-tests/testGCExactRooting.cpp @@ -21,3 +21,17 @@ BEGIN_TEST(testGCExactRooting) return true; } END_TEST(testGCExactRooting) + +BEGIN_TEST(testGCSuppressions) +{ + JS::AutoAssertOnGC nogc; + JS::AutoCheckCannotGC checkgc; + JS::AutoSuppressGCAnalysis noanalysis; + + JS::AutoAssertOnGC nogcRt(cx->runtime()); + JS::AutoCheckCannotGC checkgcRt(cx->runtime()); + JS::AutoSuppressGCAnalysis noanalysisRt(cx->runtime()); + + return true; +} +END_TEST(testGCSuppressions) diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp index 0bf5ae78e0d..03719c13074 100644 --- a/js/src/jsgc.cpp +++ b/js/src/jsgc.cpp @@ -188,6 +188,7 @@ #include "jscntxt.h" #include "jscompartment.h" #include "jsobj.h" +#include "jsprf.h" #include "jsscript.h" #include "jstypes.h" #include "jsutil.h" @@ -1114,13 +1115,14 @@ GCRuntime::GCRuntime(JSRuntime *rt) : mallocBytes(0), mallocGCTriggered(false), scriptAndCountsVector(nullptr), + helperState(rt), alwaysPreserveCode(false), #ifdef DEBUG noGCOrAllocationCheck(0), #endif lock(nullptr), lockOwner(nullptr), - helperState(rt) + inUnsafeRegion(0) { } @@ -4332,6 +4334,9 @@ AutoGCSession::AutoGCSession(GCRuntime *gc) // It's ok if threads other than the main thread have suppressGC set, as // they are operating on zones which will not be collected from here. JS_ASSERT(!gc->rt->mainThread.suppressGC); + + // Assert if this is a GC unsafe region. + JS::AutoAssertOnGC::VerifyIsSafeToGC(gc->rt); } AutoGCSession::~AutoGCSession() @@ -5621,32 +5626,50 @@ JS::GetGCNumber() return 0; return rt->gc.number; } +#endif -JS::AutoAssertNoGC::AutoAssertNoGC() +JS::AutoAssertOnGC::AutoAssertOnGC() : runtime(nullptr), gcNumber(0) { js::PerThreadData *data = js::TlsPerThreadData.get(); if (data) { /* * GC's from off-thread will always assert, so off-thread is implicitly - * AutoAssertNoGC. We still need to allow AutoAssertNoGC to be used in + * AutoAssertOnGC. We still need to allow AutoAssertOnGC to be used in * code that works from both threads, however. We also use this to * annotate the off thread run loops. */ runtime = data->runtimeIfOnOwnerThread(); - if (runtime) + if (runtime) { gcNumber = runtime->gc.number; + ++runtime->gc.inUnsafeRegion; + } } } -JS::AutoAssertNoGC::AutoAssertNoGC(JSRuntime *rt) +JS::AutoAssertOnGC::AutoAssertOnGC(JSRuntime *rt) : runtime(rt), gcNumber(rt->gc.number) { + ++rt->gc.inUnsafeRegion; } -JS::AutoAssertNoGC::~AutoAssertNoGC() +JS::AutoAssertOnGC::~AutoAssertOnGC() { - if (runtime) - MOZ_ASSERT(gcNumber == runtime->gc.number, "GC ran inside an AutoAssertNoGC scope."); + if (runtime) { + --runtime->gc.inUnsafeRegion; + MOZ_ASSERT(runtime->gc.inUnsafeRegion >= 0); + + /* + * The following backstop assertion should never fire: if we bumped the + * gcNumber, we should have asserted because inUnsafeRegion was true. + */ + MOZ_ASSERT(gcNumber == runtime->gc.number, "GC ran inside an AutoAssertOnGC scope."); + } +} + +/* static */ void +JS::AutoAssertOnGC::VerifyIsSafeToGC(JSRuntime *rt) +{ + if (rt->gc.inUnsafeRegion > 0) + MOZ_CRASH("[AutoAssertOnGC] possible GC in GC-unsafe region"); } -#endif diff --git a/js/src/jsgcinlines.h b/js/src/jsgcinlines.h index 5c291cc8621..ecf2fa9f010 100644 --- a/js/src/jsgcinlines.h +++ b/js/src/jsgcinlines.h @@ -504,6 +504,10 @@ CheckAllocatorState(ThreadSafeContext *cx, AllocKind kind) JS_ASSERT(rt->gc.isAllocAllowed()); #endif + // Crash if we perform a GC action when it is not safe. + if (allowGC && !rt->mainThread.suppressGC) + JS::AutoAssertOnGC::VerifyIsSafeToGC(rt); + // For testing out of memory conditions if (!PossiblyFail()) { js_ReportOutOfMemory(cx); diff --git a/js/src/jsobj.h b/js/src/jsobj.h index 443c6a8879f..35f24f40c06 100644 --- a/js/src/jsobj.h +++ b/js/src/jsobj.h @@ -1203,7 +1203,7 @@ inline JSObject * GetInnerObject(JSObject *obj) { if (js::InnerObjectOp op = obj->getClass()->ext.innerObject) { - JS::AutoAssertNoGC nogc; + JS::AutoSuppressGCAnalysis nogc; return op(obj); } return obj; diff --git a/js/src/jsweakmap.cpp b/js/src/jsweakmap.cpp index 968a97d6bb6..ec39df76429 100644 --- a/js/src/jsweakmap.cpp +++ b/js/src/jsweakmap.cpp @@ -165,7 +165,7 @@ ObjectValueMap::findZoneEdges() * edge to ensure that the delegate zone does finish marking after the key * zone. */ - JS::AutoAssertNoGC nogc; + JS::AutoSuppressGCAnalysis nogc; Zone *mapZone = compartment->zone(); for (Range r = all(); !r.empty(); r.popFront()) { JSObject *key = r.front().key(); diff --git a/js/src/jsworkers.cpp b/js/src/jsworkers.cpp index 5b7c24d5178..b9124f6b861 100644 --- a/js/src/jsworkers.cpp +++ b/js/src/jsworkers.cpp @@ -1047,7 +1047,7 @@ WorkerThread::handleGCHelperWorkload() void WorkerThread::threadLoop() { - JS::AutoAssertNoGC nogc; + JS::AutoSuppressGCAnalysis nogc; AutoLockWorkerThreadState lock; js::TlsPerThreadData.set(threadData.addr()); diff --git a/js/src/vm/ForkJoin.cpp b/js/src/vm/ForkJoin.cpp index 24f932f1c75..4b618aee9d0 100644 --- a/js/src/vm/ForkJoin.cpp +++ b/js/src/vm/ForkJoin.cpp @@ -1503,15 +1503,15 @@ ForkJoinShared::executePortion(PerThreadData *perThread, ThreadPoolWorker *worke // WARNING: This code runs ON THE PARALLEL WORKER THREAD. // Be careful when accessing cx_. - // ForkJoinContext already contains an AutoAssertNoGC; however, the analysis - // does not propagate this type information. We duplicate the assertion - // here for maximum clarity. - JS::AutoAssertNoGC nogc(runtime()); - Allocator *allocator = allocators_[worker->id()]; ForkJoinContext cx(perThread, worker, allocator, this, &records_[worker->id()]); AutoSetForkJoinContext autoContext(&cx); + // ForkJoinContext already contains an AutoSuppressGCAnalysis; however, the + // analysis does not propagate this type information. We duplicate the + // assertion here for maximum clarity. + JS::AutoSuppressGCAnalysis nogc; + #ifdef DEBUG // Set the maximum worker and slice number for prettier spewing. cx.maxWorkerId = threadPool_->numWorkers(); @@ -1622,7 +1622,7 @@ ForkJoinContext::ForkJoinContext(PerThreadData *perThreadData, ThreadPoolWorker shared_(shared), worker_(worker), acquiredJSContext_(false), - nogc_(shared->runtime()) + nogc_() { /* * Unsafely set the zone. This is used to track malloc counters and to diff --git a/js/src/vm/ForkJoin.h b/js/src/vm/ForkJoin.h index fac1a93cfef..cf77a7ea89d 100644 --- a/js/src/vm/ForkJoin.h +++ b/js/src/vm/ForkJoin.h @@ -426,7 +426,7 @@ class ForkJoinContext : public ThreadSafeContext // ForkJoinContext is allocated on the stack. It would be dangerous to GC // with it live because of the GC pointer fields stored in the context. - JS::AutoAssertNoGC nogc_; + JS::AutoSuppressGCAnalysis nogc_; }; // Locks a JSContext for its scope. Be very careful, because locking a diff --git a/js/src/vm/SPSProfiler.cpp b/js/src/vm/SPSProfiler.cpp index ea78760ab67..f9e8a12da24 100644 --- a/js/src/vm/SPSProfiler.cpp +++ b/js/src/vm/SPSProfiler.cpp @@ -143,7 +143,7 @@ SPSProfiler::markEvent(const char *event) { JS_ASSERT(enabled()); if (eventMarker_) { - JS::AutoAssertNoGC nogc; + JS::AutoSuppressGCAnalysis nogc; eventMarker_(event); } } diff --git a/js/src/vm/Stack.cpp b/js/src/vm/Stack.cpp index cd24c7d9a73..ee4a3990ecf 100644 --- a/js/src/vm/Stack.cpp +++ b/js/src/vm/Stack.cpp @@ -559,7 +559,7 @@ FrameIter::settleOnActivation() if (data_.principals_) { JSContext *cx = data_.cx_->asJSContext(); if (JSSubsumesOp subsumes = cx->runtime()->securityCallbacks->subsumes) { - JS::AutoAssertNoGC nogc; + JS::AutoSuppressGCAnalysis nogc; if (!subsumes(data_.principals_, activation->compartment()->principals)) { ++data_.activations_; continue; diff --git a/js/src/vm/StructuredClone.cpp b/js/src/vm/StructuredClone.cpp index d351f2923d7..7d9ba4da365 100644 --- a/js/src/vm/StructuredClone.cpp +++ b/js/src/vm/StructuredClone.cpp @@ -370,7 +370,7 @@ Discard(uint64_t *buffer, size_t nbytes, const JSStructuredCloneCallbacks *cb, v return; // freeTransfer should not GC - JS::AutoAssertNoGC nogc; + JS::AutoSuppressGCAnalysis nogc; uint64_t numTransferables = LittleEndian::readUint64(point++); while (numTransferables--) { diff --git a/js/xpconnect/src/nsCxPusher.cpp b/js/xpconnect/src/nsCxPusher.cpp index 202115104b1..2747ac0ec60 100644 --- a/js/xpconnect/src/nsCxPusher.cpp +++ b/js/xpconnect/src/nsCxPusher.cpp @@ -183,7 +183,7 @@ AutoJSContext::AutoJSContext(bool aSafe MOZ_GUARD_OBJECT_NOTIFIER_PARAM_IN_IMPL) void AutoJSContext::Init(bool aSafe MOZ_GUARD_OBJECT_NOTIFIER_PARAM_IN_IMPL) { - JS::AutoAssertNoGC nogc; + JS::AutoSuppressGCAnalysis nogc; MOZ_ASSERT(!mCx, "mCx should not be initialized!"); MOZ_GUARD_OBJECT_NOTIFIER_INIT;