Migrate FlutterRestorationPlugin, FlutterTextureRegistryRelay, FlutterScreenAndSceneIfLoaded to ARC #51984
Conversation
…rScreenAndSceneIfLoaded to ARC
| @@ -54,8 +54,7 @@ - (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result { | |||
|
|
|||
| - (void)setRestorationData:(NSData*)data { | |||
| if (data != _restorationData) { | |||
| [_restorationData release]; | |||
| _restorationData = [data retain]; | |||
| _restorationData = data; | |||
There was a problem hiding this comment.
This doesn't look like it needs to be a copy based on its usage. It's strong in the header.
There was a problem hiding this comment.
I would expect NSData to use copy for the same reason as NSString and the collection types: you never know when someone might pass you the mutable version instead. The framework classes generally use CoW so that copy is ~free when not actually needed.
There was a problem hiding this comment.
Do you think swapping that in the header is a breaking change somehow?
There was a problem hiding this comment.
In theory it could break us if we have code somewhere relying on passing in NSMutabledata and then modifying it after the fact. Hopefully that's not the case; it sounded like you had determined that it wasn't. But I can do a sweep too for safety.
There was a problem hiding this comment.
I only see it set in one place, and that's from method channel handler arguments, which should definitely not be something that changes after the fact. Switching to copy looks safe to me.
There was a problem hiding this comment.
I had done a sweep and it looked good to me, but for some reason when I asked just now I thought FlutterRestorationPlugin.h was in the umbrella header. That's what I get for doing this at 11:30pm and then paging it all out for a day. Thanks for double checking me 🙂
There was a problem hiding this comment.
The header for this file is declaring parent as assign; it should be weak so that it's safe.
| @@ -22,7 +22,7 @@ FLUTTER_DARWIN_EXPORT | |||
| /** | |||
| * A weak reference to a FlutterEngine that will be passed texture registration. | |||
| */ | |||
| @property(nonatomic, assign) NSObject<FlutterTextureRegistry>* parent; | |||
| @property(nonatomic, weak) NSObject<FlutterTextureRegistry>* parent; | |||
There was a problem hiding this comment.
is there a place where we manually nil out this parent?
if we have it, we can remove it now; If we don't already have it, we may have dangling pointer problem which is fixed by this PR.
There was a problem hiding this comment.
The parent here is the FlutterEngine:
See also the last time we looked at this: #37666
I'll leave this for now, maybe we can do a sweep of these when I'm done ARC-ifying.
jmagman
added
the
autosubmit
…ay, FlutterScreenAndSceneIfLoaded to ARC (flutter/engine#51984)
…146609) flutter/engine@6f8ee9f...4e33a0b 2024-04-10 skia-flutter-autoroll@skia.org Roll Skia from 5bbca5a30653 to fcb5c05acaf2 (1 revision) (flutter/engine#52036) 2024-04-10 jonahwilliams@google.com [Engine] allow --enable-asserts flag to be passed to dart vm in profile mode. (flutter/engine#52029) 2024-04-10 magder@google.com Migrate FlutterRestorationPlugin, FlutterTextureRegistryRelay, FlutterScreenAndSceneIfLoaded to ARC (flutter/engine#51984) 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 chinmaygarde@google.com,rmistry@google.com,zra@google.com 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
…lutter#146609) flutter/engine@6f8ee9f...4e33a0b 2024-04-10 skia-flutter-autoroll@skia.org Roll Skia from 5bbca5a30653 to fcb5c05acaf2 (1 revision) (flutter/engine#52036) 2024-04-10 jonahwilliams@google.com [Engine] allow --enable-asserts flag to be passed to dart vm in profile mode. (flutter/engine#52029) 2024-04-10 magder@google.com Migrate FlutterRestorationPlugin, FlutterTextureRegistryRelay, FlutterScreenAndSceneIfLoaded to ARC (flutter/engine#51984) 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 chinmaygarde@google.com,rmistry@google.com,zra@google.com 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
Smart pointers support ARC as of #47612, and the unit tests were migrated in #48162.
Migrate
FlutterRestorationPlugin,FlutterTextureRegistryRelay, andUIViewController+FlutterScreenAndSceneIfLoadedfrom 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.