Fix iOS embedder memory management and other analyzer warnings #29623

jmagman · 2021-11-09T21:01:15Z

Run the static analyzer on the iOS embedder. Fix the issues, mostly related to memory management.

Hopefully soon we can run clang-tidy in post-submit or the push hooks flutter/flutter#91857

To run clang-tidy for iOS:

dart tools/clang_tidy/bin/main.dart --lint-all --compile-commands out/ios_debug/compile_commands.json --repo .

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.
I added new tests to check the change I am making or feature I am adding, or Hixie said 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.

jmagman · 2021-11-09T21:01:52Z

shell/platform/darwin/ios/framework/Headers/FlutterCallbackCache.h

 * The name of the callback.
 */
-@property(retain) NSString* callbackName;
+@property(copy) NSString* callbackName;


Foundation objects with mutable variants (NSMutableString) should be copy.

jmagman · 2021-11-09T21:02:11Z

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

                                                     options:0
                                                       error:NULL];
-  return [[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding];
+  [networkConfigArray release];


networkConfigArray was leaked.

The style in projects I worked on prior to the advent of ARC was that unless there was a compelling reason not to--e.g., code that's called in a tight loop--it's far better to autorelease as part of the alloc/init statement, because then there's no chance of introducing a leak later via early return. It also made it much easier to review code for correctness, because any line that did an alloc/init but not an autorelease was immediately suspect, triggering a check for a corresponding release.

So tl;dr, I would make this part of line 276 instead.

In MRC projects I've worked on the style guide was to init/release within a scope so we didn't hit memory highwater marks waiting for the autorelease pool to drain. And autorelease on return statements.
This style avoids memory bloat, but your suggested way probably makes it easier to do the right thing with memory management. MRC documentation/examples have fallen off the internet and there's no style guide in play, and Apple never published one. I'd rather avoid crashes and leaks than theoretical memory bloat issues (though they weren't theoretical in other projects I've worked on). So let's go with your style suggestion.

jmagman · 2021-11-09T21:03:32Z

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

 * Set by the initializer.
 */
-@property(nonatomic) FlutterSendKeyEvent sendEvent;
+@property(nonatomic, copy, readonly) FlutterSendKeyEvent sendEvent;


engine/shell/platform/darwin/ios/framework/Source/FlutterEmbedderKeyResponder.mm

Line 487 in b7459d5

_sendEvent = [sendEvent copy];

jmagman · 2021-11-09T21:04:08Z

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

              if (weak.get().enableObservatoryPublication) {
                [[weak.get() delegate] publishServiceProtocolPort:url];
              }
+              [url release];


url was leaked.

jmagman · 2021-11-09T21:04:54Z

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

  FlutterSendKeyEvent sendEvent =
      ^(const FlutterKeyEvent& event, FlutterKeyEventCallback callback, void* userData) {
-        [_engine.get() sendKeyEvent:event callback:callback userData:userData];
+        [weakSelf.get()->_engine.get() sendKeyEvent:event callback:callback userData:userData];


This didn't show up in the linter, but this retain cycle was exposed when I wrote the testReleasesKeyboardManagerOnDealloc and the view controller wasn't being released.

jmagman · 2021-11-09T21:06:03Z

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

 - (void)encodeRestorableStateWithCoder:(NSCoder*)coder {
  NSData* restorationData = [[_engine.get() restorationPlugin] restorationData];
  [coder encodeDataObject:restorationData];
+  [super encodeRestorableStateWithCoder:coder];


This was an analyzer warning, encodeRestorableStateWithCoder needs to encode the super class's state.

jmagman · 2021-11-09T21:07:20Z

shell/platform/darwin/ios/rendering_api_selection.mm

  if (@available(iOS METAL_IOS_VERSION_BASELINE, *)) {
    auto device = MTLCreateSystemDefaultDevice();
    ios_version_supports_metal = [device supportsFeatureSet:MTLFeatureSet_iOS_GPUFamily1_v3];
+    [device release];


MTLCreateSystemDefaultDevice is NS_RETURNS_RETAINED

jmagman · 2021-11-09T21:09:44Z

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

+  [self.keyboardManager addPrimaryResponder:responder];
+  [responder release];
+  FlutterTextInputPlugin* textInputPlugin = self.engine.textInputPlugin;
+  if (textInputPlugin != nil) {


self.engine is nullable. When I tested it threw when adding nil to the secondary responder array.

stuartmorgan-g

LGTM with nits

stuartmorgan-g · 2021-11-10T03:38:39Z

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

                                                     options:0
                                                       error:NULL];
-  return [[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding];
+  [networkConfigArray release];


The style in projects I worked on prior to the advent of ARC was that unless there was a compelling reason not to--e.g., code that's called in a tight loop--it's far better to autorelease as part of the alloc/init statement, because then there's no chance of introducing a leak later via early return. It also made it much easier to review code for correctness, because any line that did an alloc/init but not an autorelease was immediately suspect, triggering a check for a corresponding release.

So tl;dr, I would make this part of line 276 instead.

stuartmorgan-g · 2021-11-10T03:43:32Z

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

 * of the key down event.
 */
-@property(nonatomic) NSMutableDictionary<NSNumber*, NSNumber*>* pressingRecords;
+@property(nonatomic, retain, readonly) NSMutableDictionary<NSNumber*, NSNumber*>* pressingRecords;


Why do we have readonly private properties?

I didn't write this and I don't want to start a stylistic war, but I think properties should always preferred over ivars (and the backing ivar only touched in init, dealloc, getters, and setters), even when private, because of KVO, KVC, to enforce readonly, to enforce copy when it's readwrite.

https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/EncapsulatingData/EncapsulatingData.html

It’s best practice to use a property on an object any time you need to keep track of a value or another object.

Can we put a pin in this question for this PR? I only did the bare minimum to make the analyzer happy, plus some tests.

I'm aware of the property vs ivar debate; I actually don't have a strong opinion on that. I was just curious about the addition of readonly to a private proprety. It just seems odd to say that code within a class can modify the collection, but not replace it with a different collection.

But this wasn't intended to be a PR-blocking question.

I'm aware of the property vs ivar debate; I actually don't have a strong opinion on that.

Guess I'm always geared up for a religious war in these kinds of reviews 😉

Re: the readonly, you're right, that wasn't a static analyzer warning. I did it because the property wasn't being set other than in init, so it could be made readonly, which I did in the spirit of preferring stricter property attributes, readonly, immutable, nonnull, etc. But yeah, it's not API just private so it doesn't really matter (unless you're KVOing on the property and need to handle updating notifications in the property setter, which we aren't). I can change it back if you prefer.

stuartmorgan-g · 2021-11-10T03:43:58Z

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

 * The primary responders added by addPrimaryResponder.
 */
-@property(nonatomic) NSMutableArray<id<FlutterKeyPrimaryResponder>>* primaryResponders;
+@property(nonatomic, retain, readonly)


Same question here.

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

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

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

jmagman · 2021-11-10T23:00:49Z

shell/platform/darwin/ios/framework/Source/FlutterUmbrellaImport.m

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

+// FLUTTER_NOLINT: https://github.com/flutter/flutter/issues/93360


This one is new.
FlutterUmbrellaImport is used in the copy_and_verify_framework_module target, is not included in the engine framework source, and cannot be compiled on its own.

When clang-tidy is run on this file, it fails with a clang-diagnostic-error (compilation failure). Add a FLUTTER_NOLINT to this file with link to flutter/flutter#93360.

flutter/engine#29623)

…er#29623) 有一些内存泄露，有些代码修改是在新代码，这个分支就不合了

jmagman self-assigned this Nov 9, 2021

flutter-dashboard bot added the platform-ios label Nov 9, 2021

google-cla bot added the cla: yes label Nov 9, 2021

jmagman commented Nov 9, 2021

View reviewed changes

jmagman changed the title ~~Fix memory management analyzer warnings~~ Fix memory management and other analyzer warnings Nov 9, 2021

jmagman requested review from gaaclarke and stuartmorgan-g November 9, 2021 22:10

jmagman changed the title ~~Fix memory management and other analyzer warnings~~ Fix iOS embedder memory management and other analyzer warnings Nov 10, 2021

stuartmorgan-g approved these changes Nov 10, 2021

View reviewed changes

jmagman added 2 commits November 10, 2021 14:41

Fix memory management analyzer warnings

7ca4e71

Review edits

777d088

jmagman force-pushed the warnings-memory branch from 5bf262e to 777d088 Compare November 10, 2021 22:59

jmagman commented Nov 10, 2021

View reviewed changes

Fix a few more releases exposed by switching from strong to retain

3087cdc

jmagman added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 12, 2021

fluttergithubbot merged commit c19137f into flutter:master Nov 12, 2021

jmagman deleted the warnings-memory branch November 12, 2021 21:18

engine-flutter-autoroll mentioned this pull request Nov 12, 2021

Roll Engine from 85e06f206389 to c19137fd56fb (1 revision) flutter/flutter#93572

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 12, 2021

c19137f Fix iOS embedder memory management and other analyzer warnings (

212920a

flutter/engine#29623)

This was referenced Nov 15, 2021

Lint: FlutterUmbrellaImport [clang-diagnostic-error] 'Flutter/Flutter.h' file not found flutter/flutter#93360

Closed

Treat clang-analyzer-osx warnings as errors #29755

Merged

jmagman mentioned this pull request Nov 23, 2021

Run clang tidy on more targets flutter/flutter#61661

Open

5 tasks

yx-mike added a commit to yx-mike/engine that referenced this pull request May 18, 2022

Fix iOS embedder memory management and other analyzer warnings (flutt…

69bb03a

…er#29623) 有一些内存泄露，有些代码修改是在新代码，这个分支就不合了

Fix iOS embedder memory management and other analyzer warnings #29623

Fix iOS embedder memory management and other analyzer warnings #29623

Uh oh!

Conversation

jmagman commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 12, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmagman commented Nov 9, 2021 •

edited

Loading

jmagman Nov 12, 2021 •

edited

Loading