[vm] Ensure we only enqueue maps/sets to be rehashed if we know it got copied successfully

Closes #51226

TEST=vm/dart{,_2}/regress_51226_test

Change-Id: I1da2dfe45863d5f1004efde3a4ecaae89ffd62fa
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/280469
Reviewed-by: Ryan Macnak <[email protected]>
Commit-Queue: Martin Kustermann <[email protected]>

Author: mkustermann

runtime/tests/vm/dart/regress_51226_test.dart

@@ -0,0 +1,39 @@
+// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'dart:isolate';
+
+import 'package:expect/expect.dart';
+
+main() async {
+  // Make large objects whose objects object can be copied in fast path but backing
+  // store cannot.
+  final foo = Foo();
+  final objects = <dynamic>{};
+  for (int i = 0; i < 1024 * 1024; ++i) {
+    objects.add(i);
+  }
+  // Ensure there's at least one element that needs re-hashing when copying.
+  objects.add(foo);
+  foo.hashQueries = 0;
+
+  final copyOfFoo = (await copy(objects)).last as Foo;
+  Expect.equals(1, copyOfFoo.hashQueries);
+}
+
+Future<T> copy<T>(T value) async {
+  final rp = ReceivePort();
+  final sp = rp.sendPort;
+  sp.send(value);
+  return (await rp.first) as T;
+}
+
+class Foo {
+  int hashQueries = 0;
+
+  int get hashCode {
+    hashQueries++;
+    return super.hashCode;
+  }
+}

runtime/tests/vm/dart_2/regress_51226_test.dart

@@ -0,0 +1,41 @@
+// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// @dart=2.9
+
+import 'dart:isolate';
+
+import 'package:expect/expect.dart';
+
+main() async {
+  // Make large objects whose objects object can be copied in fast path but backing
+  // store cannot.
+  final foo = Foo();
+  final objects = <dynamic>{};
+  for (int i = 0; i < 1024 * 1024; ++i) {
+    objects.add(i);
+  }
+  // Ensure there's at least one element that needs re-hashing when copying.
+  objects.add(foo);
+  foo.hashQueries = 0;
+
+  final copyOfFoo = (await copy(objects)).last as Foo;
+  Expect.equals(1, copyOfFoo.hashQueries);
+}
+
+Future<T> copy<T>(T value) async {
+  final rp = ReceivePort();
+  final sp = rp.sendPort;
+  sp.send(value);
+  return (await rp.first) as T;
+}
+
+class Foo {
+  int hashQueries = 0;
+
+  int get hashCode {
+    hashQueries++;
+    return super.hashCode;
+  }
+}

runtime/vm/object_graph_copy.cc

@@ -1476,7 +1476,6 @@ class ObjectCopy : public Base {
       to_untagged->hash_mask_ = Smi::New(0);
       to_untagged->index_ = TypedData::RawCast(Object::null());
       to_untagged->deleted_keys_ = Smi::New(0);
-      Base::EnqueueObjectToRehash(to);
     }
 
     // From this point on we shouldn't use the raw pointers, since GC might
@@ -1499,6 +1498,10 @@ class ObjectCopy : public Base {
     Base::StoreCompressedPointersNoBarrier(
         from, to, OFFSET_OF(UntaggedLinkedHashBase, used_data_),
         OFFSET_OF(UntaggedLinkedHashBase, used_data_));
+
+    if (Base::exception_msg_ == nullptr && needs_rehashing) {
+      Base::EnqueueObjectToRehash(to);
+    }
   }
 
   void CopyMap(typename Types::Map from, typename Types::Map to) {