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

☂️ Migrate iOS embedder objective-c code to ARC (Auto Reference Counting) #137801

Open
1 of 3 tasks
Tracked by #112232
cyanglaz opened this issue Nov 2, 2023 · 4 comments
Open
1 of 3 tasks
Tracked by #112232

☂️ Migrate iOS embedder objective-c code to ARC (Auto Reference Counting) #137801

cyanglaz opened this issue Nov 2, 2023 · 4 comments
Labels
engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list platform-ios iOS applications specifically team-ios Owned by iOS platform team triaged-ios Triaged by iOS platform team

Comments

@cyanglaz
Copy link
Contributor

cyanglaz commented Nov 2, 2023 •
edited by jmagman
Loading

Most of the files in ios Embedder and some shared darwin codes use MRC(Manual Reference Counting).

Manual Reference Counting is easy to make memory management mistake with. Objecgtive-C with Manual Reference Counting is not pleasant to work with, often scaring away contributors.

Migrate all the objective-C code to ARC will boosts Flutter's stability on iOS and making contributing to Flutter iOS embedder more efficient.

This is an umbrella issue tracks all the work needs to be done to migrate the iOS embedder to ARC:

  1. Temporarily enable Objective-C smart pointers (scoped_nsobject, scoped_block etc) to work with both ARC and MRC.
  2. Migrate all Objective-C files to ARC.
  3. Remove the support of Objective-C smart pointers
@cyanglaz cyanglaz added platform-ios iOS applications specifically engine flutter/engine repository. See also e: labels. P1 High-priority issues at the top of the work list fyi-engine For the attention of Engine team team-ios Owned by iOS platform team labels Nov 2, 2023
@chinmaygarde
Copy link
Member

Is there a general issue for this that include macOS?

@chinmaygarde chinmaygarde added the triaged-engine Triaged by Engine team label Nov 6, 2023
@cyanglaz
Copy link
Contributor Author

cyanglaz commented Nov 6, 2023

Is there a general issue for this that include macOS?

macOS is already in ARC so I didn't create an issue for macOS. I can't think of any impact it could cause on macOS. Loop in @cbracken for might insights.

@vashworth vashworth added the triaged-ios Triaged by iOS platform team label Nov 6, 2023
@cbracken
Copy link
Member

cbracken commented Nov 7, 2023

Correct -- we're already building with ARC for macOS. I can't remember if the shared code in '//flutter/platform/darwin/common' is built with ARC enabled or not.

@cbracken
Copy link
Member

cbracken commented Nov 7, 2023 •
edited
Loading

To answer my own question, the bulk of the common code is ARC-enabled.
https://github.com/flutter/engine/blob/cc925b0021330759e18960e1ccbd7e55dec3c375/shell/platform/darwin/common/BUILD.gn#L61-L63

