iOS: Migrate FlutterEngine to ARC #55590

cbracken · 2024-10-02T18:38:47Z

Migrates FlutterEnging from manual reference counting to ARC. Migrates properties from retain to strong and assign to weak (where referencing an Obj-C object).

No semantic changes, therefore no changes to tests.

Issue: flutter/flutter#137801

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.
There weren't as many ARCane memory management gotchas in this patch as I'd have expected.
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.

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

flutter-dashboard · 2024-10-02T18:38:49Z

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.

flutter-dashboard · 2024-10-02T19:26:45Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

Migrates `FlutterEnging` from manual reference counting to ARC. Migrates properties from `retain` to strong and `assign` to `weak` (where referencing an Obj-C object). No semantic changes, therefore no changes to tests. Issue: flutter/flutter#137801

…to properties

cbracken · 2024-10-03T23:44:47Z

Apologies... this one is a little bigger than the last few.

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

cbracken · 2024-10-03T23:51:15Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

-  BOOL _allowHeadlessExecution;
-  BOOL _restorationEnabled;
  FlutterBinaryMessengerRelay* _binaryMessenger;
  FlutterTextureRegistryRelay* _textureRegistry;


These two have public getters for NSObject<FlutterBinaryMessenger> and NSObject<FlutterTextureRegistryRelay>.

I'd prefer to hold off on these to a future patch and they're alright as-is for now.

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

