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.

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

[macos] FlutterKeyboardManager memory leak fix #48824

Merged
merged 2 commits into from
Dec 20, 2023

Conversation

sergiy-sc
Copy link
Contributor

We are embedding Flutter into MacOS app, and noticed that there is a leak. leaks tool says there is a cycle reference:

12 (2.11K) ROOT CYCLE: <FlutterKeyboardManager 0x29ec55f40> [80]
       8 (432 bytes) __strong _primaryResponders --> ROOT CYCLE: <NSMutableArray 0x29ec560a0> [64]
          7 (368 bytes) ROOT CYCLE: <NSMutableArray (Storage) 0x29ec56140> [32]
             4 (224 bytes) ROOT CYCLE: <FlutterEmbedderKeyResponder 0x29ec56310> [80]
                1 (48 bytes) __strong _sendEvent --> ROOT CYCLE: <__NSMallocBlock__ 0x29ec56360> [48]
                   __strong [capture] --> CYCLE BACK TO <FlutterKeyboardManager 0x29ec55f40> [80]
                1 (48 bytes) __strong _pendingResponses --> <NSMutableDictionary 0x29ec563c0> [48]
                1 (48 bytes) __strong _pressingRecords --> <NSMutableDictionary 0x29ec56390> [48]
             2 (112 bytes) <FlutterChannelKeyResponder 0x29ec56450> [48]
                1 (64 bytes) __strong _channel --> <FlutterBasicMessageChannel 0x29ec564e0> [64]
       2 (1.55K) __strong _layoutMap --> <NSMutableDictionary 0x29ec56630> [48]
          1 (1.50K) <NSMutableDictionary (Storage) 0x123c34a00> [1536]
       1 (64 bytes) __strong _pendingEvents --> <NSMutableArray 0x29ec565f0> [64]

This patch uses weak pointer to self instead of implicit strong.

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 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.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Break cycle references, due to implicitely captured 'this'
@sergiy-sc
Copy link
Contributor Author

Added unit test.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

This is fantastic; thanks for fixing. lgtm!

LGTM stamp from a Japanese personal seal

@cbracken cbracken merged commit 3c9cc06 into flutter:main Dec 20, 2023
23 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 21, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 21, 2023
…140519)

Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions)

flutter/engine@c70f0a4...1b1b2a1

