[symbol-as-weakmap-key] Fix DCHECKs and add CanBeHeldWeakly

There are a few DCHECKs that weren't updated to allow for Symbols as weak collection keys. This CL updates those DCHECKs and also does the following refactors for clarity: - Add Object::CanBeHeldWeakly - Rename GotoIfCannotBeWeakKey -> GotoIfCannotBeHeldWeakly to align with spec AO name Bug: chromium:1370400, chromium:1370402, v8:12947 Change-Id: I380840c8377497feae97e3fca37555dae0dcc255 Fixed: chromium:1370400, chromium:1370402 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3928150 Auto-Submit: Shu-yu Guo <syg@chromium.org> Reviewed-by: Marja Hölttä <marja@chromium.org> Commit-Queue: Marja Hölttä <marja@chromium.org> Cr-Commit-Position: refs/heads/main@{#83507}
v8 · Oct 4, 2022 · 94e8282 · 94e8282
1 parent 699147d
commit 94e8282
Show file tree

Hide file tree

Showing 12 changed files with 66 additions and 62 deletions.
diff --git a/src/builtins/builtins-collections-gen.cc b/src/builtins/builtins-collections-gen.cc
@@ -425,25 +425,25 @@ TNode<IntPtrT> BaseCollectionsAssembler::EstimatedInitialSize(
 }
 
 // https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-canbeheldweakly-abstract-operation
-void BaseCollectionsAssembler::GotoIfCannotBeWeakKey(
-    const TNode<Object> obj, Label* if_cannot_be_weak_key) {
+void BaseCollectionsAssembler::GotoIfCannotBeHeldWeakly(
+    const TNode<Object> obj, Label* if_cannot_be_held_weakly) {
   Label check_symbol_key(this);
   Label end(this);
-  GotoIf(TaggedIsSmi(obj), if_cannot_be_weak_key);
+  GotoIf(TaggedIsSmi(obj), if_cannot_be_held_weakly);
   TNode<Uint16T> instance_type = LoadMapInstanceType(LoadMap(CAST(obj)));
   GotoIfNot(IsJSReceiverInstanceType(instance_type), &check_symbol_key);
   // TODO(v8:12547) Shared structs and arrays should only be able to point
   // to shared values in weak collections. For now, disallow them as weak
   // collection keys.
-  GotoIf(IsJSSharedStructInstanceType(instance_type), if_cannot_be_weak_key);
-  GotoIf(IsJSSharedArrayInstanceType(instance_type), if_cannot_be_weak_key);
+  GotoIf(IsJSSharedStructInstanceType(instance_type), if_cannot_be_held_weakly);
+  GotoIf(IsJSSharedArrayInstanceType(instance_type), if_cannot_be_held_weakly);
   Goto(&end);
   Bind(&check_symbol_key);
-  GotoIfNot(HasHarmonySymbolAsWeakmapKeyFlag(), if_cannot_be_weak_key);
-  GotoIfNot(IsSymbolInstanceType(instance_type), if_cannot_be_weak_key);
+  GotoIfNot(HasHarmonySymbolAsWeakmapKeyFlag(), if_cannot_be_held_weakly);
+  GotoIfNot(IsSymbolInstanceType(instance_type), if_cannot_be_held_weakly);
   TNode<Uint32T> flags = LoadSymbolFlags(CAST(obj));
   GotoIf(Word32And(flags, Symbol::IsInPublicSymbolTableBit::kMask),
-         if_cannot_be_weak_key);
+         if_cannot_be_held_weakly);
   Goto(&end);
   Bind(&end);
 }
@@ -2586,17 +2586,17 @@ TF_BUILTIN(WeakMapLookupHashIndex, WeakCollectionsBuiltinsAssembler) {
   auto table = Parameter<EphemeronHashTable>(Descriptor::kTable);
   auto key = Parameter<Object>(Descriptor::kKey);
 
-  Label if_cannot_be_weak_key(this);
+  Label if_cannot_be_held_weakly(this);
 
-  GotoIfCannotBeWeakKey(key, &if_cannot_be_weak_key);
+  GotoIfCannotBeHeldWeakly(key, &if_cannot_be_held_weakly);
 
-  TNode<IntPtrT> hash = GetHash(CAST(key), &if_cannot_be_weak_key);
+  TNode<IntPtrT> hash = GetHash(CAST(key), &if_cannot_be_held_weakly);
   TNode<IntPtrT> capacity = LoadTableCapacity(table);
   TNode<IntPtrT> key_index = FindKeyIndexForKey(
-      table, key, hash, EntryMask(capacity), &if_cannot_be_weak_key);
+      table, key, hash, EntryMask(capacity), &if_cannot_be_held_weakly);
   Return(SmiTag(ValueIndexFromKeyIndex(key_index)));
 
-  BIND(&if_cannot_be_weak_key);
+  BIND(&if_cannot_be_held_weakly);
   Return(SmiConstant(-1));
 }
 
@@ -2651,22 +2651,22 @@ TF_BUILTIN(WeakCollectionDelete, WeakCollectionsBuiltinsAssembler) {
   auto collection = Parameter<JSWeakCollection>(Descriptor::kCollection);
   auto key = Parameter<Object>(Descriptor::kKey);
 
-  Label call_runtime(this), if_cannot_be_weak_key(this);
+  Label call_runtime(this), if_cannot_be_held_weakly(this);
 
-  GotoIfCannotBeWeakKey(key, &if_cannot_be_weak_key);
+  GotoIfCannotBeHeldWeakly(key, &if_cannot_be_held_weakly);
 
-  TNode<IntPtrT> hash = GetHash(CAST(key), &if_cannot_be_weak_key);
+  TNode<IntPtrT> hash = GetHash(CAST(key), &if_cannot_be_held_weakly);
   TNode<EphemeronHashTable> table = LoadTable(collection);
   TNode<IntPtrT> capacity = LoadTableCapacity(table);
   TNode<IntPtrT> key_index = FindKeyIndexForKey(
-      table, key, hash, EntryMask(capacity), &if_cannot_be_weak_key);
+      table, key, hash, EntryMask(capacity), &if_cannot_be_held_weakly);
   TNode<IntPtrT> number_of_elements = LoadNumberOfElements(table, -1);
   GotoIf(ShouldShrink(capacity, number_of_elements), &call_runtime);
 
   RemoveEntry(table, key_index, number_of_elements);
   Return(TrueConstant());
 
-  BIND(&if_cannot_be_weak_key);
+  BIND(&if_cannot_be_held_weakly);
   Return(FalseConstant());
 
   BIND(&call_runtime);
@@ -2751,7 +2751,7 @@ TF_BUILTIN(WeakMapPrototypeSet, WeakCollectionsBuiltinsAssembler) {
                          "WeakMap.prototype.set");
 
   Label throw_invalid_key(this);
-  GotoIfCannotBeWeakKey(key, &throw_invalid_key);
+  GotoIfCannotBeHeldWeakly(key, &throw_invalid_key);
 
   Return(
       CallBuiltin(Builtin::kWeakCollectionSet, context, receiver, key, value));
@@ -2769,7 +2769,7 @@ TF_BUILTIN(WeakSetPrototypeAdd, WeakCollectionsBuiltinsAssembler) {
                          "WeakSet.prototype.add");
 
   Label throw_invalid_value(this);
-  GotoIfCannotBeWeakKey(value, &throw_invalid_value);
+  GotoIfCannotBeHeldWeakly(value, &throw_invalid_value);
 
   Return(CallBuiltin(Builtin::kWeakCollectionSet, context, receiver, value,
                      TrueConstant()));

