Fix CanvasKit SVG clipPath leak (#26227)

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

@@ -200,7 +200,7 @@ class HtmlViewEmbedder {
       );
       _rootViews[viewId] = newPlatformViewRoot;
     }
-    _applyMutators(params.mutators, platformView);
+    _applyMutators(params.mutators, platformView, viewId);
   }
 
   int _countClips(MutatorsStack mutators) {
@@ -246,11 +246,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<html.Element> nodesToRemove = <html.Element>[];
+      final Set<String> 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) {
@@ -278,28 +300,38 @@ class HtmlViewEmbedder {
             html.Element pathDefs =
                 _svgPathDefs!.querySelector('#sk_path_defs')!;
             _clipPathCount += 1;
+            final String clipId = 'svgClip$_clipPathCount';
             html.Node newClipPath = html.DocumentFragment.svg(
-              '<clipPath id="svgClip$_clipPathCount">'
+              '<clipPath id="$clipId">'
               '<path d="${path.toSvgString()}">'
               '</path></clipPath>',
               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, () => <String>{}).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(
-              '<clipPath id="svgClip$_clipPathCount">'
+              '<clipPath id="$clipId">'
               '<path d="${path.toSvgString()}">'
               '</path></clipPath>',
               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, () => <String>{}).add(clipId);
+            clipView.style.clipPath = 'url(#$clipId)';
           }
           _resetAnchor(clipView);
           head = clipView;
@@ -337,6 +369,9 @@ class HtmlViewEmbedder {
 
   html.Element? _svgPathDefs;
 
+  /// The nodes containing the SVG clip definitions needed to clip this view.
+  Map<int, Set<String>> _svgClipDefs = <int, Set<String>>{};
+
   /// 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() {
@@ -424,6 +459,8 @@ class HtmlViewEmbedder {
       _currentCompositionParams.remove(viewId);
       _clipCount.remove(viewId);
       _viewsToRecomposite.remove(viewId);
+      _cleanUpClipDefs(viewId);
+      _svgClipDefs.remove(viewId);
     }
     _viewsToDispose.clear();
   }
@@ -452,6 +489,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.

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 {

lib/web_ui/test/canvaskit/embedded_views_test.dart

@@ -27,6 +27,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',
@@ -122,7 +127,7 @@ void testMain() {
     List<String> getTransformChain(html.Element viewHost) {
       final List<String> chain = <String>[];
       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;
       }
@@ -145,9 +150,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),
@@ -173,9 +177,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),
@@ -189,12 +192,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<int> platformViewIds = <int>[];
@@ -210,7 +211,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++) {
@@ -360,6 +361,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<void>.delayed(Duration.zero);
+      renderTestScene();
+      expect(skPathDefs.childNodes, hasLength(1));
+
+      await Future<void>.delayed(Duration.zero);
+      renderTestScene();
+      expect(skPathDefs.childNodes, hasLength(1));
+    });
     // TODO: https://github.com/flutter/flutter/issues/60040
   }, skip: isIosSafari);
 }