[ddc] Reset static fields on first get or set

Fixes a memory leak issue where statics that are set but never
read before a hot restart are never reset and the functions to
set them are never garbage collected.

Update and add more tests for this specific scenario that inspect
the number of fields that will be reset during a hot restart.

Change-Id: Id5a56625279c84a37f53253a5ee667758bc22f87
Issue: https://github.com/dart-lang/sdk/issues/48349
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/232230
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Mark Zhou <markzipan@google.com>
Commit-Queue: Nicholas Shahan <nshahan@google.com>

sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart

@@ -836,7 +836,7 @@ defineLazyField(to, name, desc) => JS('', '''(() => {
     if (!savedLocals) {
       // Record the field on first execution so we can reset it later if
       // needed (hot restart).
-      $_resetFields.push(() => {
+      $resetFields.push(() => {
         init = initializer;
         value = null;
         savedLocals = false;
@@ -860,9 +860,17 @@ defineLazyField(to, name, desc) => JS('', '''(() => {
   $desc.configurable = true;
   if ($desc.set != null) {
     $desc.set = function(x) {
+      if (!savedLocals) {
+        $resetFields.push(() => {
+          init = initializer;
+          value = null;
+          savedLocals = false;
+          initialized = false;
+        });
+        savedLocals = true;
+      }
       init = null;
       value = x;
-      // savedLocals and initialized are dead since init is set to null
     };
   }
   return ${defineProperty(to, name, desc)};
@@ -873,6 +881,9 @@ defineLazyFieldOld(to, name, desc) => JS('', '''(() => {
   const initializer = $desc.get;
   let init = initializer;
   let value = null;
+  // Tracks if these local variables have been saved so they can be restored
+  // after a hot restart.
+  let savedLocals = false;
   $desc.get = function() {
     if (init == null) return value;
     let f = init;
@@ -881,10 +892,14 @@ defineLazyFieldOld(to, name, desc) => JS('', '''(() => {
 
     // On the first (non-cyclic) execution, record the field so we can reset it
     // later if needed (hot restart).
-    $_resetFields.push(() => {
-      init = initializer;
-      value = null;
-    });
+    if (!savedLocals) {
+      $resetFields.push(() => {
+        init = initializer;
+        value = null;
+        savedLocals = false;
+      });
+      savedLocals = true;
+    }
 
     // Try to evaluate the field, using try+catch to ensure we implement the
     // correct Dart error semantics.
@@ -901,6 +916,14 @@ defineLazyFieldOld(to, name, desc) => JS('', '''(() => {
   $desc.configurable = true;
   if ($desc.set != null) {
     $desc.set = function(x) {
+      if (!savedLocals) {
+        $resetFields.push(() => {
+          init = initializer;
+          value = null;
+          savedLocals = false;
+        });
+        savedLocals = true;
+      }
       init = null;
       value = x;
     };

sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/runtime.dart

@@ -183,7 +183,7 @@ final List<Object> _cacheMaps = JS('!', '[]');
 /// This is populated by [defineLazyField] and only contains fields that have
 /// been initialized.
 @notNull
-final List<void Function()> _resetFields = JS('', '[]');
+final List<void Function()> resetFields = JS('', '[]');
 
 /// A counter to track each time [hotRestart] is invoked. This is used to ensure
 /// that pending callbacks that were created on a previous iteration (e.g. a
@@ -199,8 +199,8 @@ int hotRestartIteration = 0;
 void hotRestart() {
   // TODO(sigmund): prevent DOM callbacks from firing.
   hotRestartIteration++;
-  for (var f in _resetFields) f();
-  _resetFields.clear();
+  for (var f in resetFields) f();
+  resetFields.clear();
   for (var m in _cacheMaps) JS('', '#.clear()', m);
   _cacheMaps.clear();
   JS('', '#.clear()', _nullComparisonSet);

tests/dartdevc/hot_restart_late_test.dart

@@ -6,35 +6,120 @@
 
 // Requirements=nnbd
 
-import 'package:expect/expect.dart';
-import 'dart:_runtime' as dart;
+import 'dart:_runtime' as dart show hotRestart, resetFields;
 
-late double l;
+import 'package:expect/expect.dart';
+
+late String noInitializer;
+late int withInitializer = 1;
 
 class Lates {
-  static late String s;
+  static late String noInitializer;
+  static late int withInitializer = 2;
+}
+
+class LatesGeneric<T> {
+  static late String noInitializer;
+  static late int withInitializer = 3;
 }
 
 main() {
-  Expect.throws(() => Lates.s);
-  Expect.throws(() => l);
-  Lates.s = "set";
-  l = 1.62;
-  Expect.equals(Lates.s, "set");
-  Expect.equals(l, 1.62);
+  // Read this static field first to avoid interference with other reset counts.
+  var weakNullSafety = hasUnsoundNullSafety;
+
+  // TODO(42495) `Expect.throws` contains the use of a const that triggers an
+  // extra field reset but consts are not correctly reset on hot restart so the
+  // extra reset only appears once. Perform an initial Expect.throws here to
+  // avoid confusion with the reset counts later.
+  Expect.throws(() => throw 'foo');
+
+  var resetFieldCount = dart.resetFields.length;
+
+  // Set uninitialized static late fields. Avoid calling getters for these
+  // statics to ensure they are reset even if they are never accessed.
+  noInitializer = 'set via setter';
+  Lates.noInitializer = 'Lates set via setter';
+  LatesGeneric.noInitializer = 'LatesGeneric set via setter';
+
+  // Initialized statics should contain their values.
+  Expect.equals(1, withInitializer);
+  Expect.equals(2, Lates.withInitializer);
+  Expect.equals(3, LatesGeneric.withInitializer);
+
+  // In weak null safety the late field lowering introduces a second static
+  // field that tracks if late field has been initialized thus doubling the
+  // number of expected resets.
+  //
+  // In sound null safety non-nullable fields don't require the extra static to
+  // track initialization because null is used as a sentinel value.
+  //
+  // Weak Null Safety - 12 total field resets
+  //  - 3 isSet write/resets for uninitialized field writes.
+  //  - 3 write/resets for the actual uninitialized field writes.
+  //  - 3 isSet reads/resets for initialized field reads.
+  //  - 3 reads/resets for the actual initialized field reads.
+  //
+  // Sound Null Safety - 6 total field resets:
+  //  - 3 write/resets for the actual uninitialized field writes.
+  //  - 3 reads/resets for the actual initialized field reads.
+  var expectedResets =
+      weakNullSafety ? resetFieldCount + 12 : resetFieldCount + 6;
+  Expect.equals(expectedResets, dart.resetFields.length);
 
   dart.hotRestart();
+  resetFieldCount = dart.resetFields.length;
 
-  Expect.throws(() => Lates.s);
-  Expect.throws(() => l);
-  Lates.s = "set";
-  Expect.equals(Lates.s, "set");
-  l = 1.62;
-  Expect.equals(l, 1.62);
+  // Late statics should throw on get when not initialized.
+  Expect.throws(() => noInitializer);
+  Expect.throws(() => Lates.noInitializer);
+  Expect.throws(() => LatesGeneric.noInitializer);
+
+  // Set uninitialized static late fields again.
+  noInitializer = 'set via setter';
+  Lates.noInitializer = 'Lates set via setter';
+  LatesGeneric.noInitializer = 'LatesGeneric set via setter';
+
+  // All statics should contain their set values.
+  Expect.equals('set via setter', noInitializer);
+  Expect.equals('Lates set via setter', Lates.noInitializer);
+  Expect.equals('LatesGeneric set via setter', LatesGeneric.noInitializer);
+  Expect.equals(1, withInitializer);
+  Expect.equals(2, Lates.withInitializer);
+  Expect.equals(3, LatesGeneric.withInitializer);
+
+  // Weak Null Safety - 12 total field resets
+  //  - 3 isSet write/resets for uninitialized field writes.
+  //  - 3 write/resets for the actual uninitialized field writes.
+  //  - 3 isSet reads/resets for initialized field reads.
+  //  - 3 reads/resets for the actual initialized field reads.
+  // Sound Null Safety - 6 total field resets:
+  //  - 3 write/resets for the actual uninitialized field writes.
+  //  - 3 reads/resets for the actual initialized field reads.
+  expectedResets = weakNullSafety ? resetFieldCount + 12 : resetFieldCount + 6;
+  Expect.equals(expectedResets, dart.resetFields.length);
 
   dart.hotRestart();
   dart.hotRestart();
+  resetFieldCount = dart.resetFields.length;
 
-  Expect.throws(() => Lates.s);
-  Expect.throws(() => l);
+  // Late statics should throw on get when not initialized.
+  Expect.throws(() => noInitializer);
+  Expect.throws(() => Lates.noInitializer);
+  Expect.throws(() => LatesGeneric.noInitializer);
+
+  // Initialized statics should contain their values.
+  Expect.equals(1, withInitializer);
+  Expect.equals(2, Lates.withInitializer);
+  Expect.equals(3, LatesGeneric.withInitializer);
+
+  // Weak Null Safety - 9 total field resets:
+  //  - 3 isSet reads/resets for uninitialized field reads.
+  //  - 3 isSet reads/resets for initialized field reads.
+  //  - 3 reads/resets for the actual initialized field reads.
+  //
+  // Sound Null Safety - 6 total field resets:
+  //  - 3 reads/resets for actual uninitialized field reads.
+  //  - 3 reads/resets for the actual initialized field reads.
+  expectedResets = weakNullSafety ? resetFieldCount + 9 : resetFieldCount + 6;
+  Expect.equals(expectedResets, dart.resetFields.length);
 }

tests/dartdevc/hot_restart_static_test.dart

@@ -0,0 +1,84 @@
+// Copyright (c) 2022, 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.
+
+// Tests that static fields are properly reset after hot restarts.
+
+// Requirements=nnbd
+
+import 'dart:_runtime' as dart show hotRestart, resetFields;
+
+import 'package:expect/expect.dart';
+
+String? noInitializer;
+int withInitializer = 1;
+
+class Statics {
+  static String? noInitializer;
+  static int withInitializer = 2;
+}
+
+class StaticsGeneric<T> {
+  static String? noInitializer;
+  static int withInitializer = 3;
+}
+
+main() {
+  var resetFieldCount = dart.resetFields.length;
+
+  // Set static fields without explicit initializers. Avoid calling getters for
+  // these statics to ensure they are reset even if they are never accessed.
+  noInitializer = 'set via setter';
+  Statics.noInitializer = 'Statics set via setter';
+  StaticsGeneric.noInitializer = 'StaticsGeneric set via setter';
+
+  // Initialized statics should contain their values.
+  Expect.equals(1, withInitializer);
+  Expect.equals(2, Statics.withInitializer);
+  Expect.equals(3, StaticsGeneric.withInitializer);
+
+  // Six new field resets from 3 setter calls and 3 getter calls.
+  var expectedResets = resetFieldCount + 6;
+  Expect.equals(expectedResets, dart.resetFields.length);
+
+  dart.hotRestart();
+  resetFieldCount = dart.resetFields.length;
+
+  // Uninitialized statics have been reset to their implicit null initial state.
+  Expect.equals(null, noInitializer);
+  Expect.equals(null, Statics.noInitializer);
+  Expect.equals(null, StaticsGeneric.noInitializer);
+
+  noInitializer = 'set via setter';
+  Statics.noInitializer = 'Statics set via setter';
+  StaticsGeneric.noInitializer = 'StaticsGeneric set via setter';
+
+  // All statics should contain their set values.
+  Expect.equals('set via setter', noInitializer);
+  Expect.equals('Statics set via setter', Statics.noInitializer);
+  Expect.equals('StaticsGeneric set via setter', StaticsGeneric.noInitializer);
+  Expect.equals(1, withInitializer);
+  Expect.equals(2, Statics.withInitializer);
+  Expect.equals(3, StaticsGeneric.withInitializer);
+
+  // Six total new field resets despite getter and setter calls on the same
+  // static fields.
+  expectedResets = resetFieldCount + 6;
+  Expect.equals(expectedResets, dart.resetFields.length);
+
+  dart.hotRestart();
+  dart.hotRestart();
+  resetFieldCount = dart.resetFields.length;
+
+  // All statics should contain their initial values.
+  Expect.equals(null, noInitializer);
+  Expect.equals(null, Statics.noInitializer);
+  Expect.equals(null, StaticsGeneric.noInitializer);
+  Expect.equals(1, withInitializer);
+  Expect.equals(2, Statics.withInitializer);
+  Expect.equals(3, StaticsGeneric.withInitializer);
+
+  // Six new field resets from 6 getter calls.
+  expectedResets = resetFieldCount + 6;
+  Expect.equals(expectedResets, dart.resetFields.length);
+}

tests/dartdevc_2/hot_restart_static_test.dart

@@ -0,0 +1,84 @@
+// Copyright (c) 2022, 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
+
+// Tests that static fields are properly reset after hot restarts.
+
+import 'dart:_runtime' as dart show hotRestart, resetFields;
+
+import 'package:expect/expect.dart';
+
+String noInitializer;
+int withInitializer = 1;
+
+class Statics {
+  static String noInitializer;
+  static int withInitializer = 2;
+}
+
+class StaticsGeneric<T> {
+  static String noInitializer;
+  static int withInitializer = 3;
+}
+
+main() {
+  var resetFieldCount = dart.resetFields.length;
+
+  // Set static fields without explicit initializers. Avoid calling getters for
+  // these statics to ensure they are reset even if they are never accessed.
+  noInitializer = 'set via setter';
+  Statics.noInitializer = 'Statics set via setter';
+  StaticsGeneric.noInitializer = 'StaticsGeneric set via setter';
+
+  // Initialized statics should contain their values.
+  Expect.equals(1, withInitializer);
+  Expect.equals(2, Statics.withInitializer);
+  Expect.equals(3, StaticsGeneric.withInitializer);
+
+  // Six new field resets from 3 setter calls and 3 getter calls.
+  var expectedResets = resetFieldCount + 6;
+  Expect.equals(expectedResets, dart.resetFields.length);
+
+  dart.hotRestart();
+  resetFieldCount = dart.resetFields.length;
+
+  // Uninitialized statics have been reset to their implicit null initial state.
+  Expect.equals(null, noInitializer);
+  Expect.equals(null, Statics.noInitializer);
+  Expect.equals(null, StaticsGeneric.noInitializer);
+
+  noInitializer = 'set via setter';
+  Statics.noInitializer = 'Statics set via setter';
+  StaticsGeneric.noInitializer = 'StaticsGeneric set via setter';
+
+  // All statics should contain their set values.
+  Expect.equals('set via setter', noInitializer);
+  Expect.equals('Statics set via setter', Statics.noInitializer);
+  Expect.equals('StaticsGeneric set via setter', StaticsGeneric.noInitializer);
+  Expect.equals(1, withInitializer);
+  Expect.equals(2, Statics.withInitializer);
+  Expect.equals(3, StaticsGeneric.withInitializer);
+
+  // Six total new field resets despite getter and setter calls on the same
+  // static fields.
+  expectedResets = resetFieldCount + 6;
+  Expect.equals(expectedResets, dart.resetFields.length);
+
+  dart.hotRestart();
+  dart.hotRestart();
+  resetFieldCount = dart.resetFields.length;
+
+  // All statics should contain their initial values.
+  Expect.equals(null, noInitializer);
+  Expect.equals(null, Statics.noInitializer);
+  Expect.equals(null, StaticsGeneric.noInitializer);
+  Expect.equals(1, withInitializer);
+  Expect.equals(2, Statics.withInitializer);
+  Expect.equals(3, StaticsGeneric.withInitializer);
+
+  // Six new field resets from 6 getter calls.
+  expectedResets = resetFieldCount + 6;
+  Expect.equals(expectedResets, dart.resetFields.length);
+}