[WeakRefs] Make unregister spec compliant

- Return true or false, not undefined
- Check that unregister token is an object

Bug: v8:8179
Change-Id: I1a4ff7730158dba16efb552fb2f4892c8d31412c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1653120Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62135}

src/builtins/builtins-weak-refs.cc

@@ -80,11 +80,29 @@ BUILTIN(FinalizationGroupUnregister) {
   HandleScope scope(isolate);
   const char* method_name = "FinalizationGroup.prototype.unregister";
 
+  // 1. Let finalizationGroup be the this value.
+  //
+  // 2. If Type(finalizationGroup) is not Object, throw a TypeError
+  //    exception.
+  //
+  // 3. If finalizationGroup does not have a [[Cells]] internal slot,
+  //    throw a TypeError exception.
   CHECK_RECEIVER(JSFinalizationGroup, finalization_group, method_name);
 
-  Handle<Object> key = args.atOrUndefined(isolate, 1);
-  JSFinalizationGroup::Unregister(finalization_group, key, isolate);
-  return ReadOnlyRoots(isolate).undefined_value();
+  Handle<Object> unregister_token = args.atOrUndefined(isolate, 1);
+
+  // 4. If Type(unregisterToken) is not Object, throw a TypeError exception.
+  if (!unregister_token->IsJSReceiver()) {
+    THROW_NEW_ERROR_RETURN_FAILURE(
+        isolate,
+        NewTypeError(MessageTemplate::kWeakRefsUnregisterTokenMustBeObject,
+                     unregister_token));
+  }
+
+  bool success = JSFinalizationGroup::Unregister(
+      finalization_group, Handle<JSReceiver>::cast(unregister_token), isolate);
+
+  return *isolate->factory()->ToBoolean(success);
 }
 
 BUILTIN(FinalizationGroupCleanupSome) {

src/execution/message-template.h

@@ -562,6 +562,8 @@ namespace internal {
   T(TraceEventPhaseError, "Trace event phase must be a number.")               \
   T(TraceEventIDError, "Trace event id must be a number.")                     \
   /* Weak refs */                                                              \
+  T(WeakRefsUnregisterTokenMustBeObject,                                       \
+    "unregisterToken ('%') must be an object")                                 \
   T(WeakRefsCleanupMustBeCallable,                                             \
     "FinalizationGroup: cleanup must be callable")                             \
   T(WeakRefsRegisterTargetMustBeObject,                                        \

src/objects/js-weak-refs-inl.h

@@ -97,16 +97,16 @@ void JSFinalizationGroup::Register(
   }
 }
 
-void JSFinalizationGroup::Unregister(
-    Handle<JSFinalizationGroup> finalization_group, Handle<Object> key,
-    Isolate* isolate) {
+bool JSFinalizationGroup::Unregister(
+    Handle<JSFinalizationGroup> finalization_group,
+    Handle<JSReceiver> unregister_token, Isolate* isolate) {
   // Iterate through the doubly linked list of WeakCells associated with the
   // key. Each WeakCell will be in the "active_cells" or "cleared_cells" list of
   // its FinalizationGroup; remove it from there.
   if (!finalization_group->key_map().IsUndefined(isolate)) {
     Handle<ObjectHashTable> key_map =
         handle(ObjectHashTable::cast(finalization_group->key_map()), isolate);
-    Object value = key_map->Lookup(key);
+    Object value = key_map->Lookup(unregister_token);
     Object undefined = ReadOnlyRoots(isolate).undefined_value();
     while (value.IsWeakCell()) {
       WeakCell weak_cell = WeakCell::cast(value);
@@ -116,9 +116,13 @@ void JSFinalizationGroup::Unregister(
       weak_cell.set_key_list_next(undefined);
     }
     bool was_present;
-    key_map = ObjectHashTable::Remove(isolate, key_map, key, &was_present);
+    key_map = ObjectHashTable::Remove(isolate, key_map, unregister_token,
+                                      &was_present);
     finalization_group->set_key_map(*key_map);
+    return was_present;
   }
+
+  return false;
 }
 
 bool JSFinalizationGroup::NeedsCleanup() const {

src/objects/js-weak-refs.h

@@ -41,8 +41,9 @@ class JSFinalizationGroup : public JSObject {
                               Handle<JSReceiver> target,
                               Handle<Object> holdings, Handle<Object> key,
                               Isolate* isolate);
-  inline static void Unregister(Handle<JSFinalizationGroup> finalization_group,
-                                Handle<Object> key, Isolate* isolate);
+  inline static bool Unregister(Handle<JSFinalizationGroup> finalization_group,
+                                Handle<JSReceiver> unregister_token,
+                                Isolate* isolate);
 
   // Returns true if the cleared_cells list is non-empty.
   inline bool NeedsCleanup() const;

test/message/fail/weak-refs-unregister.js

+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --harmony-weak-refs
+
+let fg = new FinalizationGroup(() => {});
+fg.unregister(1);

test/message/fail/weak-refs-unregister.out

+*%(basename)s:8: TypeError: unregisterToken ('1') must be an object
+fg.unregister(1);
+   ^
+TypeError: unregisterToken ('1') must be an object
+    at FinalizationGroup.unregister (<anonymous>)
+    at *%(basename)s:8:4

test/mjsunit/harmony/weakrefs/basics.js

@@ -85,7 +85,25 @@
 
 (function TestUnregisterWithNonExistentKey() {
   let fg = new FinalizationGroup(() => {});
-  fg.unregister({"k": "whatever"});
+  let success = fg.unregister({"k": "whatever"});
+  assertFalse(success);
+})();
+
+(function TestUnregisterWithNonFinalizationGroup() {
+  assertThrows(() => FinalizationGroup.prototype.unregister.call({}, {}),
+               TypeError);
+})();
+
+(function TestUnregisterWithNonObjectUnregisterToken() {
+  let fg = new FinalizationGroup(() => {});
+  assertThrows(() => fg.unregister(1), TypeError);
+  assertThrows(() => fg.unregister(1n), TypeError);
+  assertThrows(() => fg.unregister('one'), TypeError);
+  assertThrows(() => fg.unregister(Symbol()), TypeError);
+  assertThrows(() => fg.unregister(true), TypeError);
+  assertThrows(() => fg.unregister(false), TypeError);
+  assertThrows(() => fg.unregister(undefined), TypeError);
+  assertThrows(() => fg.unregister(null), TypeError);
 })();
 
 (function TestWeakRefConstructor() {

test/mjsunit/harmony/weakrefs/unregister-after-cleanup.js

@@ -36,7 +36,8 @@ let timeout_func = function() {
   assertEquals(1, cleanup_holdings_count);
 
   // Unregister an already iterated over weak reference.
-  fg.unregister(key);
+  let success = fg.unregister(key);
+  assertFalse(success);
 
   // Assert that it didn't do anything.
   setTimeout(() => { assertEquals(1, cleanup_call_count); }, 0);

test/mjsunit/harmony/weakrefs/unregister-before-cleanup.js

@@ -19,8 +19,8 @@ let key = {"k": "this is the key"};
   fg.register(object, "my holdings", key);
 
   // Clear the WeakCell before the GC has a chance to discover it.
-  let return_value = fg.unregister(key);
-  assertEquals(undefined, return_value);
+  let success = fg.unregister(key);
+  assertTrue(success);
 
   // object goes out of scope.
 })();

test/mjsunit/harmony/weakrefs/unregister-called-twice.js

@@ -19,10 +19,12 @@ let key = {"k": "this is the key"};
   fg.register(object, "holdings", key);
 
   // Unregister before the GC has a chance to discover the object.
-  fg.unregister(key);
+  let success = fg.unregister(key);
+  assertTrue(success);
 
   // Call unregister again (just to assert we handle this gracefully).
-  fg.unregister(key);
+  success = fg.unregister(key);
+  assertFalse(success);
 
   // object goes out of scope.
 })();

test/mjsunit/harmony/weakrefs/unregister-inside-cleanup1.js

@@ -8,7 +8,8 @@ let cleanup_call_count = 0;
 let cleanup_holdings_count = 0;
 let cleanup = function(iter) {
   // Unregister before we've iterated through the holdings.
-  fg.unregister(key);
+  let success = fg.unregister(key);
+  assertTrue(success);
 
   for (wc of iter) {
     ++cleanup_holdings_count;

test/mjsunit/harmony/weakrefs/unregister-inside-cleanup2.js

@@ -9,7 +9,9 @@ let cleanup_holdings_count = 0;
 let cleanup = function(iter) {
   for (holdings of iter) {
     assertEquals(holdings, "holdings");
-    fg.unregister(key);
+    let success = fg.unregister(key);
+    assertFalse(success);
+
     ++cleanup_holdings_count;
   }
   ++cleanup_call_count;

test/mjsunit/harmony/weakrefs/unregister-inside-cleanup3.js

@@ -12,7 +12,8 @@ let cleanup = function(iter) {
     ++cleanup_holdings_count;
   }
   // Unregister an already iterated over weak reference.
-  fg.unregister(key);
+  let success = fg.unregister(key);
+  assertFalse(success);
   ++cleanup_call_count;
 }
 

test/mjsunit/harmony/weakrefs/unregister-inside-cleanup4.js

@@ -10,10 +10,12 @@ let cleanup = function(iter) {
   for (holdings of iter) {
     // See which target we're iterating over and unregister the other one.
     if (holdings == 1) {
-      fg.unregister(key2);
+      let success = fg.unregister(key2);
+      assertTrue(success);
     } else {
       assertSame(holdings, 2);
-      fg.unregister(key1);
+      let success = fg.unregister(key1);
+      assertTrue(success);
     }
     ++cleanup_holdings_count;
   }

test/mjsunit/harmony/weakrefs/unregister-inside-cleanup5.js

+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --harmony-weak-refs --expose-gc --noincremental-marking
+
+let cleanup_call_count = 0;
+let cleanup_holdings_count = 0;
+let cleanup = function(iter) {
+  for (holdings of iter) {
+    assertEquals(holdings, "holdings");
+
+    // There's one more object with the same key that we haven't
+    // iterated over yet so we should be able to unregister the
+    // callback for that one.
+    let success = fg.unregister(key);
+    assertTrue(success);
+
+    ++cleanup_holdings_count;
+  }
+  ++cleanup_call_count;
+}
+
+let fg = new FinalizationGroup(cleanup);
+// Create an object and register it in the FinalizationGroup. The object needs to be inside
+// a closure so that we can reliably kill them!
+let key = {"k": "this is the key"};
+
+(function() {
+  let object = {};
+  let object2 = {};
+  fg.register(object, "holdings", key);
+  fg.register(object2, "holdings", key);
+
+  // object goes out of scope.
+})();
+
+// This GC will discover dirty WeakCells and schedule cleanup.
+gc();
+assertEquals(0, cleanup_call_count);
+
+// Assert that the cleanup function was called and iterated the WeakCell.
+let timeout_func = function() {
+  assertEquals(1, cleanup_call_count);
+  assertEquals(1, cleanup_holdings_count);
+}
+
+setTimeout(timeout_func, 0);

test/mjsunit/harmony/weakrefs/unregister-many.js

@@ -31,7 +31,8 @@ let key2 = {"k": "key2"};
   fg.register(object2, "holdings2", key2);
 
   // Unregister before the GC has a chance to discover the objects.
-  fg.unregister(key1);
+  let success = fg.unregister(key1);
+  assertTrue(success);
 
   // objects go out of scope.
 })();

test/mjsunit/harmony/weakrefs/unregister-when-cleanup-already-scheduled.js

@@ -26,7 +26,8 @@ gc();
 assertEquals(0, cleanup_call_count);
 
 // Unregister the object from the FinalizationGroup before cleanup has ran.
-fg.unregister(key);
+let success = fg.unregister(key);
+assertTrue(success);
 
 // Assert that the cleanup function won't be called.
 let timeout_func = function() {