Revert "Bug 1844878 - Eliminate redundant addProperty calls for wrapper preservation r=smaug,jandem" for causing multiple failures

This reverts commit 2c63c9b4d8.

dom/bindings/BindingUtils.cpp

@@ -1290,6 +1290,9 @@ bool TryPreserveWrapper(JS::Handle<JSObject*> obj) {
     return true;
   }
 
+  // The addProperty hook for WebIDL classes does wrapper preservation, and
+  // nothing else, so call it, if present.
+
   const JSClass* clasp = JS::GetClass(obj);
   const DOMJSClass* domClass = GetDOMClass(clasp);
 
@@ -1297,23 +1300,17 @@ bool TryPreserveWrapper(JS::Handle<JSObject*> obj) {
   MOZ_RELEASE_ASSERT(clasp->isNativeObject(),
                      "Should not call addProperty for proxies.");
 
-  if (!clasp->preservesWrapper()) {
+  JSAddPropertyOp addProperty = clasp->getAddProperty();
+  if (!addProperty) {
     return true;
   }
 
-  WrapperCacheGetter getter = domClass->mWrapperCacheGetter;
+  // The class should have an addProperty hook iff it is a CC participant.
-  MOZ_RELEASE_ASSERT(getter);
+  MOZ_RELEASE_ASSERT(domClass->mParticipant);
 
-  nsWrapperCache* cache = getter(obj);
+  JS::Rooted<jsid> dummyId(RootingCx());
-  // We obviously can't preserve if we're not initialized, we don't want
+  JS::Rooted<JS::Value> dummyValue(RootingCx());
-  // to preserve if we don't have a wrapper or if the object is in the
+  return addProperty(nullptr, obj, dummyId, dummyValue);
-  // process of being finalized.
-  if (cache && cache->GetWrapperPreserveColor()) {
-    cache->PreserveWrapper(
-        cache, reinterpret_cast<nsScriptObjectTracer*>(domClass->mParticipant));
-  }
-
-  return true;
 }
 
 bool HasReleasedWrapper(JS::Handle<JSObject*> obj) {
@@ -2443,7 +2440,6 @@ void UpdateReflectorGlobal(JSContext* aCx, JS::Handle<JSObject*> aObjArg,
   // propertyHolder. Otherwise, an object with |foo.x === foo| will
   // crash when JS_CopyOwnPropertiesAndPrivateFields tries to call wrap() on
   // foo.x.
-  static_assert(DOM_OBJECT_SLOT == JS_OBJECT_WRAPPER_SLOT);
   JS::SetReservedSlot(newobj, DOM_OBJECT_SLOT,
                       JS::GetReservedSlot(aObj, DOM_OBJECT_SLOT));
   JS::SetReservedSlot(aObj, DOM_OBJECT_SLOT, JS::PrivateValue(nullptr));

dom/bindings/BindingUtils.h

@@ -1029,22 +1029,55 @@ using ToWrapperCacheHelper = std::conditional_t<
     CastableToWrapperCache<CastableToWrapperCacheHelper::OffsetOf<T>()>,
     NeedsQIToWrapperCache>;
 
+template <class Base>
+class NativeTypeHelpersBase_nsISupports : public Base {
+ public:
+  static bool AddProperty(JSContext* cx, JS::Handle<JSObject*> aObj,
+                          JS::Handle<jsid>, JS::Handle<JS::Value>) {
+    nsISupports* self =
+        UnwrapPossiblyNotInitializedDOMObject<nsISupports>(aObj);
+    // We obviously can't preserve if we're not initialized.
+    if (self) {
+      nsWrapperCache* cache = Base::GetWrapperCache(self);
+      // We don't want to preserve if we don't have a wrapper.
+      if (cache->GetWrapperPreserveColor()) {
+        cache->PreserveWrapper(self);
+      }
+    }
+    return true;
+  }
+};
+
 template <class T,
           bool CastableToWrapperCache = std::is_base_of_v<nsWrapperCache, T>>
 class NativeTypeHelpers_nsISupports;
 
 template <class T>
 class NativeTypeHelpers_nsISupports<T, true>
-    : public CastableToWrapperCache<
+    : public NativeTypeHelpersBase_nsISupports<
-          CastableToWrapperCacheHelper::OffsetOf<T>()> {};
+          CastableToWrapperCache<CastableToWrapperCacheHelper::OffsetOf<T>()>> {
+};
 
 template <class T>
-class NativeTypeHelpers_nsISupports<T, false> : public NeedsQIToWrapperCache {};
+class NativeTypeHelpers_nsISupports<T, false>
+    : public NativeTypeHelpersBase_nsISupports<NeedsQIToWrapperCache> {};
 
 template <class T>
 class NativeTypeHelpers_Other
     : public CastableToWrapperCache<
-          CastableToWrapperCacheHelper::OffsetOf<T>()> {};
+          CastableToWrapperCacheHelper::OffsetOf<T>()> {
+ public:
+  static bool AddProperty(JSContext* cx, JS::Handle<JSObject*> aObj,
+                          JS::Handle<jsid>, JS::Handle<JS::Value>) {
+    T* self = UnwrapPossiblyNotInitializedDOMObject<T>(aObj);
+    // We obviously can't preserve if we're not initialized, and we don't want
+    // to preserve if we don't have a wrapper.
+    if (self && self->GetWrapperPreserveColor()) {
+      self->PreserveWrapper(self, NS_CYCLE_COLLECTION_PARTICIPANT(T));
+    }
+    return true;
+  }
+};
 
 template <class T>
 using NativeTypeHelpers = std::conditional_t<std::is_base_of_v<nsISupports, T>,

dom/bindings/Codegen.py

@@ -251,9 +251,9 @@ def idlTypeNeedsCallContext(type, descriptor=None, allowTreatNonCallableAsNull=F
 
 
 # TryPreserveWrapper uses the addProperty hook to preserve the wrapper of
-# non-nsISupports cycle collected objects, so if wantsPreservedWrapper is changed
+# non-nsISupports cycle collected objects, so if wantsAddProperty is changed
 # to not cover that case then TryPreserveWrapper will need to be changed.
-def wantsPreservedWrapper(desc):
+def wantsAddProperty(desc):
     return desc.concrete and desc.wrapperCache and not desc.isGlobal()
 
 
@@ -697,9 +697,6 @@ class CGDOMJSClass(CGThing):
                 )
             classFlags += " | JSCLASS_SKIP_NURSERY_FINALIZE"
 
-        if wantsPreservedWrapper(self.descriptor):
-            classFlags += " | JSCLASS_PRESERVES_WRAPPER"
-
         if self.descriptor.interface.getExtendedAttribute("NeedResolve"):
             resolveHook = RESOLVE_HOOK_NAME
             mayResolveHook = MAY_RESOLVE_HOOK_NAME
@@ -716,7 +713,7 @@ class CGDOMJSClass(CGThing):
         return fill(
             """
             static const JSClassOps sClassOps = {
-              nullptr,               /* addProperty */
+              ${addProperty}, /* addProperty */
               nullptr,               /* delProperty */
               nullptr,               /* enumerate */
               ${newEnumerate}, /* newEnumerate */
@@ -745,6 +742,11 @@ class CGDOMJSClass(CGThing):
             """,
             name=self.descriptor.interface.getClassName(),
             flags=classFlags,
+            addProperty=(
+                "NativeTypeHelpers<%s>::AddProperty" % self.descriptor.nativeType
+                if wantsAddProperty(self.descriptor)
+                else "nullptr"
+            ),
             newEnumerate=newEnumerateHook,
             resolve=resolveHook,
             mayResolve=mayResolveHook,
@@ -2038,6 +2040,40 @@ class CGAbstractClassHook(CGAbstractStaticMethod):
         assert False  # Override me!
 
 
+class CGAddPropertyHook(CGAbstractClassHook):
+    """
+    A hook for addProperty, used to preserve our wrapper from GC.
+    """
+
+    def __init__(self, descriptor):
+        args = [
+            Argument("JSContext*", "cx"),
+            Argument("JS::Handle<JSObject*>", "obj"),
+            Argument("JS::Handle<jsid>", "id"),
+            Argument("JS::Handle<JS::Value>", "val"),
+        ]
+        CGAbstractClassHook.__init__(
+            self, descriptor, ADDPROPERTY_HOOK_NAME, "bool", args
+        )
+
+    def generate_code(self):
+        assert self.descriptor.wrapperCache
+        # This hook is also called by TryPreserveWrapper on non-nsISupports
+        # cycle collected objects, so if addProperty is ever changed to do
+        # anything more or less than preserve the wrapper, TryPreserveWrapper
+        # will need to be changed.
+        return dedent(
+            """
+            // We don't want to preserve if we don't have a wrapper, and we
+            // obviously can't preserve if we're not initialized.
+            if (self && self->GetWrapperPreserveColor()) {
+              PreserveWrapper(self);
+            }
+            return true;
+            """
+        )
+
+
 class CGGetWrapperCacheHook(CGAbstractClassHook):
     """
     A hook for GetWrapperCache, used by HasReleasedWrapper to get the

js/public/Class.h

@@ -489,11 +489,7 @@ static constexpr const js::ObjectOps* JS_NULL_OBJECT_OPS = nullptr;
 
 // Classes, objects, and properties.
 
-// Must call a callback when the first property is added to an object of this
+// (1 << 0 is unused)
-// class. If this is set, the object must store a pointer at
-// JS_OBJECT_WRAPPER_SLOT to the C++ wrapper as a PrivateValue or
-// UndefinedValue() if the object does not have a wrapper.
-static const uint32_t JSCLASS_PRESERVES_WRAPPER = 1 << 0;
 
 // Class's initialization code will call `SetNewObjectMetadata` itself.
 static const uint32_t JSCLASS_DELAY_METADATA_BUILDER = 1 << 1;
@@ -597,9 +593,6 @@ static constexpr uint32_t JSCLASS_HAS_CACHED_PROTO(JSProtoKey key) {
   return uint32_t(key) << JSCLASS_CACHED_PROTO_SHIFT;
 }
 
-// See JSCLASS_PRESERVES_WRAPPER.
-static constexpr size_t JS_OBJECT_WRAPPER_SLOT = 0;
-
 struct MOZ_STATIC_CLASS JSClassOps {
   /* Function pointer members (may be null). */
   JSAddPropertyOp addProperty;
@@ -713,8 +706,6 @@ struct alignas(js::gc::JSClassAlignBytes) JSClass {
 
   bool slot0IsISupports() const { return flags & JSCLASS_SLOT0_IS_NSISUPPORTS; }
 
-  bool preservesWrapper() const { return flags & JSCLASS_PRESERVES_WRAPPER; }
-
   static size_t offsetOfFlags() { return offsetof(JSClass, flags); }
 
   // Internal / friend API accessors:

js/src/jit/CacheIR.cpp

@@ -4963,8 +4963,7 @@ static bool CanAttachAddElement(NativeObject* obj, bool isInit,
     const JSClass* clasp = obj->getClass();
     if (clasp != &ArrayObject::class_ &&
         (clasp->getAddProperty() || clasp->getResolve() ||
-         clasp->getOpsLookupProperty() || clasp->getOpsSetProperty() ||
+         clasp->getOpsLookupProperty() || clasp->getOpsSetProperty())) {
-         obj->hasUnpreservedWrapper())) {
       return false;
     }
 
@@ -5736,10 +5735,7 @@ AttachDecision SetPropIRGenerator::tryAttachAddSlotStub(
   DebugOnly<uint32_t> index;
   MOZ_ASSERT_IF(obj->is<ArrayObject>(), !IdIsIndex(id, &index));
   bool mustCallAddPropertyHook =
-      !obj->is<ArrayObject>() &&
+      obj->getClass()->getAddProperty() && !obj->is<ArrayObject>();
-      (obj->getClass()->getAddProperty() ||
-       (obj->getClass()->preservesWrapper() &&
-        !oldShape->hasObjectFlag(ObjectFlag::HasPreservedWrapper)));
 
   if (mustCallAddPropertyHook) {
     writer.addSlotAndCallAddPropHook(objId, rhsValId, newShape);

js/src/shell/js.cpp

@@ -11312,11 +11312,11 @@ static const JSClassOps FakeDOMObjectClassOps = {
     nullptr,
 };
 
-static const JSClass dom_class = {
+static const JSClass dom_class = {"FakeDOMObject",
-    "FakeDOMObject",
+                                  JSCLASS_IS_DOMJSCLASS |
-    JSCLASS_IS_DOMJSCLASS | JSCLASS_HAS_RESERVED_SLOTS(2) |
+                                      JSCLASS_HAS_RESERVED_SLOTS(2) |
-        JSCLASS_BACKGROUND_FINALIZE | JSCLASS_PRESERVES_WRAPPER,
+                                      JSCLASS_BACKGROUND_FINALIZE,
-    &FakeDOMObjectClassOps};
+                                  &FakeDOMObjectClassOps};
 
 static const JSClass* GetDomClass() { return &dom_class; }
 

js/src/vm/JSObject-inl.h

@@ -550,10 +550,7 @@ inline bool IsConstructor(const Value& v) {
 }
 
 static inline bool MaybePreserveDOMWrapper(JSContext* cx, HandleObject obj) {
-  if (!obj->getClass()->preservesWrapper()) {
+  if (!obj->getClass()->isDOMClass()) {
-    return true;
-  }
-  if (JS::GetReservedSlot(obj, JS_OBJECT_WRAPPER_SLOT).isUndefined()) {
     return true;
   }
 

js/src/vm/NativeObject-inl.h

@@ -895,7 +895,6 @@ MOZ_ALWAYS_INLINE bool AddDataPropertyToPlainObject(
   obj->initSlot(*resultSlot, v);
 
   MOZ_ASSERT(!obj->getClass()->getAddProperty());
-  MOZ_ASSERT(!obj->getClass()->preservesWrapper());
   return true;
 }
 

js/src/vm/NativeObject.cpp

@@ -1182,10 +1182,6 @@ static MOZ_ALWAYS_INLINE bool CallAddPropertyHook(JSContext* cx,
       return false;
     }
   }
-  if (MOZ_UNLIKELY(obj->hasUnpreservedWrapper())) {
-    MaybePreserveDOMWrapper(cx, obj);
-    return JSObject::setFlag(cx, obj, ObjectFlag::HasPreservedWrapper);
-  }
   return true;
 }
 
@@ -1210,11 +1206,6 @@ static MOZ_ALWAYS_INLINE bool CallAddPropertyHookDense(
       return false;
     }
   }
-
-  if (MOZ_UNLIKELY(obj->hasUnpreservedWrapper())) {
-    MaybePreserveDOMWrapper(cx, obj);
-    return JSObject::setFlag(cx, obj, ObjectFlag::HasPreservedWrapper);
-  }
   return true;
 }
 
@@ -1431,6 +1422,7 @@ static MOZ_ALWAYS_INLINE bool AddDataProperty(JSContext* cx,
 
 bool js::AddSlotAndCallAddPropHook(JSContext* cx, Handle<NativeObject*> obj,
                                    HandleValue v, Handle<Shape*> newShape) {
+  MOZ_ASSERT(obj->getClass()->getAddProperty());
   MOZ_ASSERT(newShape->asShared().lastProperty().isDataProperty());
 
   RootedId id(cx, newShape->asShared().lastProperty().key());

js/src/vm/NativeObject.h

@@ -1730,11 +1730,6 @@ class NativeObject : public JSObject {
                             /* isFixedSlot = */ false);
   }
 
-  bool hasUnpreservedWrapper() const {
-    return getClass()->preservesWrapper() &&
-           !shape()->hasObjectFlag(ObjectFlag::HasPreservedWrapper);
-  }
-
   /* JIT Accessors */
   static size_t offsetOfElements() { return offsetof(NativeObject, elements_); }
   static size_t offsetOfFixedElements() {

js/src/vm/ObjectFlags.h

@@ -86,9 +86,6 @@ enum class ObjectFlag : uint16_t {
   // with it, and thus we must trap on changes to said property.
   HasFuseProperty = 1 << 14,
 
-  // If set, we have already called the preserveWrapper hook for this object.
-  // This should only be set if `obj->getClass()->preservesWrapper()` is true.
-  HasPreservedWrapper = 1 << 15,
 };
 
 using ObjectFlags = EnumFlags<ObjectFlag>;