Bug 1246697 - Use simpler semantics for PersistentRooted<Traceable>; r=sfink

js/public/RootingAPI.h

@@ -934,7 +934,7 @@ MutableHandle<T>::MutableHandle(PersistentRooted<T>* root)
  * These roots can be used in heap-allocated data structures, so they are not
  * associated with any particular JSContext or stack. They are registered with
  * the JSRuntime itself, without locking, so they require a full JSContext to be
- * initialized, not one of its more restricted superclasses.  Initialization may
+ * initialized, not one of its more restricted superclasses. Initialization may
  * take place on construction, or in two phases if the no-argument constructor
  * is called followed by init().
  *

js/src/gc/RootMarking.cpp

@@ -136,7 +136,10 @@ JS_FOR_EACH_TRACEKIND(FINISH_ROOT_LIST)
 #undef FINISH_ROOT_LIST
     FinishPersistentRootedChain<jsid>(heapRoots_[JS::RootKind::Id]);
     FinishPersistentRootedChain<Value>(heapRoots_[JS::RootKind::Value]);
-    FinishPersistentRootedChain<ConcreteTraceable>(heapRoots_[JS::RootKind::Traceable]);
+
+    // Note that we do not finalize the Traceable list as we do not know how to
+    // safely clear memebers. We instead assert that none escape the RootLists.
+    // See the comment on RootLists::~RootLists for details.
 }
 
 inline void

js/src/jsapi-tests/testGCExactRooting.cpp

@@ -95,7 +95,7 @@ BEGIN_TEST(testGCRootedStaticStructInternalStackStorageAugmented)
         bool same;
 
         // Automatic move from stack to heap.
-        JS::PersistentRooted<MyContainer> heap(cx, container);
+        JS::PersistentRooted<MyContainer> heap(rt, container);
 
         // clear prior rooting.
         container.obj() = nullptr;
@@ -125,6 +125,42 @@ BEGIN_TEST(testGCRootedStaticStructInternalStackStorageAugmented)
 }
 END_TEST(testGCRootedStaticStructInternalStackStorageAugmented)
 
+static JS::PersistentRooted<JSObject*> sLongLived;
+BEGIN_TEST(testGCPersistentRootedOutlivesRuntime)
+{
+    JSContext* cx2 = JS_NewContext(rt, 8192);
+    CHECK(cx2);
+
+    sLongLived.init(cx2, JS_NewObject(cx, nullptr));
+    CHECK(sLongLived);
+
+    JS_DestroyContext(cx2);
+    CHECK(!sLongLived);
+
+    return true;
+}
+END_TEST(testGCPersistentRootedOutlivesRuntime)
+
+// Unlike the above, the following test is an example of an invalid usage: for
+// performance and simplicity reasons, PersistentRooted<Traceable> is not
+// allowed to outlive the container it belongs to. The following commented out
+// test can be used to verify that the relevant assertion fires as expected.
+static JS::PersistentRooted<MyContainer> sContainer;
+BEGIN_TEST(testGCPersistentRootedTraceableCannotOutliveRuntime)
+{
+    JS::Rooted<MyContainer> container(cx);
+    container.obj() = JS_NewObject(cx, nullptr);
+    container.str() = JS_NewStringCopyZ(cx, "Hello");
+    sContainer.init(rt, container);
+
+    // Commenting the following line will trigger an assertion that the
+    // PersistentRooted outlives the runtime it is attached to.
+    sContainer.reset();
+
+    return true;
+}
+END_TEST(testGCPersistentRootedTraceableCannotOutliveRuntime)
+
 using MyHashMap = js::GCHashMap<js::Shape*, JSObject*>;
 
 BEGIN_TEST(testGCRootedHashMap)

js/src/jspubtd.h

@@ -125,9 +125,6 @@ typedef void
 (* JSTraceDataOp)(JSTracer* trc, void* data);
 
 namespace js {
-
-void FinishGC(JSRuntime* rt);
-
 namespace gc {
 class AutoTraceSession;
 class StoreBuffer;
@@ -292,6 +289,29 @@ class RootLists
         }
     }
 
+    ~RootLists() {
+        // The semantics of PersistentRooted containing pointers and tagged
+        // pointers are somewhat different from those of PersistentRooted
+        // containing a structure with a trace method. PersistentRooted
+        // containing pointers are allowed to outlive the owning RootLists,
+        // whereas those containing a traceable structure are not.
+        //
+        // The purpose of this feature is to support lazy initialization of
+        // global references for the several places in Gecko that do not have
+        // access to a tighter context, but that still need to refer to GC
+        // pointers. For such pointers, FinishPersistentRootedChains ensures
+        // that the contained references are nulled out when the owning
+        // RootLists dies to prevent UAF errors.
+        //
+        // However, for RootKind::Traceable, we do not know the concrete type
+        // of the held thing, so we simply cannot do this without accruing
+        // extra overhead and complexity for all users for a case that is
+        // unlikely to ever be used in practice. For this reason, the following
+        // assertion disallows usage of PersistentRooted<Traceable> that
+        // outlives the RootLists.
+        MOZ_ASSERT(heapRoots_[JS::RootKind::Traceable].isEmpty());
+    }
+
     void traceStackRoots(JSTracer* trc);
     void checkNoGCRooters();