@flutter-triage-bot flutter-triage-bot bot removed fyi-engine For the attention of Engine team triaged-engine Triaged by Engine team labels Nov 7, 2023
@cyanglaz cyanglaz mentioned this issue Nov 11, 2023
Merged
8 tasks
auto-submit bot pushed a commit to flutter/engine that referenced this issue Nov 15, 2023
[ios] introduce weak_nsobject (#47947)
b29c564 
Introduce weak_nsobject from chromium. 

There are some usages of weak_ptr wrapping Objective-C ids, weak_ptr is not really designed for ids and such usages are blocking the arc migration. 

This PR mostly copies the weak_nsobject from chromium, at the same hash that we copied the ARC/MRC compatible scoped_nsobject: https://chromium.googlesource.com/chromium/src/+/fd625125b8a6a3aceaf09993a5f74cfe5368b17f

To match how we used weak_ptr for those ids, I made some changes to the weak_nsobject:
- WeakNSObjects needs to be generated by a WeakNSObjectFactory. The WeakNSObjectFactory is owned by the objc class and acts as the generator of the WeakNSObjects. All the WeakNSObjects' derefing thread should be the same of the WeakNSObjectFactory's creation thread.
- chromuim's WeakNSObjects can be detached from the thread and re-attached to a new thread. To match our weak_ptr behavior, I changed WeakNSObjects to be only accessed from a single thread, the same as weak_ptr

This PR also moves the FlutterEngine to use WeakNSObject and updated related classes.

part of flutter/flutter#137801

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
@cyanglaz cyanglaz mentioned this issue Nov 16, 2023
Merged
8 tasks
auto-submit bot pushed a commit to flutter/engine that referenced this issue Nov 17, 2023
[Ios] move flutter_test_ios_mrc unittests to arc (#48162)
1427b9c 
Move all tests in flutter_test_ios_mrc to arc

Part of flutter/flutter#137801

Changes mostly involves trivial mrc to arc changes. non-trivial changes are commented inline. 

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
@jmagman jmagman added P2 Important issues not at the top of the work list and removed P1 High-priority issues at the top of the work list labels Jan 9, 2024
@jmagman jmagman mentioned this issue Mar 22, 2024
Merged
8 tasks
This was referenced Apr 9, 2024
Closed
Merged
Merged
Merged
auto-submit bot pushed a commit to flutter/engine that referenced this issue Apr 10, 2024
@jmagman
Migrate FlutterRestorationPlugin, FlutterTextureRegistryRelay, Flutte…
5b95daf 
…rScreenAndSceneIfLoaded to ARC (#51984)

Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

Migrate `FlutterRestorationPlugin`, `FlutterTextureRegistryRelay`, and `UIViewController+FlutterScreenAndSceneIfLoaded` from MRC to ARC.  These files do not themselves import any MRC files, making them leaf nodes in the dependency graph of MRC files.  

Doing a few at a time to make the dependency graph manageable, and to easily revert if this causes retain cycles or other memory management issues.

Part of flutter/flutter#137801.
@jmagman jmagman mentioned this issue Apr 11, 2024
Merged
auto-submit bot pushed a commit to flutter/engine that referenced this issue Apr 11, 2024
@jmagman
Update iOS KeyCodeMap dictionary literal and migrate to ARC (#51981)
76a270f 
Framework template updated in flutter/flutter#146481.  See [gen_keycodes README](https://github.com/flutter/flutter/tree/master/dev/tools/gen_keycodes ) for details.

Fixes flutter/flutter#146480 `-Wobjc-redundant-literal-use` error.

Note `-Wobjc-redundant-literal-use` is already on for clang-tidy 
https://github.com/flutter/engine/blob/6dc91bff96a56513a57ed5dd036fb16d25c945fd/.clang-tidy#L13 but in this case it's only true triggered when the file is compiled with ARC.  When I migrated this file to ARC as part of flutter/flutter#137801, it triggered the error.
@jmagman jmagman mentioned this issue Apr 12, 2024
Merged
auto-submit bot pushed a commit to flutter/engine that referenced this issue Apr 12, 2024
@jmagman
Migrate FlutterEmbedderKeyResponder to ARC (#52048)
6bd43ce 
Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

Migrate `FlutterEmbedderKeyResponder` from MRC to ARC.  Clean up some header imports which made this seem like it depended on not-yet-migrated MRC files.

Part of flutter/flutter#137801.
@jmagman jmagman mentioned this issue Apr 13, 2024
Merged
8 tasks
auto-submit bot pushed a commit to flutter/engine that referenced this issue Apr 15, 2024
@jmagman
Migrate FlutterCallbackCache and FlutterKeyboardManager to ARC (#51983)
a504499 
Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

Migrate`FlutterCallbackCache` and `FlutterKeyboardManager` from MRC to ARC.  These files do not themselves import any MRC files, making them leaf nodes in the dependency graph of MRC files.  

Doing a few at a time to make the dependency graph manageable, and to easily revert if this causes retain cycles or other memory management issues.

Part of flutter/flutter#137801.
@jmagman jmagman mentioned this issue Apr 15, 2024
Merged
auto-submit bot pushed a commit to flutter/engine that referenced this issue Apr 15, 2024
@jmagman
Migrate FlutterDartVMServicePublisher to ARC (#52081)
557ffb1 
Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

Migrate `FlutterDartVMServicePublisher` from MRC to ARC.  I validated `flutter attach` works on this engine PR, so the VM service URL is being advertised and the tool is discovering it.

Part of flutter/flutter#137801.
@jmagman jmagman mentioned this issue Apr 16, 2024
Merged
auto-submit bot pushed a commit to flutter/engine that referenced this issue Apr 16, 2024
@jmagman
Migrate vsync_waiter_ios to ARC (#52104)
ae00c28 
Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

Migrate `vsync_waiter_ios`  from MRC to ARC.

Part of flutter/flutter#137801.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
auto-submit bot pushed a commit to flutter/engine that referenced this issue Apr 16, 2024
@jmagman
Migrate FlutterUIPressProxy, ios_context*, rendering_api_selection, a…
0e9ba75 
…nd a few other files to ARC (#51633)

Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

Migrate some files from MRC to ARC.  These files do not themselves import any MRC files, only ARC-ified files like `FlutterMetalLayer.h`, making them leaf nodes in the dependency graph of MRC files.  Just doing a few at a time to make the dependency graph manageable, and to easily revert if this causes retain cycles or other memory management issues.

Part of flutter/flutter#137801.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
auto-submit bot pushed a commit to flutter/engine that referenced this issue Apr 16, 2024
@jmagman
Migrate FlutterChannelKeyResponder and FlutterSpellCheckPlugin to ARC (…
e7d8c62 
…#52148)

Clean up headers in FlutterChannelKeyResponder and FlutterSpellCheckPlugin.  Migrate to ARC.

Move `FlutterSpellCheckResult` interface into the .mm since it's only used there.

Part of flutter/flutter#137801.
Blocked by #51633 `FlutterUIPressProxy`
This was referenced Apr 18, 2024
Merged
Merged
Merged
auto-submit bot pushed a commit to flutter/engine that referenced this issue Apr 22, 2024
@jmagman
Refactor and migrate FlutterUndoManagerPlugin to ARC (#52234)
caf81dd 
Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

Migrate `FlutterUndoManagerPlugin` from MRC to ARC. 

1. Refactor so the plugin and its tests don't need to understand the details of `FlutterViewController` or `FlutterEngine` (its delegate).  This decouples the plugin, and means it doesn't depend on any MRC classes.
2. Change the delegate so conforming only requires the objects the undo plugin actually needs.

Part of flutter/flutter#137801.
auto-submit bot pushed a commit to flutter/engine that referenced this issue Apr 22, 2024
@jmagman
Break dependency cycle of FlutterViewController <-> FlutterPlatformVi…
bb4c58d 
…ew (#52271)

Trying to migrate the iOS embedder MRC to ARC, I'm attempting to break some dependency cycles so it's easier to migrate the "leaf" dependencies to ARC, working my way up.

The cycle is:
**`FlutterViewController`** -> `FlutterView` -> `FlutterPlatformView` ->  (Before this PR)**`FlutterViewController`** 

`FlutterViewController` depends on many other large MRC classes, like FlutterEngine., so I'd like to pull that one out so `FlutterView`, `FlutterPlatformView`, and `FlutterOverlayView` can be migrated to ARC in a smaller PR.

`FlutterPlatformView` only depends on `FlutterViewController` in this one place, casting a `UIViewController` and calling `-forceTouchesCancelled:`.  

https://github.com/flutter/engine/blob/55670b71eb00fbe80601b7227f68c1df4cae827b/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm#L1201

Instead, move `-forceTouchesCancelled:` to the existing `FlutterViewResponder` protocol with the other touch events, which `FlutterViewController` already implements.  The cast can then be removed, breaking the cycle.

Clean up all the imports.

Part of flutter/flutter#137801, though this doesn't actually migrate anything to ARC.
auto-submit bot pushed a commit to flutter/engine that referenced this issue Apr 24, 2024
@jmagman
Migrate ios_surface files to ARC (#52139)
77605fa 
Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

Migrate `ios_surface` classes from MRC to ARC.

Decorate C functions that take or return Objective-C objects or structs containing Objective-C objects with [`cf_audited_transfer`](https://clang.llvm.org/docs/AutomaticReferenceCounting.html#auditing-of-c-retainable-pointer-interfaces).  

Part of flutter/flutter#137801.
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 1, 2024
@jmagman
Migrate PlatformMessageHandlerIos to ARC (#52226)
2d73fa2 
Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.

Migrate `PlatformMessageHandlerIos` from MRC to ARC.  Clean up the `#include`s.

Part of flutter/flutter#137801.
@jmagman jmagman mentioned this issue May 2, 2024
Merged
jmagman added a commit to flutter/engine that referenced this issue May 10, 2024
@jmagman
Migrate FlutterView, FlutterPlatformViews, FlutterOverlayView to ARC (#…
f553caa 
…52535)

Smart pointers support ARC as of
#47612, and the unit tests were
migrated in #48162.

Migrate `FlutterView`, `FlutterPlatformViews`, and `FlutterOverlayView`
from MRC to ARC.

Part of flutter/flutter#137801.
This was referenced May 10, 2024
Merged
Merged
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 13, 2024
@jmagman
Migrate FlutterDartProject and FlutterPluginAppLifeCycleDelegate to A…
aeff9b1 
…RC (#52719)

- Migrate `FlutterDartProject` and `FlutterPluginAppLifeCycleDelegate` from MRC to ARC.  
- Swap a few dictionary `objectForKey:` to subscripting.
- Header cleanup.
- Cleanup build DEPS.

Part of flutter/flutter#137801.
This was referenced May 13, 2024
Merged
Merged
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 13, 2024
@jmagman
ios_external_view_embedder to ARC (#52782)
25a0860 
`ios_external_view_embedder` is only C++ and there was nothing to migrate.

Also add a few missing `FLUTTER_ASSERT_ARC` macros.

Part of flutter/flutter#137801.
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 14, 2024
@jmagman
Refactor Semantics in preparation for ARC migration (#52729)
86c55dd 
Split the too-large `Semantics` [MRC to ARC migration](flutter/flutter#137801) into two PRs: this one, which refactors, and the next which will actually do the migration.

1. Use properties instead of their backing ivars (except in the usual init, dealloc, getters/setters)
2. For Foundation collections prefer `copy` over `strong`.
3. Dot notation for properties
4. Change `privateSetParent:` to instead use a `readwrite` property in `SemanticsObject ()`. 
5. Switch the `semanticsObject` property from `weak` to `retain` to get the synthesized property (keeping it as `weak` is a compilation error in MRC) but I'll swap it back to a `weak` in the ARC migration PR coming next.
6. `SemanticsObjectTest` fails on my machine and passes on CI.  Switched the cleaner `CGRectEqualToRect` (and related) checks to instead assert x, y, width, height so we can see the value when it fails:
```
((CGSizeEqualToSize( scrollView.contentSize, CGSizeMake((w + scrollExtentMax) * effectivelyScale, h * effectivelyScale))) is true) failed
```
becomes:
```
((scrollView.contentSize.height) equal to (h * effectivelyScale)) failed: ("33.3333333333") is not equal to ("33.333336")
```
Use `XCTAssertEqualWithAccuracy` now that I can see it's a floating point precision issue.
@jmagman jmagman mentioned this issue May 14, 2024
Merged
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 15, 2024
@jmagman
Migrate SemanticsObject and FlutterSemanticsScrollView to ARC (#52824)
57d6b29 
On top of the `SemanticsObject` refactor #52729 migrate `SemanticsObject` and `FlutterSemanticsScrollView` to ARC.

Part of flutter/flutter#137801.
auto-submit bot pushed a commit to flutter/engine that referenced this issue May 15, 2024
@jmagman
Migrate TextInputSemanticsObject to ARC (#52785)
e473cff 
1. Migrate TextInputSemanticsObject to ARC
2. Rename the Objective-C-only `accessibility_text_entry.*` files to `TextInputSemanticsObject.*`
3. Move `FlutterInactiveTextInput` interface from the header file into the implementation, the only place it's used.

Blocked on `SemanticsObject` ARC migration #52824

Part of flutter/flutter#137801.
@jmagman jmagman mentioned this issue Jun 17, 2024
Open
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list platform-ios iOS applications specifically team-ios Owned by iOS platform team triaged-ios Triaged by iOS platform team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@chinmaygarde @cbracken @jmagman @cyanglaz @vashworth