Fix Value::isGCThing footgun, stop returning true for NullValue

4 files changed, 26 insertions, 42 deletions

diff --git a/js/public/CallArgs.h b/js/public/CallArgs.h
index 6e6164e55..a7774309a 100644
--- a/js/public/CallArgs.h
+++ b/js/public/CallArgs.h
@@ -290,7 +290,7 @@ class MOZ_STACK_CLASS CallArgs : public detail::CallArgsBase<detail::IncludeUsed
         args.constructing_ = constructing;
 #ifdef DEBUG
         for (unsigned i = 0; i < argc; ++i)
-            MOZ_ASSERT_IF(argv[i].isMarkable(), !GCThingIsMarkedGray(GCCellPtr(argv[i])));
+            MOZ_ASSERT_IF(argv[i].isGCThing(), !GCThingIsMarkedGray(GCCellPtr(argv[i])));
 #endif
         return args;
     }
diff --git a/js/public/Proxy.h b/js/public/Proxy.h
index 3e95538db..5acb91b26 100644
--- a/js/public/Proxy.h
+++ b/js/public/Proxy.h
@@ -456,7 +456,7 @@ SetProxyExtra(JSObject* obj, size_t n, const Value& extra)
     Value* vp = &detail::GetProxyDataLayout(obj)->values->extraSlots[n];
 
     // Trigger a barrier before writing the slot.
-    if (vp->isMarkable() || extra.isMarkable())
+    if (vp->isGCThing() || extra.isGCThing())
         SetValueInProxy(vp, extra);
     else
         *vp = extra;
@@ -482,7 +482,7 @@ SetReservedOrProxyPrivateSlot(JSObject* obj, size_t slot, const Value& value)
     MOZ_ASSERT(slot == 0);
     MOZ_ASSERT(slot < JSCLASS_RESERVED_SLOTS(GetObjectClass(obj)) || IsProxy(obj));
     shadow::Object* sobj = reinterpret_cast<shadow::Object*>(obj);
-    if (sobj->slotRef(slot).isMarkable() || value.isMarkable())
+    if (sobj->slotRef(slot).isGCThing() || value.isGCThing())
         SetReservedOrProxyPrivateSlotWithBarrier(obj, slot, value);
     else
         sobj->slotRef(slot) = value;
diff --git a/js/public/TraceKind.h b/js/public/TraceKind.h
index 2eda9cb2c..3270966fc 100644
--- a/js/public/TraceKind.h
+++ b/js/public/TraceKind.h
@@ -40,9 +40,11 @@ enum class TraceKind
     // Note: The order here is determined by our Value packing. Other users
     //       should sort alphabetically, for consistency.
     Object = 0x00,
-    String = 0x01,
-    Symbol = 0x02,
-    Script = 0x03,
+    String = 0x02,
+    Symbol = 0x03,
+
+    // 0x1 is not used for any GCThing Value tag, so we use it for Script.
+    Script = 0x01,
 
     // Shape details are exposed through JS_TraceShapeCycleCollectorChildren.
     Shape = 0x04,
diff --git a/js/public/Value.h b/js/public/Value.h
index 00fdad586..a40e65c83 100644
--- a/js/public/Value.h
+++ b/js/public/Value.h
@@ -51,12 +51,12 @@ JS_ENUM_HEADER(JSValueType, uint8_t)
     JSVAL_TYPE_DOUBLE              = 0x00,
     JSVAL_TYPE_INT32               = 0x01,
     JSVAL_TYPE_UNDEFINED           = 0x02,
-    JSVAL_TYPE_BOOLEAN             = 0x03,
-    JSVAL_TYPE_MAGIC               = 0x04,
-    JSVAL_TYPE_STRING              = 0x05,
-    JSVAL_TYPE_SYMBOL              = 0x06,
-    JSVAL_TYPE_PRIVATE_GCTHING     = 0x07,
-    JSVAL_TYPE_NULL                = 0x08,
+    JSVAL_TYPE_NULL                = 0x03,
+    JSVAL_TYPE_BOOLEAN             = 0x04,
+    JSVAL_TYPE_MAGIC               = 0x05,
+    JSVAL_TYPE_STRING              = 0x06,
+    JSVAL_TYPE_SYMBOL              = 0x07,
+    JSVAL_TYPE_PRIVATE_GCTHING     = 0x08,
     JSVAL_TYPE_OBJECT              = 0x0c,
 
     /* These never appear in a jsval; they are only provided as an out-of-band value. */
