disallow double dispose; better assertion messages

Author: yjbanov

lib/web_ui/lib/src/engine/canvaskit/image.dart

@@ -70,10 +70,10 @@ class CkAnimatedImage implements ui.Codec, StackTraceDebugger {
 
   @override
   void dispose() {
-    if (_disposed) {
-      // This method is idempotent.
-      return;
-    }
+    assert(
+      !_disposed,
+      'Cannot dispose a codec that has already been disposed.',
+    );
     _disposed = true;
 
     // This image is no longer usable. Bump the ref count.
@@ -141,9 +141,10 @@ class CkImage implements ui.Image, StackTraceDebugger {
 
   @override
   void dispose() {
-    if (_disposed) {
-      return;
-    }
+    assert(
+      !_disposed,
+      'Cannot dispose an image that has already been disposed.',
+    );
     _disposed = true;
     box.unref(this);
   }

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

@@ -273,9 +273,9 @@ abstract class StackTraceDebugger {
 /// The [delete] method may be called any number of times. The box
 /// will only delete the object once.
 class SkiaObjectBox<R extends StackTraceDebugger, T> {
-  SkiaObjectBox(R debugWrapper, this.skiaObject) : _skDeletable = skiaObject as SkDeletable {
+  SkiaObjectBox(R debugReferrer, this.skiaObject) : _skDeletable = skiaObject as SkDeletable {
     if (assertionsEnabled) {
-      debugReferrers.add(debugWrapper);
+      debugReferrers.add(debugReferrer);
     }
     if (browserSupportsFinalizationRegistry) {
       boxRegistry.register(this, _skDeletable);
@@ -295,7 +295,7 @@ class SkiaObjectBox<R extends StackTraceDebugger, T> {
   /// The length of this list is always identical to [refCount].
   ///
   /// This list can be used for debugging ref counting issues.
-  final List<R> debugReferrers = <R>[];
+  final Set<R> debugReferrers = <R>{};
 
   /// If asserts are enabled, the [StackTrace]s representing when a reference
   /// was created.
@@ -329,13 +329,14 @@ class SkiaObjectBox<R extends StackTraceDebugger, T> {
   /// sharing ownership of the underlying [skiaObject].
   ///
   /// Clones must be [dispose]d when finished.
-  void ref(R debugWrapper) {
+  void ref(R debugReferrer) {
     assert(!_isDeleted, 'Cannot increment ref count on a deleted handle.');
     assert(_refCount > 0);
+    assert(
+      debugReferrers.add(debugReferrer),
+      'Attempted to increment ref count by the same referrer more than once.',
+    );
     _refCount += 1;
-    if (assertionsEnabled) {
-      debugReferrers.add(debugWrapper);
-    }
     assert(refCount == debugReferrers.length);
   }
 
@@ -345,10 +346,13 @@ class SkiaObjectBox<R extends StackTraceDebugger, T> {
   ///
   /// If this causes the reference count to drop to zero, deletes the
   /// [skObject].
-  void unref(R debugWrapper) {
+  void unref(R debugReferrer) {
     assert(!_isDeleted, 'Attempted to unref an already deleted Skia object.');
+    assert(
+      debugReferrers.remove(debugReferrer),
+      'Attempted to decrement ref count by the same referrer more than once.',
+    );
     _refCount -= 1;
-    assert(debugReferrers.remove(debugWrapper));
     assert(refCount == debugReferrers.length);
     if (_refCount == 0) {
       _isDeleted = true;

lib/web_ui/test/canvaskit/image_test.dart

@@ -10,6 +10,7 @@ import 'package:test/test.dart';
 import 'package:ui/src/engine.dart';
 import 'package:ui/ui.dart' as ui;
 
+import '../matchers.dart';
 import 'common.dart';
 import 'test_data.dart';
 
@@ -30,9 +31,9 @@ void testMain() {
       image.dispose();
       expect(image.box.isDeleted, true);
       expect(image.debugDisposed, true);
-      image.dispose();
-      expect(image.box.isDeleted, true);
-      expect(image.debugDisposed, true);
+
+      // Disallow double-dispose.
+      expect(() => image.dispose(), throwsAssertionError);
     });
 
     test('CkAnimatedImage can be cloned and explicitly disposed of', () async {
@@ -68,9 +69,9 @@ void testMain() {
       image.dispose();
       expect(image.debugDisposed, true);
       expect(image.box.isDeleted, true);
-      image.dispose();
-      expect(image.debugDisposed, true);
-      expect(image.box.isDeleted, true);
+
+      // Disallow double-dispose.
+      expect(() => image.dispose(), throwsAssertionError);
     });
 
     test('CkImage can be explicitly disposed of when cloned', () async {