From 566f7cd8ae79766fab6972a6c0a7923331e63e29 Mon Sep 17 00:00:00 2001 From: Harry Terkelsen Date: Wed, 19 May 2021 17:50:53 -0700 Subject: [PATCH] Fix CanvasKit SVG clipPath leak (#26227) --- .../src/engine/canvaskit/embedded_views.dart | 57 ++++++++++++++-- lib/web_ui/test/canvaskit/common.dart | 36 +++++----- .../test/canvaskit/embedded_views_test.dart | 67 +++++++++++++++---- 3 files changed, 123 insertions(+), 37 deletions(-) diff --git a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart index aa9055f9318ac..9c3e978682e1d 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart @@ -187,7 +187,7 @@ class HtmlViewEmbedder { ); _rootViews[viewId] = newPlatformViewRoot; } - _applyMutators(params.mutators, platformView); + _applyMutators(params.mutators, platformView, viewId); } int _countClips(MutatorsStack mutators) { @@ -233,11 +233,33 @@ class HtmlViewEmbedder { return head; } - void _applyMutators(MutatorsStack mutators, html.Element embeddedView) { + /// Clean up the old SVG clip definitions, as this platform view is about to + /// be recomposited. + void _cleanUpClipDefs(int viewId) { + if (_svgClipDefs.containsKey(viewId)) { + final html.Element clipDefs = + _svgPathDefs!.querySelector('#sk_path_defs')!; + final List nodesToRemove = []; + final Set oldDefs = _svgClipDefs[viewId]!; + for (html.Element child in clipDefs.children) { + if (oldDefs.contains(child.id)) { + nodesToRemove.add(child); + } + } + for (html.Element node in nodesToRemove) { + node.remove(); + } + _svgClipDefs[viewId]!.clear(); + } + } + + void _applyMutators( + MutatorsStack mutators, html.Element embeddedView, int viewId) { html.Element head = embeddedView; Matrix4 headTransform = Matrix4.identity(); double embeddedOpacity = 1.0; _resetAnchor(head); + _cleanUpClipDefs(viewId); for (final Mutator mutator in mutators) { switch (mutator.type) { @@ -265,28 +287,38 @@ class HtmlViewEmbedder { html.Element pathDefs = _svgPathDefs!.querySelector('#sk_path_defs')!; _clipPathCount += 1; + final String clipId = 'svgClip$_clipPathCount'; html.Node newClipPath = html.DocumentFragment.svg( - '' + '' '' '', treeSanitizer: _NullTreeSanitizer(), ); pathDefs.append(newClipPath); - clipView.style.clipPath = 'url(#svgClip$_clipPathCount)'; + // Store the id of the node instead of [newClipPath] directly. For + // some reason, calling `newClipPath.remove()` doesn't remove it + // from the DOM. + _svgClipDefs.putIfAbsent(viewId, () => {}).add(clipId); + clipView.style.clipPath = 'url(#$clipId)'; } else if (mutator.path != null) { final CkPath path = mutator.path as CkPath; _ensureSvgPathDefs(); html.Element pathDefs = _svgPathDefs!.querySelector('#sk_path_defs')!; _clipPathCount += 1; + final String clipId = 'svgClip$_clipPathCount'; html.Node newClipPath = html.DocumentFragment.svg( - '' + '' '' '', treeSanitizer: _NullTreeSanitizer(), ); pathDefs.append(newClipPath); - clipView.style.clipPath = 'url(#svgClip$_clipPathCount)'; + // Store the id of the node instead of [newClipPath] directly. For + // some reason, calling `newClipPath.remove()` doesn't remove it + // from the DOM. + _svgClipDefs.putIfAbsent(viewId, () => {}).add(clipId); + clipView.style.clipPath = 'url(#$clipId)'; } _resetAnchor(clipView); head = clipView; @@ -323,6 +355,9 @@ class HtmlViewEmbedder { html.Element? _svgPathDefs; + /// The nodes containing the SVG clip definitions needed to clip this view. + Map> _svgClipDefs = >{}; + /// Ensures we add a container of SVG path defs to the DOM so they can /// be referred to in clip-path: url(#blah). void _ensureSvgPathDefs() { @@ -411,6 +446,8 @@ class HtmlViewEmbedder { _currentCompositionParams.remove(viewId); _clipCount.remove(viewId); _viewsToRecomposite.remove(viewId); + _cleanUpClipDefs(viewId); + _svgClipDefs.remove(viewId); } _viewsToDispose.clear(); } @@ -439,6 +476,14 @@ class HtmlViewEmbedder { _overlays[viewId] = overlay; } + + /// Deletes SVG clip paths, useful for tests. + void debugCleanupSvgClipPaths() { + _svgPathDefs?.children.single.children.forEach((element) { + element.remove(); + }); + _svgClipDefs.clear(); + } } /// Caches surfaces used to overlay platform views. diff --git a/lib/web_ui/test/canvaskit/common.dart b/lib/web_ui/test/canvaskit/common.dart index 45b3bbd66c8a9..3bb74c9e915e1 100644 --- a/lib/web_ui/test/canvaskit/common.dart +++ b/lib/web_ui/test/canvaskit/common.dart @@ -9,8 +9,9 @@ import 'package:ui/ui.dart' as ui; /// Whether the current browser is Safari on iOS. // TODO: https://github.com/flutter/flutter/issues/60040 -bool get isIosSafari => browserEngine == BrowserEngine.webkit && - operatingSystem == OperatingSystem.iOs; +bool get isIosSafari => + browserEngine == BrowserEngine.webkit && + operatingSystem == OperatingSystem.iOs; /// Whether the current browser is Firefox. bool get isFirefox => browserEngine == BrowserEngine.firefox; @@ -24,8 +25,7 @@ late TestCollector testCollector; /// Common test setup for all CanvasKit unit-tests. void setUpCanvasKitTest() { setUpAll(() async { - expect(useCanvasKit, true, - reason: 'This test must run in CanvasKit mode.'); + expect(useCanvasKit, true, reason: 'This test must run in CanvasKit mode.'); debugResetBrowserSupportsFinalizationRegistry(); await ui.webOnlyInitializePlatform(assetManager: WebOnlyMockAssetManager()); }); @@ -81,8 +81,10 @@ class _TestCollection { /// /// Tests should use [collectNow] and [collectAfterTest] to trigger collections. class TestCollector implements Collector { - final List<_TestFinalizerRegistration> _activeRegistrations = <_TestFinalizerRegistration>[]; - final List<_TestFinalizerRegistration> _collectedRegistrations = <_TestFinalizerRegistration>[]; + final List<_TestFinalizerRegistration> _activeRegistrations = + <_TestFinalizerRegistration>[]; + final List<_TestFinalizerRegistration> _collectedRegistrations = + <_TestFinalizerRegistration>[]; final List<_TestCollection> _pendingCollections = <_TestCollection>[]; final List<_TestCollection> _completedCollections = <_TestCollection>[]; @@ -113,7 +115,8 @@ class TestCollector implements Collector { } if (activeRegistration == null) { late final _TestFinalizerRegistration? collectedRegistration; - for (_TestFinalizerRegistration registration in _collectedRegistrations) { + for (_TestFinalizerRegistration registration + in _collectedRegistrations) { if (identical(registration.deletable, collection.deletable)) { collectedRegistration = registration; break; @@ -121,16 +124,15 @@ class TestCollector implements Collector { } if (collectedRegistration == null) { fail( - 'Attempted to collect an object that was never registered for finalization.\n' - 'The collection was requested here:\n' - '${collection.stackTrace}' - ); + 'Attempted to collect an object that was never registered for finalization.\n' + 'The collection was requested here:\n' + '${collection.stackTrace}'); } else { - final _TestCollection firstCollection = _completedCollections.firstWhere( - (_TestCollection completedCollection) { - return identical(completedCollection.deletable, collection.deletable); - } - ); + final _TestCollection firstCollection = _completedCollections + .firstWhere((_TestCollection completedCollection) { + return identical( + completedCollection.deletable, collection.deletable); + }); fail( 'Attempted to collect an object that was previously collected.\n' 'The object was registered for finalization here:\n' @@ -138,7 +140,7 @@ class TestCollector implements Collector { 'The first collection was requested here:\n' '${firstCollection.stackTrace}\n\n' 'The second collection was requested here:\n' - '${collection.stackTrace}' + '${collection.stackTrace}', ); } } else { diff --git a/lib/web_ui/test/canvaskit/embedded_views_test.dart b/lib/web_ui/test/canvaskit/embedded_views_test.dart index 4788d61c5cead..970d59577afb1 100644 --- a/lib/web_ui/test/canvaskit/embedded_views_test.dart +++ b/lib/web_ui/test/canvaskit/embedded_views_test.dart @@ -28,6 +28,11 @@ void testMain() { window.debugOverrideDevicePixelRatio(1); }); + tearDown(() { + EnginePlatformDispatcher.instance.rasterizer?.surface.viewEmbedder + .debugCleanupSvgClipPaths(); + }); + test('embeds interactive platform views', () async { ui.platformViewRegistry.registerViewFactory( 'test-platform-view', @@ -123,7 +128,7 @@ void testMain() { List getTransformChain(html.Element viewHost) { final List chain = []; html.Element? element = viewHost; - while(element != null && element.tagName.toLowerCase() != 'flt-scene') { + while (element != null && element.tagName.toLowerCase() != 'flt-scene') { chain.add(element.style.transform); element = element.parent; } @@ -146,9 +151,8 @@ void testMain() { sb.pushOffset(3, 3); sb.addPlatformView(0, width: 10, height: 10); dispatcher.rasterizer!.draw(sb.build().layerTree); - final html.Element viewHost = domRenderer.sceneElement! - .querySelectorAll('#view-0') - .single; + final html.Element viewHost = + domRenderer.sceneElement!.querySelectorAll('#view-0').single; expect( getTransformChain(viewHost), @@ -174,9 +178,8 @@ void testMain() { sb.pushOffset(9, 9); sb.addPlatformView(0, width: 10, height: 10); dispatcher.rasterizer!.draw(sb.build().layerTree); - final html.Element viewHost = domRenderer.sceneElement! - .querySelectorAll('#view-0') - .single; + final html.Element viewHost = + domRenderer.sceneElement!.querySelectorAll('#view-0').single; expect( getTransformChain(viewHost), @@ -190,12 +193,10 @@ void testMain() { test('renders overlays on top of platform views', () async { expect(OverlayCache.instance.debugLength, 0); - final CkPicture testPicture = paintPicture( - ui.Rect.fromLTRB(0, 0, 10, 10), - (CkCanvas canvas) { - canvas.drawCircle(ui.Offset(5, 5), 5, CkPaint()); - } - ); + final CkPicture testPicture = + paintPicture(ui.Rect.fromLTRB(0, 0, 10, 10), (CkCanvas canvas) { + canvas.drawCircle(ui.Offset(5, 5), 5, CkPaint()); + }); // Initialize all platform views to be used in the test. final List platformViewIds = []; @@ -211,7 +212,7 @@ void testMain() { final EnginePlatformDispatcher dispatcher = ui.window.platformDispatcher as EnginePlatformDispatcher; - void renderTestScene({ required int viewCount }) { + void renderTestScene({required int viewCount}) { LayerSceneBuilder sb = LayerSceneBuilder(); sb.pushOffset(0, 0); for (int i = 0; i < viewCount; i++) { @@ -361,6 +362,44 @@ void testMain() { hasLength(0), ); }); + + test( + 'removes old SVG clip definitions from the DOM when the view is recomposited', + () async { + ui.platformViewRegistry.registerViewFactory( + 'test-platform-view', + (viewId) => html.DivElement()..id = 'test-view', + ); + await _createPlatformView(0, 'test-platform-view'); + + final EnginePlatformDispatcher dispatcher = + ui.window.platformDispatcher as EnginePlatformDispatcher; + + void renderTestScene() { + LayerSceneBuilder sb = LayerSceneBuilder(); + sb.pushOffset(0, 0); + sb.pushClipRRect( + ui.RRect.fromLTRBR(0, 0, 10, 10, ui.Radius.circular(3))); + sb.addPlatformView(0, width: 10, height: 10); + dispatcher.rasterizer!.draw(sb.build().layerTree); + } + + final html.Node skPathDefs = + domRenderer.sceneElement!.querySelector('#sk_path_defs')!; + + expect(skPathDefs.childNodes, hasLength(0)); + + renderTestScene(); + expect(skPathDefs.childNodes, hasLength(1)); + + await Future.delayed(Duration.zero); + renderTestScene(); + expect(skPathDefs.childNodes, hasLength(1)); + + await Future.delayed(Duration.zero); + renderTestScene(); + expect(skPathDefs.childNodes, hasLength(1)); + }); // TODO: https://github.com/flutter/flutter/issues/60040 }, skip: isIosSafari); }