@@ -75,11 +75,11 @@ JS_ENUM_HEADER(JSValueTag, uint32_t)
     JSVAL_TAG_CLEAR                = 0xFFFFFF80,
     JSVAL_TAG_INT32                = JSVAL_TAG_CLEAR | JSVAL_TYPE_INT32,
     JSVAL_TAG_UNDEFINED            = JSVAL_TAG_CLEAR | JSVAL_TYPE_UNDEFINED,
+    JSVAL_TAG_NULL                 = JSVAL_TAG_CLEAR | JSVAL_TYPE_NULL,
     JSVAL_TAG_STRING               = JSVAL_TAG_CLEAR | JSVAL_TYPE_STRING,
     JSVAL_TAG_SYMBOL               = JSVAL_TAG_CLEAR | JSVAL_TYPE_SYMBOL,
     JSVAL_TAG_BOOLEAN              = JSVAL_TAG_CLEAR | JSVAL_TYPE_BOOLEAN,
     JSVAL_TAG_MAGIC                = JSVAL_TAG_CLEAR | JSVAL_TYPE_MAGIC,
-    JSVAL_TAG_NULL                 = JSVAL_TAG_CLEAR | JSVAL_TYPE_NULL,
     JSVAL_TAG_OBJECT               = JSVAL_TAG_CLEAR | JSVAL_TYPE_OBJECT,
     JSVAL_TAG_PRIVATE_GCTHING      = JSVAL_TAG_CLEAR | JSVAL_TYPE_PRIVATE_GCTHING
 } JS_ENUM_FOOTER(JSValueTag);
@@ -95,11 +95,11 @@ JS_ENUM_HEADER(JSValueTag, uint32_t)
     JSVAL_TAG_MAX_DOUBLE           = 0x1FFF0,
     JSVAL_TAG_INT32                = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_INT32,
     JSVAL_TAG_UNDEFINED            = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_UNDEFINED,
+    JSVAL_TAG_NULL                 = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_NULL,
     JSVAL_TAG_STRING               = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_STRING,
     JSVAL_TAG_SYMBOL               = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_SYMBOL,
     JSVAL_TAG_BOOLEAN              = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_BOOLEAN,
     JSVAL_TAG_MAGIC                = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_MAGIC,
-    JSVAL_TAG_NULL                 = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_NULL,
     JSVAL_TAG_OBJECT               = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_OBJECT,
     JSVAL_TAG_PRIVATE_GCTHING      = JSVAL_TAG_MAX_DOUBLE | JSVAL_TYPE_PRIVATE_GCTHING
 } JS_ENUM_FOOTER(JSValueTag);
@@ -112,11 +112,11 @@ JS_ENUM_HEADER(JSValueShiftedTag, uint64_t)
     JSVAL_SHIFTED_TAG_MAX_DOUBLE      = ((((uint64_t)JSVAL_TAG_MAX_DOUBLE)     << JSVAL_TAG_SHIFT) | 0xFFFFFFFF),
     JSVAL_SHIFTED_TAG_INT32           = (((uint64_t)JSVAL_TAG_INT32)           << JSVAL_TAG_SHIFT),
     JSVAL_SHIFTED_TAG_UNDEFINED       = (((uint64_t)JSVAL_TAG_UNDEFINED)       << JSVAL_TAG_SHIFT),
