Issue #1742 - Part 4: Don't trigger read barriers when comparing wrapped pointers types

This is actually an undocumented dependency of Bug 1325406 for Linux and
SunOS.

Ref: BZ 1308236

dom/plugins/base/nsJSNPRuntime.cpp

@@ -1747,7 +1747,7 @@ NPObjWrapper_ObjectMoved(JSObject *obj, const JSObject *old)
   auto entry =
     static_cast<NPObjWrapperHashEntry*>(sNPObjWrappers->Search(npobj));
   MOZ_ASSERT(entry && entry->mJSObj);
-  MOZ_ASSERT(entry->mJSObj.unbarrieredGetPtr() == old);
+  MOZ_ASSERT(entry->mJSObj == old);
   entry->mJSObj = obj;
 }
 

js/ipc/JavaScriptShared.cpp

@@ -100,7 +100,7 @@ IdToObjectMap::has(const ObjectId& id, const JSObject* obj) const
     auto p = table_.lookup(id);
     if (!p)
         return false;
-    return p->value().unbarrieredGet() == obj;
+    return p->value() == obj;
 }
 #endif
 

js/public/RootingAPI.h

@@ -149,10 +149,6 @@ template<typename T>
 struct PersistentRootedMarker;
 } /* namespace gc */
 
-#define DECLARE_POINTER_COMPARISON_OPS(T)                                                         \
-    bool operator==(const T& other) const { return get() == other; }                              \
-    bool operator!=(const T& other) const { return get() != other; }
-
 // Important: Return a reference so passing a Rooted<T>, etc. to
 // something that takes a |const T&| is not a GC hazard.
 #define DECLARE_POINTER_CONSTREF_OPS(T)                                                           \
@@ -242,6 +238,8 @@ class MOZ_NON_MEMMOVABLE Heap : public js::HeapBase<T, Heap<T>>
     static_assert(js::IsHeapConstructibleType<T>::value,
                   "Type T must be a public GC pointer type");
   public:
+    using ElementType = T;
+
     Heap() {
         static_assert(sizeof(T) == sizeof(Heap<T>),
                       "Heap<T> must be binary compatible with T.");
@@ -376,6 +374,8 @@ template <typename T>
 class TenuredHeap : public js::HeapBase<T, TenuredHeap<T>>
 {
   public:
+    using ElementType = T;
+
     TenuredHeap() : bits(0) {
         static_assert(sizeof(T) == sizeof(TenuredHeap<T>),
                       "TenuredHeap<T> must be binary compatible with T.");
@@ -383,9 +383,6 @@ class TenuredHeap : public js::HeapBase<T, TenuredHeap<T>>
     explicit TenuredHeap(T p) : bits(0) { setPtr(p); }
     explicit TenuredHeap(const TenuredHeap<T>& p) : bits(0) { setPtr(p.getPtr()); }
 
-    bool operator==(const TenuredHeap<T>& other) { return bits == other.bits; }
-    bool operator!=(const TenuredHeap<T>& other) { return bits != other.bits; }
-
     void setPtr(T newPtr) {
         MOZ_ASSERT((reinterpret_cast<uintptr_t>(newPtr) & flagsMask) == 0);
         if (newPtr)
@@ -462,6 +459,8 @@ class MOZ_NONHEAP_CLASS Handle : public js::HandleBase<T, Handle<T>>
     friend class JS::MutableHandle<T>;
 
   public:
+    using ElementType = T;
+
     /* Creates a handle from a handle of a type convertible to T. */
     template <typename S>
     MOZ_IMPLICIT Handle(Handle<S> handle,
@@ -522,7 +521,6 @@ class MOZ_NONHEAP_CLASS Handle : public js::HandleBase<T, Handle<T>>
     MOZ_IMPLICIT Handle(MutableHandle<S>& root,
                         typename mozilla::EnableIf<mozilla::IsConvertible<S, T>::value, int>::Type dummy = 0);
 
-    DECLARE_POINTER_COMPARISON_OPS(T);
     DECLARE_POINTER_CONSTREF_OPS(T);
     DECLARE_NONPOINTER_ACCESSOR_METHODS(*ptr);
 
@@ -549,6 +547,8 @@ template <typename T>
 class MOZ_STACK_CLASS MutableHandle : public js::MutableHandleBase<T, MutableHandle<T>>
 {
   public:
+    using ElementType = T;
+
     inline MOZ_IMPLICIT MutableHandle(Rooted<T>* root);
     inline MOZ_IMPLICIT MutableHandle(PersistentRooted<T>* root);
 
@@ -781,6 +781,8 @@ class MOZ_RAII Rooted : public js::RootedBase<T, Rooted<T>>
     }
 
   public:
+    using ElementType = T;
+
     template <typename RootingContext>
     explicit Rooted(const RootingContext& cx)
       : ptr(GCPolicy<T>::initial())
@@ -813,7 +815,6 @@ class MOZ_RAII Rooted : public js::RootedBase<T, Rooted<T>>
         ptr = mozilla::Move(value);
     }
 
-    DECLARE_POINTER_COMPARISON_OPS(T);
     DECLARE_POINTER_CONSTREF_OPS(T);
     DECLARE_POINTER_ASSIGN_OPS(Rooted, T);
     DECLARE_NONPOINTER_ACCESSOR_METHODS(ptr);
@@ -890,13 +891,14 @@ template <typename T>
 class MOZ_RAII FakeRooted : public RootedBase<T, FakeRooted<T>>
 {
   public:
+    using ElementType = T;
+
     template <typename CX>
     explicit FakeRooted(CX* cx) : ptr(JS::GCPolicy<T>::initial()) {}
 
     template <typename CX>
     FakeRooted(CX* cx, T initial) : ptr(initial) {}
 
-    DECLARE_POINTER_COMPARISON_OPS(T);
     DECLARE_POINTER_CONSTREF_OPS(T);
     DECLARE_POINTER_ASSIGN_OPS(FakeRooted, T);
     DECLARE_NONPOINTER_ACCESSOR_METHODS(ptr);
@@ -917,6 +919,8 @@ template <typename T>
 class FakeMutableHandle : public js::MutableHandleBase<T, FakeMutableHandle<T>>
 {
   public:
+    using ElementType = T;
+
     MOZ_IMPLICIT FakeMutableHandle(T* t) {
         ptr = t;
     }
@@ -1107,6 +1111,8 @@ class PersistentRooted : public js::RootedBase<T, PersistentRooted<T>>,
     js::RootLists& rootLists(js::ContextFriendFields* cx) = delete;
 
   public:
+    using ElementType = T;
+
     PersistentRooted() : ptr(GCPolicy<T>::initial()) {}
 
     template <typename RootingContext>
@@ -1160,7 +1166,6 @@ class PersistentRooted : public js::RootedBase<T, PersistentRooted<T>>,
         }
     }
 
-    DECLARE_POINTER_COMPARISON_OPS(T);
     DECLARE_POINTER_CONSTREF_OPS(T);
     DECLARE_POINTER_ASSIGN_OPS(PersistentRooted, T);
     DECLARE_NONPOINTER_ACCESSOR_METHODS(ptr);
@@ -1197,6 +1202,8 @@ class JS_PUBLIC_API(ObjectPtr)
     Heap<JSObject*> value;
 
   public:
+    using ElementType = JSObject*;
+
     ObjectPtr() : value(nullptr) {}
 
     explicit ObjectPtr(JSObject* obj) : value(obj) {}
@@ -1311,6 +1318,177 @@ Swap(JS::TenuredHeap<T>& aX, JS::TenuredHeap<T>& aY)
 
 } /* namespace mozilla */
 
+namespace js {
+namespace detail {
+
+// DefineComparisonOps is a trait which selects which wrapper classes to define
+// operator== and operator!= for. It supplies a getter function to extract the
+// value to compare. This is used to avoid triggering the automatic read
+// barriers where appropriate.
+//
+// If DefineComparisonOps is not specialized for a particular wrapper you may
+// get errors such as 'invalid operands to binary expression' or 'no match for
+// operator==' when trying to compare against instances of the wrapper.
+
+template <typename T>
+struct DefineComparisonOps : mozilla::FalseType {};
+
+template <typename T>
+struct DefineComparisonOps<JS::Heap<T>> : mozilla::TrueType {
+    static const T& get(const JS::Heap<T>& v) { return v.unbarrieredGet(); }
+};
+
+template <typename T>
+struct DefineComparisonOps<JS::TenuredHeap<T>> : mozilla::TrueType {
+    static const T get(const JS::TenuredHeap<T>& v) { return v.unbarrieredGetPtr(); }
+};
+
+template <>
+struct DefineComparisonOps<JS::ObjectPtr> : mozilla::TrueType {
+    static const JSObject* get(const JS::ObjectPtr& v) { return v.unbarrieredGet(); }
+};
+
+template <typename T>
+struct DefineComparisonOps<JS::Rooted<T>> : mozilla::TrueType {
+    static const T& get(const JS::Rooted<T>& v) { return v.get(); }
+};
+
+template <typename T>
+struct DefineComparisonOps<JS::Handle<T>> : mozilla::TrueType {
+    static const T& get(const JS::Handle<T>& v) { return v.get(); }
+};
+
+template <typename T>
+struct DefineComparisonOps<JS::MutableHandle<T>> : mozilla::TrueType {
+    static const T& get(const JS::MutableHandle<T>& v) { return v.get(); }
+};
+
+template <typename T>
+struct DefineComparisonOps<JS::PersistentRooted<T>> : mozilla::TrueType {
+    static const T& get(const JS::PersistentRooted<T>& v) { return v.get(); }
+};
+
+template <typename T>
+struct DefineComparisonOps<js::FakeRooted<T>> : mozilla::TrueType {
+    static const T& get(const js::FakeRooted<T>& v) { return v.get(); }
+};
+
+template <typename T>
+struct DefineComparisonOps<js::FakeMutableHandle<T>> : mozilla::TrueType {
+    static const T& get(const js::FakeMutableHandle<T>& v) { return v.get(); }
+};
+
+} /* namespace detail */
+} /* namespace js */
+
+// Overload operator== and operator!= for all types with the DefineComparisonOps
+// trait using the supplied getter.
+//
+// There are four cases:
+
+// Case 1: comparison between two wrapper objects.
+
+template <typename T, typename U>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value &&
+                           js::detail::DefineComparisonOps<U>::value, bool>::Type
+operator==(const T& a, const U& b) {
+    return js::detail::DefineComparisonOps<T>::get(a) == js::detail::DefineComparisonOps<U>::get(b);
+}
+
+template <typename T, typename U>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value &&
+                           js::detail::DefineComparisonOps<U>::value, bool>::Type
+operator!=(const T& a, const U& b) {
+    return !(a == b);
+}
+
+// Case 2: comparison between a wrapper object and its unwrapped element type.
+
+template <typename T>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value, bool>::Type
+operator==(const T& a, const typename T::ElementType& b) {
+    return js::detail::DefineComparisonOps<T>::get(a) == b;
+}
+
+template <typename T>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value, bool>::Type
+operator!=(const T& a, const typename T::ElementType& b) {
+    return !(a == b);
+}
+
+template <typename T>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value, bool>::Type
+operator==(const typename T::ElementType& a, const T& b) {
+    return a == js::detail::DefineComparisonOps<T>::get(b);
+}
+
+template <typename T>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value, bool>::Type
+operator!=(const typename T::ElementType& a, const T& b) {
+    return !(a == b);
+}
+
+// Case 3: For pointer wrappers, comparison between the wrapper and a const
+// element pointer.
+
+template <typename T>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value &&
+                           mozilla::IsPointer<typename T::ElementType>::value, bool>::Type
+operator==(const typename mozilla::RemovePointer<typename T::ElementType>::Type* a, const T& b) {
+    return a == js::detail::DefineComparisonOps<T>::get(b);
+}
+
+template <typename T>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value &&
+                           mozilla::IsPointer<typename T::ElementType>::value, bool>::Type
+operator!=(const typename mozilla::RemovePointer<typename T::ElementType>::Type* a, const T& b) {
+    return !(a == b);
+}
+
+template <typename T>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value &&
+                           mozilla::IsPointer<typename T::ElementType>::value, bool>::Type
+operator==(const T& a, const typename mozilla::RemovePointer<typename T::ElementType>::Type* b) {
+    return js::detail::DefineComparisonOps<T>::get(a) == b;
+}
+
+template <typename T>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value &&
+                           mozilla::IsPointer<typename T::ElementType>::value, bool>::Type
+operator!=(const T& a, const typename mozilla::RemovePointer<typename T::ElementType>::Type* b) {
+    return !(a == b);
+}
+
+// Case 4: For pointer wrappers, comparison between the wrapper and nullptr.
+
+template <typename T>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value &&
+                           mozilla::IsPointer<typename T::ElementType>::value, bool>::Type
+operator==(std::nullptr_t a, const T& b) {
+    return a == js::detail::DefineComparisonOps<T>::get(b);
+}
+
+template <typename T>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value &&
+                           mozilla::IsPointer<typename T::ElementType>::value, bool>::Type
+operator!=(std::nullptr_t a, const T& b) {
+    return !(a == b);
+}
+
+template <typename T>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value &&
+                           mozilla::IsPointer<typename T::ElementType>::value, bool>::Type
+operator==(const T& a, std::nullptr_t b) {
+    return js::detail::DefineComparisonOps<T>::get(a) == b;
+}
+
+template <typename T>
+typename mozilla::EnableIf<js::detail::DefineComparisonOps<T>::value &&
+                           mozilla::IsPointer<typename T::ElementType>::value, bool>::Type
+operator!=(const T& a, std::nullptr_t b) {
+    return !(a == b);
+}
+
 #undef DELETE_ASSIGNMENT_OPS
 
 #endif  /* js_RootingAPI_h */

js/src/gc/Barrier.h

@@ -356,7 +356,8 @@ class WriteBarrieredBase : public BarrieredBase<T>,
     explicit WriteBarrieredBase(const T& v) : BarrieredBase<T>(v) {}
 
   public:
-    DECLARE_POINTER_COMPARISON_OPS(T);
+    using ElementType = T;
+
     DECLARE_POINTER_CONSTREF_OPS(T);
 
     // Use this if the automatic coercion to T isn't working.
@@ -611,14 +612,13 @@ class ReadBarriered : public ReadBarrieredBase<T>,
         return *this;
     }
 
-    const T get() const {
+    const T& get() const {
-        if (!InternalBarrierMethods<T>::isMarkable(this->value))
+        if (InternalBarrierMethods<T>::isMarkable(this->value))
-            return JS::GCPolicy<T>::initial();
+            this->read();
-        this->read();
         return this->value;
     }
 
-    const T unbarrieredGet() const {
+    const T& unbarrieredGet() const {
         return this->value;
     }
 
@@ -626,9 +626,9 @@ class ReadBarriered : public ReadBarrieredBase<T>,
         return bool(this->value);
     }
 
-    operator const T() const { return get(); }
+    operator const T&() const { return get(); }
 
-    const T operator->() const { return get(); }
+    const T& operator->() const { return get(); }
 
     T* unsafeGet() { return &this->value; }
     T const* unsafeGet() const { return &this->value; }
@@ -934,6 +934,36 @@ typedef ReadBarriered<WasmTableObject*> ReadBarrieredWasmTableObject;
 
 typedef ReadBarriered<Value> ReadBarrieredValue;
 
+namespace detail {
+
+template <typename T>
+struct DefineComparisonOps<PreBarriered<T>> : mozilla::TrueType {
+    static const T& get(const PreBarriered<T>& v) { return v.get(); }
+};
+
+template <typename T>
+struct DefineComparisonOps<GCPtr<T>> : mozilla::TrueType {
+    static const T& get(const GCPtr<T>& v) { return v.get(); }
+};
+
+template <typename T>
+struct DefineComparisonOps<HeapPtr<T>> : mozilla::TrueType {
+    static const T& get(const HeapPtr<T>& v) { return v.get(); }
+};
+
+template <typename T>
+struct DefineComparisonOps<ReadBarriered<T>> : mozilla::TrueType {
+    static const T& get(const ReadBarriered<T>& v) { return v.unbarrieredGet(); }
+};
+
+template <>
+struct DefineComparisonOps<HeapSlot> : mozilla::TrueType {
+    static const Value& get(const HeapSlot& v) { return v.get(); }
+};
+
+} /* namespace detail */
+
+
 } /* namespace js */
 
 #endif /* gc_Barrier_h */

js/src/jsapi-tests/testGCHeapPostBarriers.cpp

@@ -4,6 +4,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+#include "mozilla/TypeTraits.h"
 #include "mozilla/UniquePtr.h"
 
 #include "js/RootingAPI.h"
@@ -146,3 +147,110 @@ TestHeapPostBarrierInitFailure()
 }
 
 END_TEST(testGCHeapPostBarriers)
+
+BEGIN_TEST(testUnbarrieredEquality)
+{
+    // Use ArrayBuffers because they have finalizers, which allows using them
+    // in ObjectPtr without awkward conversations about nursery allocatability.
+    JS::RootedObject robj(cx, JS_NewArrayBuffer(cx, 20));
+    JS::RootedObject robj2(cx, JS_NewArrayBuffer(cx, 30));
+    cx->gc.evictNursery(); // Need tenured objects
+
+    // Need some bare pointers to compare against.
+    JSObject* obj = robj;
+    JSObject* obj2 = robj2;
+    const JSObject* constobj = robj;
+    const JSObject* constobj2 = robj2;
+
+    // Make them gray. We will make sure they stay gray. (For most reads, the
+    // barrier will unmark gray.)
+    using namespace js::gc;
+    TenuredCell* cell = &obj->asTenured();
+    TenuredCell* cell2 = &obj2->asTenured();
+    cell->markIfUnmarked(GRAY);
+    cell2->markIfUnmarked(GRAY);
+    MOZ_ASSERT(cell->isMarked(GRAY));
+    MOZ_ASSERT(cell2->isMarked(GRAY));
+
+    {
+        JS::Heap<JSObject*> heap(obj);
+        JS::Heap<JSObject*> heap2(obj2);
+        CHECK(TestWrapper(obj, obj2, heap, heap2));
+        CHECK(TestWrapper(constobj, constobj2, heap, heap2));
+    }
+
+    {
+        JS::TenuredHeap<JSObject*> heap(obj);
+        JS::TenuredHeap<JSObject*> heap2(obj2);
+        CHECK(TestWrapper(obj, obj2, heap, heap2));
+        CHECK(TestWrapper(constobj, constobj2, heap, heap2));
+    }
+
+    {
+        JS::ObjectPtr objptr(obj);
+        JS::ObjectPtr objptr2(obj2);
+        CHECK(TestWrapper(obj, obj2, objptr, objptr2));
+        CHECK(TestWrapper(constobj, constobj2, objptr, objptr2));
+        objptr.finalize(cx);       
+        objptr2.finalize(cx);
+    }
+    // Sanity check that the barriers normally mark things black.
+    {
+        JS::Heap<JSObject*> heap(obj);
+        JS::Heap<JSObject*> heap2(obj2);
+        heap.get();
+        heap2.get();
+        CHECK(cell->isMarked(BLACK));
+        CHECK(cell2->isMarked(BLACK));
+    }
+
+    return true;
+}
+
+template <typename ObjectT, typename WrapperT>
+bool
+TestWrapper(ObjectT obj, ObjectT obj2, WrapperT& wrapper, WrapperT& wrapper2)
+{
+    using namespace js::gc;
+
+    const TenuredCell& cell = obj->asTenured();
+    const TenuredCell& cell2 = obj2->asTenured();
+
+    int x = 0;
+
+    CHECK(cell.isMarked(GRAY));
+    CHECK(cell2.isMarked(GRAY));
+    x += obj == obj2;
+    CHECK(cell.isMarked(GRAY));
+    CHECK(cell2.isMarked(GRAY));
+    x += obj == wrapper2;
+    CHECK(cell.isMarked(GRAY));
+    CHECK(cell2.isMarked(GRAY));
+    x += wrapper == obj2;
+    CHECK(cell.isMarked(GRAY));
+    CHECK(cell2.isMarked(GRAY));
+    x += wrapper == wrapper2;
+    CHECK(cell.isMarked(GRAY));
+    CHECK(cell2.isMarked(GRAY));
+
+    CHECK(x == 0);
+
+    x += obj != obj2;
+    CHECK(cell.isMarked(GRAY));
+    CHECK(cell2.isMarked(GRAY));
+    x += obj != wrapper2;
+    CHECK(cell.isMarked(GRAY));
+    CHECK(cell2.isMarked(GRAY));
+    x += wrapper != obj2;
+    CHECK(cell.isMarked(GRAY));
+    CHECK(cell2.isMarked(GRAY));
+    x += wrapper != wrapper2;
+    CHECK(cell.isMarked(GRAY));
+    CHECK(cell2.isMarked(GRAY));
+
+    CHECK(x == 4);
+
+    return true;
+}
+
+END_TEST(testUnbarrieredEquality)

js/src/vm/SharedMem.h

@@ -11,8 +11,8 @@
 template<typename T>
 class SharedMem
 {
-    static_assert(mozilla::IsPointer<T>::value,
+    // static_assert(mozilla::IsPointer<T>::value,
-                  "SharedMem encapsulates pointer types");
+    //               "SharedMem encapsulates pointer types");
 
     enum Sharedness {
         IsUnshared,

js/xpconnect/src/XPCInlines.h

@@ -470,7 +470,7 @@ inline
 void XPCWrappedNativeTearOff::JSObjectMoved(JSObject* obj, const JSObject* old)
 {
     MOZ_ASSERT(!IsMarked());
-    MOZ_ASSERT(mJSObject.unbarrieredGetPtr() == old);
+    MOZ_ASSERT(mJSObject == old);
     mJSObject = obj;
 }
 

js/xpconnect/src/XPCWrappedNative.cpp

@@ -935,7 +935,7 @@ void
 XPCWrappedNative::FlatJSObjectMoved(JSObject* obj, const JSObject* old)
 {
     JS::AutoAssertGCCallback inCallback(obj);
-    MOZ_ASSERT(mFlatJSObject.unbarrieredGetPtr() == old);
+    MOZ_ASSERT(mFlatJSObject == old);
 
     nsWrapperCache* cache = nullptr;
     CallQueryInterface(mIdentity, &cache);

js/xpconnect/src/XPCWrappedNativeProto.cpp

@@ -114,7 +114,7 @@ XPCWrappedNativeProto::CallPostCreatePrototype()
 void
 XPCWrappedNativeProto::JSProtoObjectFinalized(js::FreeOp* fop, JSObject* obj)
 {
-    MOZ_ASSERT(obj == mJSProtoObject.unbarrieredGet(), "huh?");
+    MOZ_ASSERT(obj == mJSProtoObject, "huh?");
 
     // Only remove this proto from the map if it is the one in the map.
     ClassInfo2WrappedNativeProtoMap* map = GetScope()->GetWrappedNativeProtoMap();
@@ -129,7 +129,7 @@ XPCWrappedNativeProto::JSProtoObjectFinalized(js::FreeOp* fop, JSObject* obj)
 void
 XPCWrappedNativeProto::JSProtoObjectMoved(JSObject* obj, const JSObject* old)
 {
-    MOZ_ASSERT(mJSProtoObject.unbarrieredGet() == old);
+    MOZ_ASSERT(mJSProtoObject == old);
     mJSProtoObject.init(obj); // Update without triggering barriers.
 }