Bug 1013531 - Clarify the naming of the rooting analysis supression guards; r=sfink

--HG--
extra : rebase_source : ce44e4a4428414a132eae7167c485ff366b1f0a6

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);

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)

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;
 }
 

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)

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;

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)

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

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);

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;

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();

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());

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

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

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);
     }
 }

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;

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--) {

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;