cbracken · 2024-10-04T00:03:25Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

 - (void)startProfiler {
  FML_DCHECK(!_threadHost->name_prefix.empty());
  _profiler_metrics = std::make_shared<flutter::ProfilerMetricsIOS>();
+  __weak FlutterEngine* weakSelf = self;


This is just preserving existing weak C++ lambda capture of self.

cbracken · 2024-10-04T00:06:30Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

  if (_shell && self.shell.IsSetup()) {
-    FlutterPlatformPlugin* platformPlugin = _platformPlugin.get();
-    [_platformChannel.get() setMethodCallHandler:^(FlutterMethodCall* call, FlutterResult result) {
-      [platformPlugin handleMethodCall:call result:result];


Here and a couple more below -- notice that I've moved all these to use weakSelf.somePlugin.

Technically, this does very slightly change the semantics here, but for the better since if self were to be cleaned up, these will all devolve into no-op messages to nil.

cbracken · 2024-10-04T00:13:20Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformPlugin.mm

 @implementation FlutterPlatformPlugin

- (instancetype)initWithEngine:(fml::WeakNSObject<FlutterEngine>)engine {
+- (instancetype)initWithEngine:(FlutterEngine*)engine {


We assign to _engine, which is a weak property.

cbracken · 2024-10-04T00:15:53Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformPlugin.h

 - (instancetype)init NS_UNAVAILABLE;
 + (instancetype)new NS_UNAVAILABLE;
- (instancetype)initWithEngine:(fml::WeakNSObject<FlutterEngine>)engine NS_DESIGNATED_INITIALIZER;
+- (instancetype)initWithEngine:(FlutterEngine*)engine NS_DESIGNATED_INITIALIZER;


We can make this (and probably some others) (__weak FlutterEngine*) once FlutterViewController.mm (which also #imports this) is migrated to ARC, but either way we're assigning to a weak property.

jmagman

LGTM once the [labelPrefix copy] is added to init.

This code gives me 30% less heebie jeebies than it did before. Thank you so much for your service 🫡

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

jmagman · 2024-10-04T02:45:08Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

  std::shared_ptr<flutter::ProfilerMetricsIOS> _profiler_metrics;
  std::shared_ptr<flutter::SamplingProfiler> _profiler;

-  // Channels


shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

stuartmorgan-g

🎉 LGTM with one or two fixes and some optional nits. (I did a full audit of all of the blocks in addition to reviewing the diff; I know you did as well, but just as a record that the area most likely to have subtle bugs did get more eyes.)

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

stuartmorgan-g · 2024-10-04T11:37:05Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

    [center removeObserver:_flutterViewControllerWillDeallocObserver];
-    [_flutterViewControllerWillDeallocObserver release];
  }
  [center removeObserver:self];


Optional: It's harmless to leave, but this line hasn't been necessary since we dropped iOS 8 support. (The thing above looks like it probably is still because of the way it was registered.)

Will followup in another PR just to "minimise" the change in this patch, which admittedly there is nothing minimal about at the moment. Mostly just want to work through why it's unnecessary, and it's currently to early in the morning for my brain to do that.

Totally fair :)

To save your brain work later, it's because of this:

If your app targets iOS 9.0 and later or macOS 10.11 and later, and you used addObserver:selector:name:object:, you do not need to unregister the observer. If you forget or are unable to remove the observer, the system cleans up the next time it would have posted to it.

https://developer.apple.com/documentation/foundation/nsnotificationcenter/1413994-removeobserver

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

stuartmorgan-g · 2024-10-04T11:53:22Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

-        [self recreatePlatformViewController];
-        self->_platformViewsController->SetTaskRunner(
+      [weakSelf](flutter::Shell& shell) {
+        FlutterEngine* strongSelf = weakSelf;


This code isn't safe if the comment you removed was wrong (and even if it happens to currently be safe, we shouldn't leave it this way) because the block is unconditionally using strongSelf. There should be a:

if (!strongSelf) { return; }

right after this line.

Nice catch -- did this in a couple others but totally missed it here. This block is expected to return a std::unique_ptr<flutter::PlatformView>, so returning a null one here in the hopes that whoever calls this is careful enough to check it (they are, at the moment):

engine/shell/common/shell.cc

Lines 236 to 239 in e03449e

auto platform_view = on_create_platform_view(*shell.get());

if (!platform_view || !platform_view->GetWeakPtr()) {

return nullptr;

}

…156239) flutter/engine@2054840...d38f5e5 2024-10-04 [email protected] [Impeller] generate mipmaps for toImage. (flutter/engine#55655) 2024-10-04 [email protected] iOS: Migrate FlutterEngine to ARC (flutter/engine#55590) 2024-10-04 [email protected] Roll Skia from 0dfa080b5d71 to e8e0a8c46345 (1 revision) (flutter/engine#55652) 2024-10-04 [email protected] Roll Dart SDK from 750b6e44b765 to ecba03620fc8 (1 revision) (flutter/engine#55650) 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

Causing flakes on the _Mac mac_unopt (Cocoon)_ shard. This reverts commit 449df25. Issue: flutter/flutter#156177 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Migrates `FlutterEnging` from manual reference counting to ARC. Migrates properties from `retain` to strong and `assign` to `weak` (where referencing an Obj-C object). No semantic changes, therefore no changes to tests. Issue: flutter#137801 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions bot added the platform-ios label Oct 2, 2024

cbracken marked this pull request as draft October 2, 2024 19:25

jmagman reviewed Oct 2, 2024

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm Show resolved Hide resolved

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm Show resolved Hide resolved

cbracken added 25 commits October 3, 2024 16:32

iOS: Migrate FlutterEngine to ARC

f648cce

Migrates `FlutterEnging` from manual reference counting to ARC. Migrates properties from `retain` to strong and `assign` to `weak` (where referencing an Obj-C object). No semantic changes, therefore no changes to tests. Issue: flutter/flutter#137801

Migrate _dartProject to property

d7af9ab

Migrate _labelPrefix to property

9c8140c

Migrate _platformPlugin to a property

bfbd8ae

Migrate _textInputPlugin to property

6e39593

Migrate _undoManagerPlugin to property

708ef4c

Add TODO to use weakSelf

9ede541

Migrate _spellCheckPlugin to property

4d80478

Migrate _restorationPlugin to property

8c67e32

Migrate _localizationChannel to property

582b846

Migrate _navigationChannel to property

6d44d05

Migrate _restorationChannel to property

cde9792

Migrate _platformChannel to property

59a5666

Migrate _platformViewChannel to property

4fb023b

Migrate _textInputChannel to property

0cfd5dd

Migrate _undoManagerChannel to property

30be228

Migrate _scribbleChannel to property

c87e1fc

Migrate _spellCheckChannel to property

4beaff5

Migrate _lifecycleChannel to property

3ff898c

Migrate _systemChannel to property

9cb6ca2

Migrate _settingsChannel to property

a25f67d

Migrate _keyEventChannel to property

3bbdc73

Migrate _screenshotChannel to property

67e50cc

Migrate _publisher to property

f93f238

Migrate _nextTextureId, _allowHeadlessExecution, _restorationEnabled …

dab2366

…to properties

cbracken added 2 commits October 3, 2024 16:32

Add TODO to migrate _viewController

1542c3f

format

f676742

cbracken marked this pull request as ready for review October 3, 2024 23:38

cbracken requested a review from hellohuanlin October 3, 2024 23:40

cbracken commented Oct 3, 2024

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm Show resolved Hide resolved

cbracken commented Oct 3, 2024

View reviewed changes

Use weakSelf for handleMethodCall on plugins

66923dd

cbracken commented Oct 4, 2024

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm Outdated Show resolved Hide resolved

cbracken commented Oct 4, 2024

View reviewed changes

cbracken requested a review from stuartmorgan-g October 4, 2024 00:07

cbracken commented Oct 4, 2024

View reviewed changes

jmagman approved these changes Oct 4, 2024

View reviewed changes

copy labelPrefix

0ac6fa1

stuartmorgan-g approved these changes Oct 4, 2024

View reviewed changes

cbracken added 2 commits October 4, 2024 07:15

Review feedback

ac5e0c2

Reformat

d93d56e

cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 4, 2024

auto-submit bot merged commit 449df25 into flutter:main Oct 4, 2024
30 checks passed

engine-flutter-autoroll mentioned this pull request Oct 4, 2024

Roll Flutter Engine from 205484009711 to 449df252a2f1 (3 revisions) flutter/flutter#156234

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 4, 2024

449df252a iOS: Migrate FlutterEngine to ARC (flutter/engine#55590)

2b9da43

cbracken deleted the arc-engine branch October 4, 2024 17:00

engine-flutter-autoroll mentioned this pull request Oct 4, 2024

Roll Flutter Engine from 205484009711 to d38f5e560a98 (4 revisions) flutter/flutter#156239

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 4, 2024

449df252a iOS: Migrate FlutterEngine to ARC (flutter/engine#55590)

1c1fa39

	auto platform_view = on_create_platform_view(*shell.get());
	if (!platform_view \|\| !platform_view->GetWeakPtr()) {
	return nullptr;
	}

iOS: Migrate FlutterEngine to ARC #55590

iOS: Migrate FlutterEngine to ARC #55590

Uh oh!

Conversation

cbracken commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Oct 2, 2024

Uh oh!

flutter-dashboard bot commented Oct 2, 2024

Uh oh!

Uh oh!

Uh oh!

cbracken commented Oct 3, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cbracken commented Oct 2, 2024 •

edited

Loading

cbracken Oct 4, 2024 •

edited

Loading

cbracken Oct 4, 2024 •

edited

Loading

cbracken Oct 4, 2024 •

edited

Loading