+    JSVAL_SHIFTED_TAG_NULL            = (((uint64_t)JSVAL_TAG_NULL)            << JSVAL_TAG_SHIFT),
     JSVAL_SHIFTED_TAG_STRING          = (((uint64_t)JSVAL_TAG_STRING)          << JSVAL_TAG_SHIFT),
     JSVAL_SHIFTED_TAG_SYMBOL          = (((uint64_t)JSVAL_TAG_SYMBOL)          << JSVAL_TAG_SHIFT),
     JSVAL_SHIFTED_TAG_BOOLEAN         = (((uint64_t)JSVAL_TAG_BOOLEAN)         << JSVAL_TAG_SHIFT),
     JSVAL_SHIFTED_TAG_MAGIC           = (((uint64_t)JSVAL_TAG_MAGIC)           << JSVAL_TAG_SHIFT),
-    JSVAL_SHIFTED_TAG_NULL            = (((uint64_t)JSVAL_TAG_NULL)            << JSVAL_TAG_SHIFT),
     JSVAL_SHIFTED_TAG_OBJECT          = (((uint64_t)JSVAL_TAG_OBJECT)          << JSVAL_TAG_SHIFT),
     JSVAL_SHIFTED_TAG_PRIVATE_GCTHING = (((uint64_t)JSVAL_TAG_PRIVATE_GCTHING) << JSVAL_TAG_SHIFT)
 } JS_ENUM_FOOTER(JSValueShiftedTag);
