[web canvaskit] fix SkiaObjectBox instrumentation (flutter/engine#26622)

* [web canvaskit] fix SkiaObjectBox instrumentation

engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/skia_object_cache.dart

@@ -351,16 +351,16 @@ class SkiaObjectBox<R extends StackTraceDebugger, T extends Object>
       R debugReferrer, T initialValue, this._resurrector) {
     assert(!browserSupportsFinalizationRegistry);
     _initialize(debugReferrer, initialValue);
-    SkiaObjects.manageExpensive(this);
-  }
-
-  void _initialize(R debugReferrer, T initialValue) {
-    _update(initialValue);
     if (Instrumentation.enabled) {
       Instrumentation.instance.incrementCounter(
         '${_skDeletable?.constructor.name} created',
       );
     }
+    SkiaObjects.manageExpensive(this);
+  }
+
+  void _initialize(R debugReferrer, T initialValue) {
+    _update(initialValue);
     if (assertionsEnabled) {
       debugReferrers.add(debugReferrer);
     }

engine/src/flutter/lib/web_ui/lib/src/engine/profiler.dart

@@ -230,7 +230,20 @@ class Instrumentation {
   /// Whether instrumentation is enabled.
   ///
   /// Check this value before calling any other methods in this class.
-  static const bool enabled = const bool.fromEnvironment(
+  static bool get enabled => _enabled;
+  static set enabled(bool value) {
+    if (_enabled == value) {
+      return;
+    }
+
+    if (!value) {
+      _instance._counters.clear();
+      _instance._printTimer = null;
+    }
+
+    _enabled = value;
+  }
+  static bool _enabled = const bool.fromEnvironment(
     'FLUTTER_WEB_ENABLE_INSTRUMENTATION',
     defaultValue: false,
   );
@@ -245,7 +258,7 @@ class Instrumentation {
 
   static void _checkInstrumentationEnabled() {
     if (!enabled) {
-      throw Exception(
+      throw StateError(
         'Cannot use Instrumentation unless it is enabled. '
         'You can enable it by setting the `FLUTTER_WEB_ENABLE_INSTRUMENTATION` '
         'environment variable to true, or by passing '
@@ -255,7 +268,10 @@ class Instrumentation {
     }
   }
 
+  Map<String, int> get debugCounters => _counters;
   final Map<String, int> _counters = <String, int>{};
+
+  Timer? get debugPrintTimer => _printTimer;
   Timer? _printTimer;
 
   /// Increments the count of a particular event by one.
@@ -266,7 +282,11 @@ class Instrumentation {
     _printTimer ??= Timer(
       const Duration(seconds: 2),
       () {
+        if (_printTimer == null || !_enabled) {
+          return;
+        }
         final StringBuffer message = StringBuffer('Engine counters:\n');
+        // Entries are sorted for readability and testability.
         final List<MapEntry<String, int>> entries = _counters.entries.toList()
           ..sort((MapEntry<String, int> a, MapEntry<String, int> b) {
             return a.key.compareTo(b.key);

engine/src/flutter/lib/web_ui/test/canvaskit/skia_objects_cache_test.dart

@@ -11,6 +11,7 @@ import 'package:ui/src/engine.dart';
 import 'package:ui/ui.dart';
 
 import '../matchers.dart';
+import '../spy.dart';
 import 'common.dart';
 
 void main() {
@@ -114,44 +115,64 @@ void _tests() {
 
   group(SkiaObjectBox, () {
     test('Records stack traces and respects refcounts', () async {
-      TestSkDeletable.deleteCount = 0;
-      TestBoxWrapper.resurrectCount = 0;
-      final TestBoxWrapper original = TestBoxWrapper();
+      final ZoneSpy spy = ZoneSpy();
+      spy.run(() {
+        Instrumentation.enabled = true;
+        TestSkDeletable.deleteCount = 0;
+        TestBoxWrapper.resurrectCount = 0;
+        final TestBoxWrapper original = TestBoxWrapper();
 
-      expect(original.box.debugGetStackTraces().length, 1);
-      expect(original.box.refCount, 1);
-      expect(original.box.isDeletedPermanently, false);
+        expect(original.box.debugGetStackTraces().length, 1);
+        expect(original.box.refCount, 1);
+        expect(original.box.isDeletedPermanently, false);
 
-      final TestBoxWrapper clone = original.clone();
-      expect(clone.box, same(original.box));
-      expect(clone.box.debugGetStackTraces().length, 2);
-      expect(clone.box.refCount, 2);
-      expect(original.box.debugGetStackTraces().length, 2);
-      expect(original.box.refCount, 2);
-      expect(original.box.isDeletedPermanently, false);
+        final TestBoxWrapper clone = original.clone();
+        expect(clone.box, same(original.box));
+        expect(clone.box.debugGetStackTraces().length, 2);
+        expect(clone.box.refCount, 2);
+        expect(original.box.debugGetStackTraces().length, 2);
+        expect(original.box.refCount, 2);
+        expect(original.box.isDeletedPermanently, false);
 
-      original.dispose();
+        original.dispose();
 
-      testCollector.collectNow();
-      expect(TestSkDeletable.deleteCount, 0);
+        testCollector.collectNow();
+        expect(TestSkDeletable.deleteCount, 0);
 
-      expect(clone.box.debugGetStackTraces().length, 1);
-      expect(clone.box.refCount, 1);
-      expect(original.box.debugGetStackTraces().length, 1);
-      expect(original.box.refCount, 1);
+        spy.fakeAsync.elapse(const Duration(seconds: 2));
+        expect(
+          spy.printLog,
+          ['Engine counters:\n'
+           '  TestSkDeletable created: 1\n'],
+        );
 
-      clone.dispose();
-      expect(clone.box.debugGetStackTraces().length, 0);
-      expect(clone.box.refCount, 0);
-      expect(original.box.debugGetStackTraces().length, 0);
-      expect(original.box.refCount, 0);
-      expect(original.box.isDeletedPermanently, true);
+        expect(clone.box.debugGetStackTraces().length, 1);
+        expect(clone.box.refCount, 1);
+        expect(original.box.debugGetStackTraces().length, 1);
+        expect(original.box.refCount, 1);
 
-      testCollector.collectNow();
-      expect(TestSkDeletable.deleteCount, 1);
-      expect(TestBoxWrapper.resurrectCount, 0);
+        clone.dispose();
+        expect(clone.box.debugGetStackTraces().length, 0);
+        expect(clone.box.refCount, 0);
+        expect(original.box.debugGetStackTraces().length, 0);
+        expect(original.box.refCount, 0);
+        expect(original.box.isDeletedPermanently, true);
 
-      expect(() => clone.box.unref(clone), throwsAssertionError);
+        testCollector.collectNow();
+        expect(TestSkDeletable.deleteCount, 1);
+        expect(TestBoxWrapper.resurrectCount, 0);
+
+        expect(() => clone.box.unref(clone), throwsAssertionError);
+        spy.printLog.clear();
+        spy.fakeAsync.elapse(const Duration(seconds: 2));
+        expect(
+          spy.printLog,
+          ['Engine counters:\n'
+           '  TestSkDeletable created: 1\n'
+           '  TestSkDeletable deleted: 1\n'],
+        );
+        Instrumentation.enabled = false;
+      });
     });
 
     test('Can resurrect Skia objects', () async {

engine/src/flutter/lib/web_ui/test/engine/profiler_test.dart

@@ -12,11 +12,23 @@ import 'package:test/test.dart';
 import 'package:ui/src/engine.dart';
 import 'package:ui/ui.dart';
 
+import '../spy.dart';
+
 void main() {
   internalBootstrapBrowserTest(() => testMain);
 }
 
 void testMain() {
+  group('$Profiler', () {
+    _profilerTests();
+  });
+
+  group('$Instrumentation', () {
+    _instrumentationTests();
+  });
+}
+
+void _profilerTests() {
   setUp(() {
     Profiler.isBenchmarkMode = true;
     Profiler.ensureInitialized();
@@ -73,6 +85,52 @@ void testMain() {
   });
 }
 
+void _instrumentationTests() {
+  setUp(() {
+    Instrumentation.enabled = false;
+  });
+
+  tearDown(() {
+    Instrumentation.enabled = false;
+  });
+
+  test('when disabled throws instead of initializing', () {
+    expect(() => Instrumentation.instance, throwsStateError);
+  });
+
+  test('when disabled throws instead of incrementing counter', () {
+    Instrumentation.enabled = true;
+    final Instrumentation instrumentation = Instrumentation.instance;
+    Instrumentation.enabled = false;
+    expect(() => instrumentation.incrementCounter('test'), throwsStateError);
+  });
+
+  test('when enabled increments counter', () {
+    final ZoneSpy spy = ZoneSpy();
+    spy.run(() {
+      Instrumentation.enabled = true;
+      final Instrumentation instrumentation = Instrumentation.instance;
+      expect(instrumentation.debugPrintTimer, isNull);
+      instrumentation.incrementCounter('foo');
+      expect(instrumentation.debugPrintTimer, isNotNull);
+      instrumentation.incrementCounter('foo');
+      instrumentation.incrementCounter('bar');
+      expect(spy.printLog, isEmpty);
+
+      expect(instrumentation.debugPrintTimer, isNotNull);
+      spy.fakeAsync.elapse(const Duration(seconds: 2));
+      expect(instrumentation.debugPrintTimer, isNull);
+      expect(spy.printLog, hasLength(1));
+      expect(
+        spy.printLog.single,
+        'Engine counters:\n'
+        '  bar: 1\n'
+        '  foo: 2\n',
+      );
+    });
+  });
+}
+
 class BenchmarkDatapoint {
   BenchmarkDatapoint(this.name, this.value);
 

engine/src/flutter/lib/web_ui/test/spy.dart

@@ -2,8 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'dart:async';
 import 'dart:typed_data';
 
+import 'package:quiver/testing/async.dart';
 import 'package:ui/src/engine.dart' hide window;
 import 'package:ui/ui.dart';
 
@@ -67,3 +69,22 @@ class PlatformMessagesSpy {
     window.onPlatformMessage = _backup;
   }
 }
+
+/// Runs code in a [FakeAsync] zone and spies on what's going on in it.
+class ZoneSpy {
+  final FakeAsync fakeAsync = FakeAsync();
+  final List<String> printLog = <String>[];
+
+  dynamic run(dynamic Function() function) {
+    final ZoneSpecification printInterceptor = ZoneSpecification(
+      print: (Zone self, ZoneDelegate parent, Zone zone, String line) {
+        printLog.add(line);
+      },
+    );
+    return Zone.current.fork(specification: printInterceptor).run<dynamic>(() {
+      return fakeAsync.run((FakeAsync self) {
+        return function();
+      });
+    });
+  }
+}