Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix CanvasKit SVG clipPath leak #26227

Merged
merged 5 commits into from
May 20, 2021
Merged

Fix CanvasKit SVG clipPath leak #26227

merged 5 commits into from
May 20, 2021

Conversation

harryterkelsen
Copy link
Contributor

Removes old SVG clip path definitions when a platform view is recomposited. This is to avoid a leak caused by infinitely adding SVG clip definitions to the DOM.

Fixes flutter/flutter#82768

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.
  • The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label May 18, 2021
@google-cla google-cla bot added the cla: yes label May 18, 2021
ditman
ditman reviewed May 18, 2021
View reviewed changes
lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart Outdated
final html.Element clipDefs =
_svgPathDefs!.querySelector('#sk_path_defs')!;
for (String id in _svgClipDefs[viewId]!) {
clipDefs.querySelector('#$id')?.remove();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep a reference to the actual DOM Node of each clipDef (newClipPath?), so we don't have to do any DOM lookups here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how I tried to do it at first (keeping a Map<int, List<html.Node>>... but for whatever reason, when I remove the html.Node created by html.DocumentFragment.svg, it doesn't remove the <clipPath> element from the DOM...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment explaining why we can't just store the node directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

querySelector used to be extremely expensive. I think the issue is that DocumentFragment is a transient concept. When attaching it to the DOM it simply iterates over its children and attaches them one by one. The fragment itself is not attached. Would it be possible to wrap all clip elements under a common parent and then attach the parent? This way we can keep the reference to the parent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The common parent exists, it seems to be the clipDef tag. Maybe we can just inject it into the page just in case it's needed (right now, it's being probed every time there's a mutator of type clip, it seems)

yjbanov
yjbanov approved these changes May 18, 2021
View reviewed changes
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but it would be good to avoid querySelector, if possible.

lgtm

lib/web_ui/test/canvaskit/common.dart Outdated
'The first collection was requested here:\n'
'${firstCollection.stackTrace}\n\n'
'The second collection was requested here:\n'
'${collection.stackTrace}');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think dartfmt might do a better job if there was a trailing comma after the message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

ditman
ditman approved these changes May 18, 2021
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Small comment about not doing "many" querySelector calls (which is probably a micro-optimization and not worth the effort of the change)

lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart Outdated Show resolved Hide resolved
@ditman
Copy link
Member

ditman commented May 19, 2021

Keeps LGTM!

@harryterkelsen
Copy link
Contributor Author

Landing even though tests are "In Progress". According to LUCI, they finished successfully.

@harryterkelsen harryterkelsen merged commit 07fa0dc into flutter:master May 20, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 20, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 20, 2021
@engine-flutter-autoroll
07fa0dc Fix CanvasKit SVG clipPath leak (flutter/engine#26227)
d65d7e3
christopherfujino pushed a commit to christopherfujino/engine that referenced this pull request May 25, 2021
@christopherfujino
Fix CanvasKit SVG clipPath leak (flutter#26227)
566f7cd
@christopherfujino christopherfujino mentioned this pull request May 25, 2021
Merged
christopherfujino added a commit that referenced this pull request May 26, 2021
@christopherfujino @yjbanov
[flutter_releases] Flutter Stable 2.2.1 Engine Cherrypicks (#26405)
0fdb562 
* 'Update Dart SDK to 375a2d7'

* Fix a11y tab traversal (#25797)

* fix race with framework when using tab to traverse in a11y mode

* Fix CanvasKit SVG clipPath leak (#26227)

Co-authored-by: Yegor <yjbanov@google.com>
Co-authored-by: Harry Terkelsen <hterkelsen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ditman ditman ditman approved these changes

@yjbanov yjbanov yjbanov approved these changes

Assignees
No one assigned
Labels
cla: yes cp: 2.2 platform-web Code specifically for the web engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[web] svgClip "leak" in canvaskit renderer
4 participants
@harryterkelsen @ditman @yjbanov @ferhatb