This repository was archived by the owner on Feb 25, 2025. It is now read-only.
iOS: Eliminate FlutterEngine dealloc notification#56650
Merged
auto-submit[bot] merged 1 commit intoflutter:mainfrom Nov 17, 2024
cbracken:eliminate-bridge-cast
Merged
iOS: Eliminate FlutterEngine dealloc notification#56650auto-submit[bot] merged 1 commit intoflutter:mainfrom cbracken:eliminate-bridge-cast
auto-submit[bot] merged 1 commit intoflutter:mainfrom
cbracken:eliminate-bridge-cast
Conversation
cbracken
requested review from
hellohuanlin,
jmagman,
loic-sharma and
stuartmorgan-g
cbracken
commented
Nov 16, 2024
| // That's reasonable behaviour on a regular NSPointerArray but not for a weakObjectPointerArray. | ||
| // As a workaround, we mutate it first. See: http://www.openradar.me/15396578 | ||
| [self.engines addPointer:nil]; | ||
| [self.engines compact]; |
Member
Author
There was a problem hiding this comment.
I won't comment on just how much time I spent staring at and debugging this line to arrive at the comment above. Thanks, noble OpenRadar submitter, for saving my sanity.
cbracken
commented
Nov 16, 2024
| XCTAssertNotNil(spawner); | ||
| XCTAssertNotNil(weakSpawner); | ||
| } | ||
| XCTAssertNil(weakSpawner); |
Member
Author
There was a problem hiding this comment.
I could probably get rid of this additional check, but it's arguably useful for avoiding any future accidents wherein someone updates the implementation to hold a strong reference that retains the engine instead of a weak reference. :/
stuartmorgan-g
approved these changes
Nov 17, 2024
shell/platform/darwin/ios/framework/Source/FlutterEngineGroup.mm
Outdated
Show resolved
Hide resolved
FlutterEngineGroup keeps an array of all live FlutterEngine instances it has created such that after the first engine, it can spawn further engines using the first. Previously we manually managed this array by adding engines to it upon creation, then having [FlutterEngine dealloc] emit a notification such that FlutterEngineGroup can listen for it, and remove instances from the array upon dealloc. Instead, we now use an NSPointerArray of of weak pointers such that pointers are automatically nilled out by ARC after the last strong reference is collected. This eliminates the need for the manual notification during dealloc. Unfortunately, NSPointerArray is "clever" and assumes that if the array has not been mutated to store a nil pointer since its last compact call, it must must contain a nil pointer and can thus skip compaction. When holding weak pointers under ARC, this is no longer the case and so we work around the issue by storing a nil pointer before calling compact. See: http://www.openradar.me/15396578 I'm not thrilled with the fact that we're replacing one sort of TODO with another, but the code is much simpler and avoids relying on a trip through the notification center, which seems objectively better. Issue: flutter/flutter#155943
cbracken
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 17, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 17, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 17, 2024
github-merge-queue bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Nov 17, 2024
flutter/engine@6f9854a...9d0720a 2024-11-17 [email protected] Roll Skia from b88c7b03a838 to 1086e39c04cf (1 revision) (flutter/engine#56654) 2024-11-17 [email protected] iOS: Eliminate FlutterEngine dealloc notification (flutter/engine#56650) 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
nick9822
pushed a commit
to nick9822/flutter
that referenced
this pull request
Dec 18, 2024
FlutterEngineGroup keeps an array of all live FlutterEngine instances it has created such that after the first engine, it can spawn further engines using the first. Previously we manually managed this array by adding engines to it upon creation, then having [FlutterEngine dealloc] emit a notification such that FlutterEngineGroup can listen for it, and remove instances from the array upon reception of the notification. Instead, we now use an NSPointerArray of of weak pointers such that pointers are automatically nilled out by ARC after the last strong reference is collected. This eliminates the need for the manual notification during dealloc. Unfortunately, NSPointerArray is "clever" and assumes that if the array has not been mutated to store a nil pointer since its last compact call, it must not contain a nil pointer and can thus skip compaction. Under ARC, weak pointers are automatically nilled out when the last strong reference is released, so the above heuristic is no longer valid. We work around the issue by storing a nil pointer before calling compact. See http://www.openradar.me/15396578 for the radar tracking this bug. I'm not thrilled with the fact that we're replacing one sort of TODO with another, but the code is much simpler and avoids relying on a trip through the notification center, which seems objectively better. Issue: flutter#155943 Issue: flutter#137801 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FlutterEngineGroup keeps an array of all live FlutterEngine instances it has created such that after the first engine, it can spawn further engines using the first.
Previously we manually managed this array by adding engines to it upon creation, then having [FlutterEngine dealloc] emit a notification such that FlutterEngineGroup can listen for it, and remove instances from the array upon reception of the notification.
Instead, we now use an NSPointerArray of of weak pointers such that pointers are automatically nilled out by ARC after the last strong reference is collected. This eliminates the need for the manual notification during dealloc.
Unfortunately, NSPointerArray is "clever" and assumes that if the array has not been mutated to store a nil pointer since its last compact call, it must not contain a nil pointer and can thus skip compaction. Under ARC, weak pointers are automatically nilled out when the last strong reference is released, so the above heuristic is no longer valid. We work around the issue by storing a nil pointer before calling compact.
See http://www.openradar.me/15396578 for the radar tracking this bug.
I'm not thrilled with the fact that we're replacing one sort of TODO with another, but the code is much simpler and avoids relying on a trip through the notification center, which seems objectively better.
Issue: flutter/flutter#155943
Issue: flutter/flutter#137801
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.