2023-12-21 [email protected] Roll Dart SDK from 4a10533a4dc8 to bcf68d22f0fa (1 revision) (flutter/engine#49329)
2023-12-21 [email protected] Roll Dart SDK from 1d0c3ecd1349 to 4a10533a4dc8 (1 revision) (flutter/engine#49326)
2023-12-21 [email protected] Ensure sorted rects in ui.Canvas for legacy compatibility (flutter/engine#49309)
2023-12-21 [email protected] Roll Dart SDK from 63a452b45026 to 1d0c3ecd1349 (1 revision) (flutter/engine#49318)
2023-12-21 [email protected] [Impeller] Make IPLR files multi-platform (flutter/engine#49253)
2023-12-21 [email protected] [web] Defer injection of platform views until needed. (flutter/engine#48960)
2023-12-21 [email protected] Roll Dart SDK from 1aef027ec953 to 63a452b45026 (2 revisions) (flutter/engine#49311)
2023-12-21 [email protected] Roll Skia from 29917d8c97ca to 4b16117e94b2 (4 revisions) (flutter/engine#49310)
2023-12-21 [email protected] Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49262)
2023-12-21 [email protected] Reland `[Impeller] new blur: refactored math and fixed expanded padding size` (flutter/engine#49302)
2023-12-20 [email protected] Multiview pipeline Pt. 1: Skip illegal render calls (flutter/engine#49266)
2023-12-20 [email protected] SemanticsUpdateBuilder: make all args non-null (flutter/engine#49148)
2023-12-20 [email protected] [Impeller] fixed Rect::Contains (flutter/engine#49294)
2023-12-20 [email protected] Roll Dart SDK from 97c3b7e1885a to 1aef027ec953 (1 revision) (flutter/engine#49295)
2023-12-20 [email protected] Revert "[Impeller] new blur: refactored math and fixed expanded padding size" (flutter/engine#49298)
2023-12-20 [email protected] [Impeller] new blur: refactored math and fixed expanded padding size (flutter/engine#49206)
2023-12-20 [email protected] Multi-view pointer event (flutter/engine#46213)
2023-12-20 [email protected] [web:multiview] Only call `Renderer.clearFragmentProgramCache` on hot restart (flutter/engine#48758)
2023-12-20 [email protected] Roll Skia from 9cb1bb1164ea to 29917d8c97ca (1 revision) (flutter/engine#49289)
2023-12-20 [email protected] [Impeller] Add interactive Blur+Clip AiksTest. (flutter/engine#49283)
2023-12-20 [email protected] [macos] FlutterKeyboardManager memory leak fix (flutter/engine#48824)
2023-12-20 [email protected] Don't guard Windows arm64 Dart SDK download on the release candidate flag (flutter/engine#49244)
2023-12-20 [email protected] Fix testAppExtensionLaunching for Xcode 15/iOS 17 (flutter/engine#49242)
2023-12-20 [email protected] Roll Skia from 8060d6b36066 to 9cb1bb1164ea (2 revisions) (flutter/engine#49288)
2023-12-20 [email protected] Roll Dart SDK from ed415d966d8a to 97c3b7e1885a (1 revision) (flutter/engine#49287)
2023-12-20 [email protected] Roll Skia from d0f09ad481f7 to 8060d6b36066 (1 revision) (flutter/engine#49285)
2023-12-20 [email protected] [release] Update release config (flutter/engine#49254)
2023-12-20 [email protected] Roll Dart SDK from 1732c4c92ccd to ed415d966d8a (1 revision) (flutter/engine#49274)
2023-12-20 [email protected] Roll Dart SDK from 4c59838945d9 to 1732c4c92ccd (1 revision) (flutter/engine#49269)
2023-12-20 [email protected] Sync lints with flutter/flutter (flutter/engine#49192)
2023-12-19 [email protected] [web] Enforce onDrawFrame/onBeginFrame render rule (flutter/engine#49214)
2023-12-19 [email protected] [Docs] Add more info about running tests on iOS (flutter/engine#48859)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
...
CoderDake pushed a commit to CoderDake/flutter that referenced this pull request Dec 28, 2023
…lutter#140519)

Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions)

flutter/engine@c70f0a4...1b1b2a1

2023-12-21 [email protected] Roll Dart SDK from 4a10533a4dc8 to bcf68d22f0fa (1 revision) (flutter/engine#49329)
2023-12-21 [email protected] Roll Dart SDK from 1d0c3ecd1349 to 4a10533a4dc8 (1 revision) (flutter/engine#49326)
2023-12-21 [email protected] Ensure sorted rects in ui.Canvas for legacy compatibility (flutter/engine#49309)
2023-12-21 [email protected] Roll Dart SDK from 63a452b45026 to 1d0c3ecd1349 (1 revision) (flutter/engine#49318)
2023-12-21 [email protected] [Impeller] Make IPLR files multi-platform (flutter/engine#49253)
2023-12-21 [email protected] [web] Defer injection of platform views until needed. (flutter/engine#48960)
2023-12-21 [email protected] Roll Dart SDK from 1aef027ec953 to 63a452b45026 (2 revisions) (flutter/engine#49311)
2023-12-21 [email protected] Roll Skia from 29917d8c97ca to 4b16117e94b2 (4 revisions) (flutter/engine#49310)
2023-12-21 [email protected] Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49262)
2023-12-21 [email protected] Reland `[Impeller] new blur: refactored math and fixed expanded padding size` (flutter/engine#49302)
2023-12-20 [email protected] Multiview pipeline Pt. 1: Skip illegal render calls (flutter/engine#49266)
2023-12-20 [email protected] SemanticsUpdateBuilder: make all args non-null (flutter/engine#49148)
2023-12-20 [email protected] [Impeller] fixed Rect::Contains (flutter/engine#49294)
2023-12-20 [email protected] Roll Dart SDK from 97c3b7e1885a to 1aef027ec953 (1 revision) (flutter/engine#49295)
2023-12-20 [email protected] Revert "[Impeller] new blur: refactored math and fixed expanded padding size" (flutter/engine#49298)
2023-12-20 [email protected] [Impeller] new blur: refactored math and fixed expanded padding size (flutter/engine#49206)
2023-12-20 [email protected] Multi-view pointer event (flutter/engine#46213)
2023-12-20 [email protected] [web:multiview] Only call `Renderer.clearFragmentProgramCache` on hot restart (flutter/engine#48758)
2023-12-20 [email protected] Roll Skia from 9cb1bb1164ea to 29917d8c97ca (1 revision) (flutter/engine#49289)
2023-12-20 [email protected] [Impeller] Add interactive Blur+Clip AiksTest. (flutter/engine#49283)
2023-12-20 [email protected] [macos] FlutterKeyboardManager memory leak fix (flutter/engine#48824)
2023-12-20 [email protected] Don't guard Windows arm64 Dart SDK download on the release candidate flag (flutter/engine#49244)
2023-12-20 [email protected] Fix testAppExtensionLaunching for Xcode 15/iOS 17 (flutter/engine#49242)
2023-12-20 [email protected] Roll Skia from 8060d6b36066 to 9cb1bb1164ea (2 revisions) (flutter/engine#49288)
2023-12-20 [email protected] Roll Dart SDK from ed415d966d8a to 97c3b7e1885a (1 revision) (flutter/engine#49287)
2023-12-20 [email protected] Roll Skia from d0f09ad481f7 to 8060d6b36066 (1 revision) (flutter/engine#49285)
2023-12-20 [email protected] [release] Update release config (flutter/engine#49254)
2023-12-20 [email protected] Roll Dart SDK from 1732c4c92ccd to ed415d966d8a (1 revision) (flutter/engine#49274)
2023-12-20 [email protected] Roll Dart SDK from 4c59838945d9 to 1732c4c92ccd (1 revision) (flutter/engine#49269)
2023-12-20 [email protected] Sync lints with flutter/flutter (flutter/engine#49192)
2023-12-19 [email protected] [web] Enforce onDrawFrame/onBeginFrame render rule (flutter/engine#49214)
2023-12-19 [email protected] [Docs] Add more info about running tests on iOS (flutter/engine#48859)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
...
@sergiy-sc sergiy-sc deleted the keyboardMemLeak branch January 9, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants