Bug 1624810 - Assert that only JS holders with the approprate flag set can contain pointers to GC things in more than one zone r=mccr8
authorJon Coppeard <jcoppeard@mozilla.com>
Thu, 26 Mar 2020 10:49:20 +0000
changeset 520490 258117b770a56d136c08bcf3ac9a1fbdf2c085ce
parent 520489 f4d11e6a6e3f74cd0f43c2a657b596b1eb4a1e55
child 520491 b9355b941f2f2f2df906c44a85dd32deeea6b762
push id37252
push usermalexandru@mozilla.com
push dateThu, 26 Mar 2020 15:34:27 +0000
treeherdermozilla-central@31360ced8ff8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs1624810
milestone76.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1624810 - Assert that only JS holders with the approprate flag set can contain pointers to GC things in more than one zone r=mccr8 Differential Revision: https://phabricator.services.mozilla.com/D68197
js/src/jsfriendapi.cpp
js/src/jsfriendapi.h
xpcom/base/CycleCollectedJSRuntime.cpp
--- a/js/src/jsfriendapi.cpp
+++ b/js/src/jsfriendapi.cpp
@@ -1459,8 +1459,14 @@ bool js::AddMozDateTimeFormatConstructor
   return IntlNotEnabled(cx);
 }
 
 bool js::AddListFormatConstructor(JSContext* cx, JS::HandleObject intl) {
   return IntlNotEnabled(cx);
 }
 
 #endif  // !JS_HAS_INTL_API
+
+#ifdef DEBUG
+JS_FRIEND_API JS::Zone* js::GetObjectZoneFromAnyThread(JSObject* obj) {
+  return MaybeForwarded(obj)->zoneFromAnyThread();
+}
+#endif
--- a/js/src/jsfriendapi.h
+++ b/js/src/jsfriendapi.h
@@ -2681,11 +2681,15 @@ namespace gc {
 
 enum class PerformanceHint { Normal, InPageLoad };
 
 extern JS_FRIEND_API void SetPerformanceHint(JSContext* cx,
                                              PerformanceHint hint);
 
 } /* namespace gc */
 
+#ifdef DEBUG
+extern JS_FRIEND_API JS::Zone* GetObjectZoneFromAnyThread(JSObject* obj);
+#endif
+
 } /* namespace js */
 
 #endif /* jsfriendapi_h */
--- a/xpcom/base/CycleCollectedJSRuntime.cpp
+++ b/xpcom/base/CycleCollectedJSRuntime.cpp
@@ -1063,22 +1063,136 @@ struct JsGcTracer : public TraceCallback
 };
 
 void mozilla::TraceScriptHolder(nsISupports* aHolder, JSTracer* aTracer) {
   nsXPCOMCycleCollectionParticipant* participant = nullptr;
   CallQueryInterface(aHolder, &participant);
   participant->Trace(aHolder, JsGcTracer(), aTracer);
 }
 
