macOS: fix leak in CurrentKeyboardLayout #56420
Conversation
Previously, if `layout_data` was not nil, we failed to `CFRelease` `source`. However, if `layout_data` was nil, we dif free source, then immediately set it to a new CoreFoundation object, which we then failed to free. We now use `fml::CFRef` which just does the right thing. Finally, we were returning `(__brdige_transfer)CFRetain(layout_data)` but `__bridge_transfer` releases the transferred object at the end of the enclosing expression, so this is equivalent to `(__bridge)layout_data`, which is what we now do.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). 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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM
(It's unfortunate that flm::CFRef—a class that exists solely to help manage memory—has absolutely no documentation about what the memory semantics of any of its methods is, and the only way to know what takes an owning ref is to read the implementation.)
|
test-exempt: code refactor with no semantic change (Ideally we would test the leak, but this would be a good change even if it weren't leaking before since it's inherently safer.) |
Yes! Good news though. This patch actually came from a separate patch that documents the class and migrates existing manual code to it. I broke out the bug fixes into their own patches though. |
cbracken
added
the
autosubmit
Nice! |
…158336) flutter/engine@ac50b20...371c86f 2024-11-07 [email protected] [Impeller] disable overdraw prevention for source draws. (flutter/engine#56403) 2024-11-07 [email protected] macOS: fix leak in CurrentKeyboardLayout (flutter/engine#56420) 2024-11-07 [email protected] macOS: Fix use after free in FlutterViewControllerTests (flutter/engine#56418) 2024-11-07 [email protected] Roll Skia from 6f16a8c83bf4 to e9a7546ef3d9 (3 revisions) (flutter/engine#56426) 2024-11-07 [email protected] Roll Skia from 8444ee0c8a76 to 6f16a8c83bf4 (1 revision) (flutter/engine#56425) 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] 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
Previously, if `layout_data` was not nil, we failed to `CFRelease` `source`. However, if `layout_data` was nil, we dif free source, then immediately set it to a new CoreFoundation object, which we then failed to free. We now use `fml::CFRef` which just does the right thing. Finally, we were returning `(__brdige_transfer)CFRetain(layout_data)` but `__bridge_transfer` releases the transferred object at the end of the enclosing expression, so this is equivalent to `(__bridge)layout_data`, which is what we now do. No tests since this is a refactor with no semantic changes, other than fixing an internal leak. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
2 participants
Previously, if
layout_datawas not nil, we failed toCFReleasesource. However, iflayout_datawas nil, we dif free source, then immediately set it to a new CoreFoundation object, which we then failed to free.We now use
fml::CFRefwhich just does the right thing.Finally, we were returning
(__brdige_transfer)CFRetain(layout_data)but__bridge_transferreleases the transferred object at the end of the enclosing expression, so this is equivalent to(__bridge)layout_data, which is what we now do.No tests since this is a refactor with no semantic changes, other than fixing an internal leak.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.