[WeakRefs] Update cleanupSome to be spec compliant

Make sure to use the callback passed to cleanupSome

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

src/builtins/builtins-weak-refs.cc

@@ -91,14 +91,34 @@ BUILTIN(FinalizationGroupCleanupSome) {
   HandleScope scope(isolate);
   const char* method_name = "FinalizationGroup.prototype.cleanupSome";
 
+  // 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);
 
-  // TODO(marja, gsathya): Add missing "cleanup" callback.
+  Handle<Object> callback(finalization_group->cleanup(), isolate);
+  Handle<Object> callback_obj = args.atOrUndefined(isolate, 1);
+
+  // 4. If callback is not undefined and IsCallable(callback) is
+  //    false, throw a TypeError exception.
+  if (!callback_obj->IsUndefined(isolate)) {
+    if (!callback_obj->IsCallable()) {
+      THROW_NEW_ERROR_RETURN_FAILURE(
+          isolate,
+          NewTypeError(MessageTemplate::kWeakRefsCleanupMustBeCallable));
+    }
+    callback = callback_obj;
+  }
 
   // Don't do set_scheduled_for_cleanup(false); we still have the microtask
   // scheduled and don't want to schedule another one in case the user never
   // executes microtasks.
-  JSFinalizationGroup::Cleanup(finalization_group, isolate);
+  JSFinalizationGroup::Cleanup(isolate, finalization_group, callback);
+
   return ReadOnlyRoots(isolate).undefined_value();
 }
 

src/init/bootstrapper.cc

@@ -4310,7 +4310,7 @@ void Genesis::InitializeGlobal_harmony_weak_refs() {
 
     SimpleInstallFunction(isolate(), finalization_group_prototype,
                           "cleanupSome",
-                          Builtins::kFinalizationGroupCleanupSome, 0, false);
+                          Builtins::kFinalizationGroupCleanupSome, 1, false);
   }
   {
     // Create %WeakRefPrototype%

src/objects/js-weak-refs.h

@@ -57,8 +57,9 @@ class JSFinalizationGroup : public JSObject {
 
   // Constructs an iterator for the WeakCells in the cleared_cells list and
   // calls the user's cleanup function.
-  static void Cleanup(Handle<JSFinalizationGroup> finalization_group,
-                      Isolate* isolate);
+  static void Cleanup(Isolate* isolate,
+                      Handle<JSFinalizationGroup> finalization_group,
+                      Handle<Object> callback);
 
   // Layout description.
   DEFINE_FIELD_OFFSET_CONSTANTS(JSObject::kHeaderSize,

src/objects/objects.cc

@@ -8112,7 +8112,9 @@ HashTable<NameDictionary, NameDictionaryShape>::Shrink(Isolate* isolate,
                                                        int additionalCapacity);
 
 void JSFinalizationGroup::Cleanup(
-    Handle<JSFinalizationGroup> finalization_group, Isolate* isolate) {
+    Isolate* isolate, Handle<JSFinalizationGroup> finalization_group,
+    Handle<Object> cleanup) {
+  DCHECK(cleanup->IsCallable());
   // It's possible that the cleared_cells list is empty, since
   // FinalizationGroup.unregister() removed all its elements before this task
   // ran. In that case, don't call the cleanup function.
@@ -8130,7 +8132,6 @@ void JSFinalizationGroup::Cleanup(
               Handle<AllocationSite>::null()));
       iterator->set_finalization_group(*finalization_group);
     }
-    Handle<Object> cleanup(finalization_group->cleanup(), isolate);
 
     v8::TryCatch try_catch(reinterpret_cast<v8::Isolate*>(isolate));
     v8::Local<v8::Value> result;

src/runtime/runtime-weak-refs.cc

@@ -20,7 +20,8 @@ RUNTIME_FUNCTION(Runtime_FinalizationGroupCleanupJob) {
   CONVERT_ARG_HANDLE_CHECKED(JSFinalizationGroup, finalization_group, 0);
   finalization_group->set_scheduled_for_cleanup(false);
 
-  JSFinalizationGroup::Cleanup(finalization_group, isolate);
+  Handle<Object> cleanup(finalization_group->cleanup(), isolate);
+  JSFinalizationGroup::Cleanup(isolate, finalization_group, cleanup);
   return ReadOnlyRoots(isolate).undefined_value();
 }
 

test/mjsunit/harmony/weakrefs/basics.js

@@ -138,3 +138,15 @@
   let rv = FinalizationGroup.prototype.cleanupSome.call(fg);
   assertEquals(undefined, rv);
 })();
+
+(function TestCleanupSomeWithNonCallableCallback() {
+  let fg = new FinalizationGroup(() => {});
+  assertThrows(() => fg.cleanupSome(1), TypeError);
+  assertThrows(() => fg.cleanupSome(1n), TypeError);
+  assertThrows(() => fg.cleanupSome(Symbol()), TypeError);
+  assertThrows(() => fg.cleanupSome({}), TypeError);
+  assertThrows(() => fg.cleanupSome('foo'), TypeError);
+  assertThrows(() => fg.cleanupSome(true), TypeError);
+  assertThrows(() => fg.cleanupSome(false), TypeError);
+  assertThrows(() => fg.cleanupSome(null), TypeError);
+})();

test/mjsunit/harmony/weakrefs/cleanupsome-2.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 --allow-natives-syntax
+
+let cleanup_count = 0;
+let cleanup_holdings = [];
+let cleanup = function(iter) {
+  for (holdings of iter) {
+    cleanup_holdings.push(holdings);
+  }
+  ++cleanup_count;
+}
+
+let fg = new FinalizationGroup(cleanup);
+(function() {
+  let o = {};
+  fg.register(o, "holdings");
+
+  assertEquals(0, cleanup_count);
+})();
+
+// GC will detect o as dead.
+gc();
+
+// passing no callback, should trigger cleanup function
+fg.cleanupSome();
+assertEquals(1, cleanup_count);
+assertEquals(1, cleanup_holdings.length);
+assertEquals("holdings", cleanup_holdings[0]);

test/mjsunit/harmony/weakrefs/cleanupsome.js

@@ -2,11 +2,15 @@
 // 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
+// Flags: --harmony-weak-refs --expose-gc --noincremental-marking --allow-natives-syntax
 
 let cleanup_count = 0;
 let cleanup_holdings = [];
 let cleanup = function(iter) {
+  %AbortJS("shouldn't be called");
+}
+
+let cleanup2 = function(iter) {
   for (holdings of iter) {
     cleanup_holdings.push(holdings);
   }
@@ -19,14 +23,14 @@ let fg = new FinalizationGroup(cleanup);
   fg.register(o, "holdings");
 
   // cleanupSome won't do anything since there are no reclaimed targets.
-  fg.cleanupSome();
+  fg.cleanupSome(cleanup2);
   assertEquals(0, cleanup_count);
 })();
 
 // GC will detect o as dead.
 gc();
 
-fg.cleanupSome();
+fg.cleanupSome(cleanup2);
 assertEquals(1, cleanup_count);
 assertEquals(1, cleanup_holdings.length);
 assertEquals("holdings", cleanup_holdings[0]);