diff --git a/src/builtins/builtins-collections-gen.h b/src/builtins/builtins-collections-gen.h
@@ -27,8 +27,8 @@ class BaseCollectionsAssembler : public CodeStubAssembler {
 
   virtual ~BaseCollectionsAssembler() = default;
 
-  void GotoIfCannotBeWeakKey(const TNode<Object> obj,
-                             Label* if_cannot_be_weak_key);
+  void GotoIfCannotBeHeldWeakly(const TNode<Object> obj,
+                                Label* if_cannot_be_held_weakly);
 
  protected:
   enum Variant { kMap, kSet, kWeakMap, kWeakSet };

diff --git a/src/builtins/builtins-weak-refs.cc b/src/builtins/builtins-weak-refs.cc
@@ -27,23 +27,10 @@ BUILTIN(FinalizationRegistryUnregister) {
 
   // 4. If CanBeHeldWeakly(unregisterToken) is false, throw a TypeError
   // exception.
-  if (v8_flags.harmony_symbol_as_weakmap_key) {
-    if (!unregister_token->IsJSReceiver() &&
-        (!unregister_token->IsSymbol() ||
-         Handle<Symbol>::cast(unregister_token)->is_in_public_symbol_table())) {
-      THROW_NEW_ERROR_RETURN_FAILURE(
-          isolate,
-          NewTypeError(MessageTemplate::kInvalidWeakRefsUnregisterToken,
-                       unregister_token));
-    }
-  } else {
-    // 4. If Type(unregisterToken) is not Object, throw a TypeError exception.
-    if (!unregister_token->IsJSReceiver()) {
-      THROW_NEW_ERROR_RETURN_FAILURE(
-          isolate,
-          NewTypeError(MessageTemplate::kInvalidWeakRefsUnregisterToken,
-                       unregister_token));
-    }
+  if (!unregister_token->CanBeHeldWeakly()) {
+    THROW_NEW_ERROR_RETURN_FAILURE(
+        isolate, NewTypeError(MessageTemplate::kInvalidWeakRefsUnregisterToken,
+                              unregister_token));
   }
 
   bool success = JSFinalizationRegistry::Unregister(

diff --git a/src/builtins/finalization-registry.tq b/src/builtins/finalization-registry.tq
@@ -16,7 +16,7 @@ extern transitioning macro
 RemoveFinalizationRegistryCellFromUnregisterTokenMap(
     JSFinalizationRegistry, WeakCell): void;
 
-extern macro WeakCollectionsBuiltinsAssembler::GotoIfCannotBeWeakKey(JSAny):
+extern macro WeakCollectionsBuiltinsAssembler::GotoIfCannotBeHeldWeakly(JSAny):
     void labels NotWeakKey;
 
 macro SplitOffTail(weakCell: WeakCell): WeakCell|Undefined {
@@ -140,7 +140,7 @@ FinalizationRegistryRegister(
       MessageTemplate::kIncompatibleMethodReceiver,
       'FinalizationRegistry.prototype.register', receiver);
   // 3. If CanBeHeldWeakly(target) is false, throw a TypeError exception.
-  GotoIfCannotBeWeakKey(arguments[0])
+  GotoIfCannotBeHeldWeakly(arguments[0])
       otherwise ThrowTypeError(MessageTemplate::kInvalidWeakRefsRegisterTarget);
 
   const target = UnsafeCast<(JSReceiver | Symbol)>(arguments[0]);
@@ -159,7 +159,7 @@ FinalizationRegistryRegister(
   if (IsUndefined(unregisterTokenRaw)) {
     unregisterToken = Undefined;
   } else {
-    GotoIfCannotBeWeakKey(unregisterTokenRaw)
+    GotoIfCannotBeHeldWeakly(unregisterTokenRaw)
         otherwise ThrowTypeError(
         MessageTemplate::kInvalidWeakRefsUnregisterToken, unregisterTokenRaw);
     unregisterToken = UnsafeCast<(JSReceiver | Symbol)>(unregisterTokenRaw);

diff --git a/src/builtins/weak-ref.tq b/src/builtins/weak-ref.tq
@@ -24,7 +24,7 @@ WeakRefConstructor(
   }
 
   // 2. If CanBeHeldWeakly(weakTarget) is false, throw a TypeError exception.
-  GotoIfCannotBeWeakKey(weakTarget) otherwise ThrowTypeError(
+  GotoIfCannotBeHeldWeakly(weakTarget) otherwise ThrowTypeError(
       MessageTemplate::kInvalidWeakRefsWeakRefConstructorTarget);
 
   // 3. Let weakRef be ? OrdinaryCreateFromConstructor(NewTarget,

diff --git a/src/diagnostics/objects-debug.cc b/src/diagnostics/objects-debug.cc
@@ -1300,9 +1300,7 @@ void JSSharedArray::JSSharedArrayVerify(Isolate* isolate) {
 void WeakCell::WeakCellVerify(Isolate* isolate) {
   CHECK(IsWeakCell());
 
-  CHECK(target().IsJSReceiver() || target().IsUndefined(isolate) ||
-        (target().IsSymbol() &&
-         !Symbol::cast(target()).is_in_public_symbol_table()));
+  CHECK(target().IsUndefined(isolate) || target().CanBeHeldWeakly());
 
   CHECK(prev().IsWeakCell() || prev().IsUndefined(isolate));
   if (prev().IsWeakCell()) {
@@ -1330,9 +1328,7 @@ void WeakCell::WeakCellVerify(Isolate* isolate) {
 void JSWeakRef::JSWeakRefVerify(Isolate* isolate) {
   CHECK(IsJSWeakRef());
   JSObjectVerify(isolate);
-  CHECK(target().IsUndefined(isolate) || target().IsJSReceiver() ||
-        (target().IsSymbol() &&
-         !Symbol::cast(target()).is_in_public_symbol_table()));
+  CHECK(target().IsUndefined(isolate) || target().CanBeHeldWeakly());
 }
 
 void JSFinalizationRegistry::JSFinalizationRegistryVerify(Isolate* isolate) {

diff --git a/src/objects/js-weak-refs-inl.h b/src/objects/js-weak-refs-inl.h
@@ -170,9 +170,7 @@ void WeakCell::Nullify(Isolate* isolate,
   // only called for WeakCells which haven't been unregistered yet, so they will
   // be in the active_cells list. (The caller must guard against calling this
   // for unregistered WeakCells by checking that the target is not undefined.)
-  DCHECK(target().IsJSReceiver() ||
-         (target().IsSymbol() &&
-          !Symbol::cast(target()).is_in_public_symbol_table()));
+  DCHECK(target().CanBeHeldWeakly());
   set_target(ReadOnlyRoots(isolate).undefined_value());
 
   JSFinalizationRegistry fr =
@@ -218,7 +216,7 @@ void WeakCell::RemoveFromFinalizationRegistryCells(Isolate* isolate) {
 
   // It's important to set_target to undefined here. This guards that we won't
   // call Nullify (which assumes that the WeakCell is in active_cells).
-  DCHECK(target().IsUndefined() || target().IsJSReceiver());
+  DCHECK(target().IsUndefined() || target().CanBeHeldWeakly());
   set_target(ReadOnlyRoots(isolate).undefined_value());
 
   JSFinalizationRegistry fr =

diff --git a/src/objects/objects-inl.h b/src/objects/objects-inl.h
@@ -1206,6 +1206,23 @@ MaybeHandle<Object> Object::Share(Isolate* isolate, Handle<Object> value,
                    throw_if_cannot_be_shared);
 }
 
+// https://tc39.es/proposal-symbols-as-weakmap-keys/#sec-canbeheldweakly-abstract-operation
+bool Object::CanBeHeldWeakly() const {
+  if (IsJSReceiver()) {
+    // TODO(v8:12547) Shared structs and arrays should only be able to point
+    // to shared values in weak collections. For now, disallow them as weak
+    // collection keys.
+    if (v8_flags.harmony_struct) {
+      return !IsJSSharedStruct() && !IsJSSharedArray();
+    }
+    return true;
+  }
+  if (v8_flags.harmony_symbol_as_weakmap_key) {
+    return IsSymbol() && !Symbol::cast(*this).is_in_public_symbol_table();
+  }
+  return false;
+}
+
 Handle<Object> ObjectHashTableShape::AsHandle(Handle<Object> key) {
   return key;
 }

diff --git a/src/objects/objects.h b/src/objects/objects.h
@@ -785,6 +785,11 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
                                        Handle<HeapObject> value,
                                        ShouldThrow throw_if_cannot_be_shared);
 
+  // Whether this Object can be held weakly, i.e. whether it can be used as a
+  // key in WeakMap, as a key in WeakSet, as the target of a WeakRef, or as a
+  // target or unregister token of a FinalizationRegistry.
+  inline bool CanBeHeldWeakly() const;
+
  protected:
   inline Address field_address(size_t offset) const {
     return ptr() + offset - kHeapObjectTag;

diff --git a/src/runtime/runtime-collections.cc b/src/runtime/runtime-collections.cc
@@ -79,7 +79,7 @@ RUNTIME_FUNCTION(Runtime_WeakCollectionDelete) {
   int hash = args.smi_value_at(2);
 
 #ifdef DEBUG
-  DCHECK(key->IsJSReceiver());
+  DCHECK(key->CanBeHeldWeakly());
   DCHECK(EphemeronHashTable::IsKey(ReadOnlyRoots(isolate), *key));
   Handle<EphemeronHashTable> table(
       EphemeronHashTable::cast(weak_collection->table()), isolate);
@@ -102,7 +102,7 @@ RUNTIME_FUNCTION(Runtime_WeakCollectionSet) {
   int hash = args.smi_value_at(3);
 
 #ifdef DEBUG
-  DCHECK(key->IsJSReceiver());
+  DCHECK(key->CanBeHeldWeakly());
   DCHECK(EphemeronHashTable::IsKey(ReadOnlyRoots(isolate), *key));
   Handle<EphemeronHashTable> table(
       EphemeronHashTable::cast(weak_collection->table()), isolate);

diff --git a/src/runtime/runtime-weak-refs.cc b/src/runtime/runtime-weak-refs.cc
@@ -44,13 +44,7 @@ RUNTIME_FUNCTION(Runtime_JSWeakRefAddToKeptObjects) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
   Handle<HeapObject> object = args.at<HeapObject>(0);
-  if (FLAG_harmony_symbol_as_weakmap_key) {
-    DCHECK(object->IsJSReceiver() ||
-           (object->IsSymbol() &&
-            !(Handle<Symbol>::cast(object))->is_in_public_symbol_table()));
-  } else {
-    DCHECK(object->IsJSReceiver());
-  }
+  DCHECK(object->CanBeHeldWeakly());
 
   isolate->heap()->KeepDuringJob(object);
 

diff --git a/test/mjsunit/harmony/symbol-as-weakmap-key.js b/test/mjsunit/harmony/symbol-as-weakmap-key.js
@@ -99,3 +99,10 @@
   assertFalse(set.delete(key));
   assertFalse(set.has(key));
 })();
+
+(function TestFinalizationRegistryUnregister() {
+  const fr = new FinalizationRegistry(function() {});
+  const key = {};
+  fr.register(Symbol('foo'), "holdings", key);
+  fr.unregister(key);
+})();