From 59ef334c8e6a2d82a61a81facabfc5844b11ecab Mon Sep 17 00:00:00 2001
From: Yegor <yjbanov@google.com>
Date: Fri, 7 Jan 2022 13:37:58 -0800
Subject: [PATCH] [web] check that picture is not disposed prior to drawing it
 (flutter/engine#30720)

* check that picture is not disposed prior to drawing it
* use StateError instead of AssertionError
---
 .../lib/src/engine/canvaskit/canvas.dart      |  1 +
 .../lib/src/engine/canvaskit/picture.dart     | 35 +++++++++++++++++--
 .../web_ui/test/canvaskit/picture_test.dart   | 27 +++++++++++---
 3 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/canvas.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/canvas.dart
index 7a9ea502d0..e529ebd5f8 100644
--- a/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/canvas.dart
+++ b/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/canvas.dart
@@ -218,6 +218,7 @@ class CkCanvas {
   }
 
   void drawPicture(CkPicture picture) {
+    assert(picture.debugCheckNotDisposed('Failed to draw picture.'));
     skCanvas.drawPicture(picture.skiaObject);
   }
 
diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/picture.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/picture.dart
index 2591f7f303..25ace9d47d 100644
--- a/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/picture.dart
+++ b/engine/src/flutter/lib/web_ui/lib/src/engine/canvaskit/picture.dart
@@ -43,9 +43,38 @@ class CkPicture extends ManagedSkiaObject<SkPicture> implements ui.Picture {
   /// similar flag [SkiaObjectBox.isDeletedPermanently].
   bool _isDisposed = false;
 
+  /// The stack trace taken when [dispose] was called.
+  ///
+  /// Returns null if [dispose] has not been called. Returns null in non-debug
+  /// modes.
+  StackTrace? _debugDisposalStackTrace;
+
+  /// Throws an [AssertionError] if this picture was disposed.
+  ///
+  /// The [mainErrorMessage] is used as the first line in the error message. It
+  /// is expected to end with a period, e.g. "Failed to draw picture." The full
+  /// message will also explain that the error is due to the fact that the
+  /// picture was disposed and include the stack trace taken when the picture
+  /// was disposed.
+  bool debugCheckNotDisposed(String mainErrorMessage) {
+    if (_isDisposed) {
+      throw StateError(
+        '$mainErrorMessage\n'
+        'The picture has been disposed. When the picture was disposed the '
+        'stack trace was:\n'
+        '$_debugDisposalStackTrace',
+      );
+    }
+    return true;
+  }
+
   @override
   void dispose() {
-    assert(!_isDisposed, 'Object has been disposed.');
+    assert(debugCheckNotDisposed('Cannot dispose picture.'));
+    assert(() {
+      _debugDisposalStackTrace = StackTrace.current;
+      return true;
+    }());
     if (Instrumentation.enabled) {
       Instrumentation.instance.incrementCounter('Picture disposed');
     }
@@ -59,7 +88,7 @@ class CkPicture extends ManagedSkiaObject<SkPicture> implements ui.Picture {
 
   @override
   Future<ui.Image> toImage(int width, int height) async {
-    assert(!_isDisposed);
+    assert(debugCheckNotDisposed('Cannot convert picture to image.'));
     final SkSurface skSurface = canvasKit.MakeSurface(width, height);
     final SkCanvas skCanvas = skSurface.getCanvas();
     skCanvas.drawPicture(skiaObject);
@@ -82,7 +111,7 @@ class CkPicture extends ManagedSkiaObject<SkPicture> implements ui.Picture {
     // If a picture has been explicitly disposed of, it can no longer be
     // resurrected. An attempt to resurrect after the framework told the
     // engine to dispose of the picture likely indicates a bug in the engine.
-    assert(!_isDisposed);
+    assert(debugCheckNotDisposed('Cannot resurrect picture.'));
     return _snapshot!.toPicture();
   }
 
diff --git a/engine/src/flutter/lib/web_ui/test/canvaskit/picture_test.dart b/engine/src/flutter/lib/web_ui/test/canvaskit/picture_test.dart
index 250618a796..cb75efa4ce 100644
--- a/engine/src/flutter/lib/web_ui/test/canvaskit/picture_test.dart
+++ b/engine/src/flutter/lib/web_ui/test/canvaskit/picture_test.dart
@@ -8,7 +8,6 @@ import 'package:test/test.dart';
 import 'package:ui/src/engine.dart';
 import 'package:ui/ui.dart' as ui;
 
-import '../matchers.dart';
 import 'common.dart';
 
 void main() {
@@ -29,19 +28,39 @@ void testMain() {
         final CkPicture picture = recorder.endRecording() as CkPicture;
         expect(picture.rawSkiaObject, isNotNull);
         expect(picture.debugIsDisposed, isFalse);
+        picture.debugCheckNotDisposed('Test.'); // must not throw
         picture.dispose();
         expect(picture.rawSkiaObject, isNull);
         expect(picture.debugIsDisposed, isTrue);
 
+        StateError? actualError;
+        try {
+          picture.debugCheckNotDisposed('Test.');
+        } on StateError catch (error) {
+          actualError = error;
+        }
+
+        expect(actualError, isNotNull);
+
+        // TODO(yjbanov): cannot test precise message due to https://github.com/flutter/flutter/issues/96298
+        expect('$actualError', allOf(
+          startsWith(
+            'Bad state: Test.\n'
+            'The picture has been disposed. '
+            'When the picture was disposed the stack trace was:\n'
+          ),
+          contains('StackTrace_current'),
+        ));
+
         // Emulate SkiaObjectCache deleting the picture
         picture.delete();
         picture.didDelete();
         expect(picture.rawSkiaObject, isNull);
 
         // A Picture that's been disposed of can no longer be resurrected
-        expect(() => picture.resurrect(), throwsAssertionError);
-        expect(() => picture.toImage(10, 10), throwsAssertionError);
-        expect(() => picture.dispose(), throwsAssertionError);
+        expect(() => picture.resurrect(), throwsStateError);
+        expect(() => picture.toImage(10, 10), throwsStateError);
+        expect(() => picture.dispose(), throwsStateError);
       });
 
       test('can be deleted by SkiaObjectCache', () {