@@ -140,7 +140,6 @@ static_assert(sizeof(JSValueShiftedTag) == sizeof(uint64_t),
 
 #define JSVAL_TYPE_TO_TAG(type)      ((JSValueTag)(JSVAL_TAG_CLEAR | (type)))
 
-#define JSVAL_LOWER_INCL_TAG_OF_OBJ_OR_NULL_SET         JSVAL_TAG_NULL
 #define JSVAL_UPPER_EXCL_TAG_OF_PRIMITIVE_SET           JSVAL_TAG_OBJECT
 #define JSVAL_UPPER_INCL_TAG_OF_NUMBER_SET              JSVAL_TAG_INT32
 #define JSVAL_LOWER_INCL_TAG_OF_GCTHING_SET             JSVAL_TAG_STRING
@@ -152,12 +151,10 @@ static_assert(sizeof(JSValueShiftedTag) == sizeof(uint64_t),
 #define JSVAL_TYPE_TO_TAG(type)      ((JSValueTag)(JSVAL_TAG_MAX_DOUBLE | (type)))
 #define JSVAL_TYPE_TO_SHIFTED_TAG(type) (((uint64_t)JSVAL_TYPE_TO_TAG(type)) << JSVAL_TAG_SHIFT)
 
-#define JSVAL_LOWER_INCL_TAG_OF_OBJ_OR_NULL_SET         JSVAL_TAG_NULL
 #define JSVAL_UPPER_EXCL_TAG_OF_PRIMITIVE_SET           JSVAL_TAG_OBJECT
 #define JSVAL_UPPER_INCL_TAG_OF_NUMBER_SET              JSVAL_TAG_INT32
 #define JSVAL_LOWER_INCL_TAG_OF_GCTHING_SET             JSVAL_TAG_STRING
 
-#define JSVAL_LOWER_INCL_SHIFTED_TAG_OF_OBJ_OR_NULL_SET  JSVAL_SHIFTED_TAG_NULL
 #define JSVAL_UPPER_EXCL_SHIFTED_TAG_OF_PRIMITIVE_SET    JSVAL_SHIFTED_TAG_OBJECT
 #define JSVAL_UPPER_EXCL_SHIFTED_TAG_OF_NUMBER_SET       JSVAL_SHIFTED_TAG_UNDEFINED
 #define JSVAL_LOWER_INCL_SHIFTED_TAG_OF_GCTHING_SET      JSVAL_SHIFTED_TAG_STRING
@@ -537,12 +534,7 @@ class MOZ_NON_PARAM alignas(8) Value
     }
 
     bool isObjectOrNull() const {
-        MOZ_ASSERT(uint32_t(toTag()) <= uint32_t(JSVAL_TAG_OBJECT));
-#if defined(JS_NUNBOX32)
-        return uint32_t(toTag()) >= uint32_t(JSVAL_LOWER_INCL_TAG_OF_OBJ_OR_NULL_SET);
-#elif defined(JS_PUNBOX64)
-        return data.asBits >= JSVAL_LOWER_INCL_SHIFTED_TAG_OF_OBJ_OR_NULL_SET;
-#endif
+        return isObject() || isNull();
     }
 
     bool isGCThing() const {
@@ -575,12 +567,8 @@ class MOZ_NON_PARAM alignas(8) Value
         return isMagic();
     }
 
-    bool isMarkable() const {
-        return isGCThing() && !isNull();
-    }
-
     JS::TraceKind traceKind() const {
-        MOZ_ASSERT(isMarkable());
+        MOZ_ASSERT(isGCThing());
         static_assert((JSVAL_TAG_STRING & 0x03) == size_t(JS::TraceKind::String),
                       "Value type tags must correspond with JS::TraceKinds.");
         static_assert((JSVAL_TAG_SYMBOL & 0x03) == size_t(JS::TraceKind::Symbol),
@@ -684,11 +672,6 @@ class MOZ_NON_PARAM alignas(8) Value
 #endif
     }
 
-    js::gc::Cell* toMarkablePointer() const {
-        MOZ_ASSERT(isMarkable());
-        return toGCThing();
-    }
-
     GCCellPtr toGCCellPtr() const {
         return GCCellPtr(toGCThing(), traceKind());
     }
@@ -760,9 +743,9 @@ class MOZ_NON_PARAM alignas(8) Value
      * Private GC Thing API
      *
      * Non-JSObject, JSString, and JS::Symbol cells may be put into the 64-bit
-     * payload as private GC things. Such Values are considered isMarkable()
-     * and isGCThing(), and as such, automatically marked. Their traceKind()
-     * is gotten via their cells.
+     * payload as private GC things. Such Values are considered isGCThing(), and
+     * as such, automatically marked. Their traceKind() is gotten via their
+     * cells.
      */
 
     void setPrivateGCThing(js::gc::Cell* cell) {
@@ -980,7 +963,7 @@ IsOptimizedPlaceholderMagicValue(const Value& v)
 static MOZ_ALWAYS_INLINE void
 ExposeValueToActiveJS(const Value& v)
 {
-    if (v.isMarkable())
+    if (v.isGCThing())
         js::gc::ExposeGCThingToActiveJS(GCCellPtr(v));
 }
 
@@ -1298,7 +1281,7 @@ template <>
 struct BarrierMethods<JS::Value>
 {
     static gc::Cell* asGCThingOrNull(const JS::Value& v) {
-        return v.isMarkable() ? v.toGCThing() : nullptr;
+        return v.isGCThing() ? v.toGCThing() : nullptr;
     }
     static void postBarrier(JS::Value* v, const JS::Value& prev, const JS::Value& next) {
         JS::HeapValuePostBarrier(v, prev, next);
@@ -1338,9 +1321,8 @@ class ValueOperations
     bool isObject() const { return value().isObject(); }
     bool isMagic() const { return value().isMagic(); }
     bool isMagic(JSWhyMagic why) const { return value().isMagic(why); }
-    bool isMarkable() const { return value().isMarkable(); }
-    bool isPrimitive() const { return value().isPrimitive(); }
     bool isGCThing() const { return value().isGCThing(); }
+    bool isPrimitive() const { return value().isPrimitive(); }
 
     bool isNullOrUndefined() const { return value().isNullOrUndefined(); }
     bool isObjectOrNull() const { return value().isObjectOrNull(); }
@@ -1485,7 +1467,7 @@ DispatchTyped(F f, const JS::Value& val, Args&&... args)
         return f(val.toSymbol(), mozilla::Forward<Args>(args)...);
     if (MOZ_UNLIKELY(val.isPrivateGCThing()))
         return DispatchTyped(f, val.toGCCellPtr(), mozilla::Forward<Args>(args)...);
-    MOZ_ASSERT(!val.isMarkable());
+    MOZ_ASSERT(!val.isGCThing());
     return F::defaultValue(val);
 }