From 37fa2850d10e55bc011276568ffef02b98290917 Mon Sep 17 00:00:00 2001 From: Ferhat Date: Wed, 22 Jul 2020 12:55:22 -0700 Subject: [PATCH] [web] Prevent crash when restore is called incorrectly after recording ends. (#19948) * Prevent crash when restore is called after picture recording ends * Addressed reviewer comments --- lib/web_ui/lib/src/engine/bitmap_canvas.dart | 1 - .../src/engine/surface/recording_canvas.dart | 76 ++++++++++--------- .../test/engine/recording_canvas_test.dart | 8 ++ 3 files changed, 48 insertions(+), 37 deletions(-) diff --git a/lib/web_ui/lib/src/engine/bitmap_canvas.dart b/lib/web_ui/lib/src/engine/bitmap_canvas.dart index 39005e398..d0d871c8f 100644 --- a/lib/web_ui/lib/src/engine/bitmap_canvas.dart +++ b/lib/web_ui/lib/src/engine/bitmap_canvas.dart @@ -814,7 +814,6 @@ class BitmapCanvas extends EngineCanvas { @override void endOfPaint() { - assert(_saveCount == 0); _canvasPool.endOfPaint(); _elementCache?.commitFrame(); } diff --git a/lib/web_ui/lib/src/engine/surface/recording_canvas.dart b/lib/web_ui/lib/src/engine/surface/recording_canvas.dart index 853fcd180..22c9e4131 100644 --- a/lib/web_ui/lib/src/engine/surface/recording_canvas.dart +++ b/lib/web_ui/lib/src/engine/surface/recording_canvas.dart @@ -58,7 +58,7 @@ class RecordingCanvas { /// method for more details. ui.Rect? get pictureBounds { assert( - _debugRecordingEnded, + _recordingEnded, 'Picture bounds not available yet. Call [endRecording] before accessing picture bounds.', ); return _pictureBounds; @@ -101,9 +101,14 @@ class RecordingCanvas { bool get didDraw => _didDraw; bool _didDraw = false; - /// When assertions are enabled used to ensure that [endRecording] is called - /// before calling [apply] or [pictureBounds]. - bool _debugRecordingEnded = false; + /// Used to ensure that [endRecording] is called before calling [apply] or + /// [pictureBounds]. + /// + /// When [PaintingContext] is used by [ClipContext], the painter may + /// end a recording and start a new one and cause [ClipContext] to call + /// restore on a new canvas before prior save calls, [_recordingEnded] + /// prevents transforms removals in that case. + bool _recordingEnded = false; /// Stops recording drawing commands and computes paint bounds. /// @@ -114,9 +119,7 @@ class RecordingCanvas { /// directly it is up to you to call this method explicitly. void endRecording() { _pictureBounds = _paintBounds.computeBounds(); - if (assertionsEnabled) { - _debugRecordingEnded = true; - } + _recordingEnded = true; } /// Applies the recorded commands onto an [engineCanvas]. @@ -126,7 +129,7 @@ class RecordingCanvas { /// not applied to the [engineCanvas]. A command must have a non-zero /// intersection with the clip in order to be applied. void apply(EngineCanvas? engineCanvas, ui.Rect? clipRect) { - assert(_debugRecordingEnded); + assert(_recordingEnded); if (_debugDumpPaintCommands) { final StringBuffer debugBuf = StringBuffer(); int skips = 0; @@ -199,14 +202,14 @@ class RecordingCanvas { } void save() { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _paintBounds.saveTransformsAndClip(); _commands.add(const PaintSave()); _saveCount++; } void saveLayerWithoutBounds(SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _hasArbitraryPaint = true; // TODO(het): Implement this correctly using another canvas. _commands.add(const PaintSave()); @@ -215,7 +218,7 @@ class RecordingCanvas { } void saveLayer(ui.Rect bounds, SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _hasArbitraryPaint = true; // TODO(het): Implement this correctly using another canvas. _commands.add(const PaintSave()); @@ -224,8 +227,9 @@ class RecordingCanvas { } void restore() { - assert(!_debugRecordingEnded); - _paintBounds.restoreTransformsAndClip(); + if (!_recordingEnded) { + _paintBounds.restoreTransformsAndClip(); + } if (_commands.isNotEmpty && _commands.last is PaintSave) { // A restore followed a save without any drawing operations in between. // This means that the save didn't have any effect on drawing operations @@ -239,38 +243,38 @@ class RecordingCanvas { } void translate(double dx, double dy) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _paintBounds.translate(dx, dy); _commands.add(PaintTranslate(dx, dy)); } void scale(double sx, double sy) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _paintBounds.scale(sx, sy); _commands.add(PaintScale(sx, sy)); } void rotate(double radians) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _paintBounds.rotateZ(radians); _commands.add(PaintRotate(radians)); } void transform(Float32List matrix4) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _paintBounds.transform(matrix4); _commands.add(PaintTransform(matrix4)); } void skew(double sx, double sy) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _hasArbitraryPaint = true; _paintBounds.skew(sx, sy); _commands.add(PaintSkew(sx, sy)); } void clipRect(ui.Rect rect) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); final PaintClipRect command = PaintClipRect(rect); _paintBounds.clipRect(rect, command); _hasArbitraryPaint = true; @@ -278,7 +282,7 @@ class RecordingCanvas { } void clipRRect(ui.RRect rrect) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); final PaintClipRRect command = PaintClipRRect(rrect); _paintBounds.clipRect(rrect.outerRect, command); _hasArbitraryPaint = true; @@ -286,7 +290,7 @@ class RecordingCanvas { } void clipPath(ui.Path path, {bool doAntiAlias = true}) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); final PaintClipPath command = PaintClipPath(path as SurfacePath); _paintBounds.clipRect(path.getBounds(), command); _hasArbitraryPaint = true; @@ -294,14 +298,14 @@ class RecordingCanvas { } void drawColor(ui.Color color, ui.BlendMode blendMode) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); final PaintDrawColor command = PaintDrawColor(color, blendMode); _commands.add(command); _paintBounds.grow(_paintBounds.maxPaintBounds, command); } void drawLine(ui.Offset p1, ui.Offset p2, SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); final double paintSpread = math.max(_getPaintSpread(paint), 1.0); final PaintDrawLine command = PaintDrawLine(p1, p2, paint.paintData); // TODO(yjbanov): This can be optimized. Currently we create a box around @@ -324,7 +328,7 @@ class RecordingCanvas { } void drawPaint(SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _hasArbitraryPaint = true; _didDraw = true; final PaintDrawPaint command = PaintDrawPaint(paint.paintData); @@ -333,7 +337,7 @@ class RecordingCanvas { } void drawRect(ui.Rect rect, SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); if (paint.shader != null) { _hasArbitraryPaint = true; } @@ -349,7 +353,7 @@ class RecordingCanvas { } void drawRRect(ui.RRect rrect, SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); if (paint.shader != null || !rrect.webOnlyUniformRadii) { _hasArbitraryPaint = true; } @@ -365,7 +369,7 @@ class RecordingCanvas { } void drawDRRect(ui.RRect outer, ui.RRect inner, SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); // Check the inner bounds are contained within the outer bounds // see: https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkCanvas.cpp?l=1787-1789 ui.Rect innerRect = inner.outerRect; @@ -418,7 +422,7 @@ class RecordingCanvas { } void drawOval(ui.Rect rect, SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _hasArbitraryPaint = true; _didDraw = true; final double paintSpread = _getPaintSpread(paint); @@ -432,7 +436,7 @@ class RecordingCanvas { } void drawCircle(ui.Offset c, double radius, SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _hasArbitraryPaint = true; _didDraw = true; final double paintSpread = _getPaintSpread(paint); @@ -449,7 +453,7 @@ class RecordingCanvas { } void drawPath(ui.Path path, SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); if (paint.shader == null) { // For Rect/RoundedRect paths use drawRect/drawRRect code paths for // DomCanvas optimization. @@ -484,7 +488,7 @@ class RecordingCanvas { } void drawImage(ui.Image image, ui.Offset offset, SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _hasArbitraryPaint = true; _didDraw = true; final double left = offset.dx; @@ -496,7 +500,7 @@ class RecordingCanvas { void drawImageRect( ui.Image image, ui.Rect src, ui.Rect dst, SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _hasArbitraryPaint = true; _didDraw = true; final PaintDrawImageRect command = PaintDrawImageRect(image, src, dst, paint.paintData); @@ -505,7 +509,7 @@ class RecordingCanvas { } void drawParagraph(ui.Paragraph paragraph, ui.Offset offset) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); final EngineParagraph engineParagraph = paragraph as EngineParagraph; if (!engineParagraph._isLaidOut) { // Ignore non-laid out paragraphs. This matches Flutter's behavior. @@ -531,7 +535,7 @@ class RecordingCanvas { void drawShadow(ui.Path path, ui.Color color, double elevation, bool transparentOccluder) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _hasArbitraryPaint = true; _didDraw = true; final ui.Rect shadowRect = @@ -543,7 +547,7 @@ class RecordingCanvas { void drawVertices( SurfaceVertices vertices, ui.BlendMode blendMode, SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _hasArbitraryPaint = true; _didDraw = true; final PaintDrawVertices command = PaintDrawVertices(vertices, blendMode, paint.paintData); @@ -553,7 +557,7 @@ class RecordingCanvas { void drawRawPoints( ui.PointMode pointMode, Float32List points, SurfacePaint paint) { - assert(!_debugRecordingEnded); + assert(!_recordingEnded); _hasArbitraryPaint = true; _didDraw = true; final PaintDrawPoints command = PaintDrawPoints(pointMode, points, paint.paintData); diff --git a/lib/web_ui/test/engine/recording_canvas_test.dart b/lib/web_ui/test/engine/recording_canvas_test.dart index 3e5d4a884..d997f811b 100644 --- a/lib/web_ui/test/engine/recording_canvas_test.dart +++ b/lib/web_ui/test/engine/recording_canvas_test.dart @@ -187,6 +187,14 @@ void main() { expect(mockCanvas.methodCallLog[2].methodName, 'restore'); expect(mockCanvas.methodCallLog[3].methodName, 'endOfPaint'); }); + + // Regression test for https://github.com/flutter/flutter/issues/61697. + test('Allows restore calls after recording has ended', () { + final RecordingCanvas rc = RecordingCanvas(Rect.fromLTRB(0, 0, 200, 400)); + rc.endRecording(); + // Should not throw exception on restore. + expect(() => rc.restore(), returnsNormally); + }); } // Expect a drawDRRect call to be registered in the mock call log, with the expectedArguments -- GitLab