Skip to content

Commit

Permalink
Fix CanvasKit SVG clipPath leak (flutter#26227)
Browse files Browse the repository at this point in the history
  • Loading branch information
@christopherfujino
Harry Terkelsen authored and christopherfujino committed May 25, 2021
1 parent a9b004a commit 566f7cd
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 37 deletions.
57 changes: 51 additions & 6 deletions lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ class HtmlViewEmbedder {
);
_rootViews[viewId] = newPlatformViewRoot;
}
_applyMutators(params.mutators, platformView);
_applyMutators(params.mutators, platformView, viewId);
}

int _countClips(MutatorsStack mutators) {
Expand Down Expand Up @@ -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<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) {
Expand Down Expand Up @@ -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(
'<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;
Expand Down Expand Up @@ -323,6 +355,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() {
Expand Down Expand Up @@ -411,6 +446,8 @@ class HtmlViewEmbedder {
_currentCompositionParams.remove(viewId);
_clipCount.remove(viewId);
_viewsToRecomposite.remove(viewId);
_cleanUpClipDefs(viewId);
_svgClipDefs.remove(viewId);
}
_viewsToDispose.clear();
}
Expand Down Expand Up @@ -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.
Expand Down
36 changes: 19 additions & 17 deletions lib/web_ui/test/canvaskit/common.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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());
});
Expand Down Expand Up @@ -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>[];
Expand Down Expand Up @@ -113,32 +115,32 @@ 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;
}
}
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'
'${collection.stackTrace}\n\n'
'The first collection was requested here:\n'
'${firstCollection.stackTrace}\n\n'
'The second collection was requested here:\n'
'${collection.stackTrace}'
'${collection.stackTrace}',
);
}
} else {
Expand Down
67 changes: 53 additions & 14 deletions lib/web_ui/test/canvaskit/embedded_views_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -123,7 +128,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;
}
Expand All @@ -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),
Expand All @@ -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),
Expand All @@ -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<int> platformViewIds = <int>[];
Expand All @@ -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++) {
Expand Down Expand Up @@ -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<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);
}
Expand Down

0 comments on commit 566f7cd

Please sign in to comment.