+#ifdef DEBUG
+
+// A tracer that checks that a JS holder only holds JS GC things in a single
+// JS::Zone.
+struct CheckZoneTracer : public TraceCallbacks {
+  const char* mClassName;
+  mutable JS::Zone* mZone = nullptr;
+
+  explicit CheckZoneTracer(const char* aClassName) : mClassName(aClassName) {}
+
+  void checkZone(JS::Zone* aZone, const char* aName) const {
+    if (!mZone) {
+      mZone = aZone;
+      return;
+    }
+
+    if (aZone == mZone) {
+      return;
+    }
+
+    // Most JS holders only contain pointers to GC things in a single zone. In
+    // the future this will allow us to improve GC performance by only tracing
+    // holders in zones that are being collected.
+    //
+    // If you added a holder that has pointers into multiple zones please try to
+    // remedy this. Some options are:
+    //
+    //  - wrap all JS GC things into the same compartment
+    //  - split GC thing pointers between separate cycle collected objects
+    //
+    // If all else fails, flag the class as containing pointers into multiple
+    // zones by using NS_IMPL_CYCLE_COLLECTION_MULTI_ZONE_JSHOLDER_CLASS.
+    MOZ_CRASH_UNSAFE_PRINTF(
+        "JS holder %s contains pointers to GC things in more than one zone ("
+        "found in %s)\n",
+        mClassName, aName);
+  }
+
+  virtual void Trace(JS::Heap<JS::Value>* aPtr, const char* aName,
+                     void* aClosure) const override {
+    JS::Value value = aPtr->unbarrieredGet();
+    if (value.isObject()) {
+      checkZone(js::GetObjectZoneFromAnyThread(&value.toObject()), aName);
+    } else if (value.isString()) {
+      checkZone(JS::GetStringZone(value.toString()), aName);
+    } else if (value.isGCThing()) {
+      checkZone(JS::GetTenuredGCThingZone(value.toGCCellPtr()), aName);
+    }
+  }
+  virtual void Trace(JS::Heap<jsid>* aPtr, const char* aName,
+                     void* aClosure) const override {
+    jsid id = aPtr->unbarrieredGet();
+    if (JSID_IS_GCTHING(id)) {
+      checkZone(JS::GetTenuredGCThingZone(JSID_TO_GCTHING(id)), aName);
+    }
+  }
+  virtual void Trace(JS::Heap<JSObject*>* aPtr, const char* aName,
+                     void* aClosure) const override {
+    JSObject* obj = aPtr->unbarrieredGet();
+    if (obj) {
+      checkZone(js::GetObjectZoneFromAnyThread(obj), aName);
+    }
+  }
+  virtual void Trace(nsWrapperCache* aPtr, const char* aName,
+                     void* aClosure) const override {
+    JSObject* obj = aPtr->GetWrapperPreserveColor();
+    if (obj) {
+      checkZone(js::GetObjectZoneFromAnyThread(obj), aName);
+    }
+  }
+  virtual void Trace(JS::TenuredHeap<JSObject*>* aPtr, const char* aName,
+                     void* aClosure) const override {
+    JSObject* obj = aPtr->unbarrieredGetPtr();
+    if (obj) {
+      checkZone(js::GetObjectZoneFromAnyThread(obj), aName);
+    }
+  }
+  virtual void Trace(JS::Heap<JSString*>* aPtr, const char* aName,
+                     void* aClosure) const override {
+    JSString* str = aPtr->unbarrieredGet();
+    if (str) {
+      checkZone(JS::GetStringZone(str), aName);
+    }
+  }
+  virtual void Trace(JS::Heap<JSScript*>* aPtr, const char* aName,
+                     void* aClosure) const override {
+    JSScript* script = aPtr->unbarrieredGet();
+    if (script) {
+      checkZone(JS::GetTenuredGCThingZone(JS::GCCellPtr(script)), aName);
+    }
+  }
+  virtual void Trace(JS::Heap<JSFunction*>* aPtr, const char* aName,
+                     void* aClosure) const override {
+    JSFunction* fun = aPtr->unbarrieredGet();
+    if (fun) {
+      checkZone(js::GetObjectZoneFromAnyThread(JS_GetFunctionObject(fun)),
+                aName);
+    }
+  }
+};
+
+static inline void CheckHolderIsSingleZone(
+    void* aHolder, nsCycleCollectionParticipant* aParticipant) {
+  CheckZoneTracer tracer(aParticipant->ClassName());
+  aParticipant->Trace(aHolder, tracer, nullptr);
+}
+
+#endif
+
 void CycleCollectedJSRuntime::TraceNativeGrayRoots(JSTracer* aTracer) {
   // NB: This is here just to preserve the existing XPConnect order. I doubt it
   // would hurt to do this after the JS holders.
   TraceAdditionalNativeGrayRoots(aTracer);
 
   mJSHolders.ForEach([aTracer](void* holder, nsScriptObjectTracer* tracer) {
+#ifdef DEBUG
+    if (!tracer->IsMultiZoneJSHolder()) {
+      CheckHolderIsSingleZone(holder, tracer);
+    }
+#endif
     tracer->Trace(holder, JsGcTracer(), aTracer);
   });
 }
 
 void CycleCollectedJSRuntime::AddJSHolder(void* aHolder,
                                           nsScriptObjectTracer* aTracer) {
   mJSHolders.Put(aHolder, aTracer);
 }