Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Removed retain cycle from notification center. #13073

Merged
gaaclarke merged 2 commits into from
Oct 11, 2019
Merged

Removed retain cycle from notification center. #13073

gaaclarke merged 2 commits into from
Oct 11, 2019

Conversation

@gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Oct 11, 2019

related PR: #13062

@gaaclarke gaaclarke requested a review from jmagman October 11, 2019 00:14
@gaaclarke gaaclarke mentioned this pull request Oct 11, 2019
Closed
@jmagman
Copy link
Member

jmagman commented Oct 11, 2019

@gaaclarke and I were talking on chat, looks like the observer needs to be removed. Will hold off reviewing for now.

jmagman
jmagman reviewed Oct 11, 2019
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't comment on these lines because GitHub UI reasons, but pre-iOS 9 self as an observer should be removed from the notification center on dealloc.
so

[center removeObserver:self name:UIApplicationDidReceiveMemoryWarningNotification object:nil];

can become:

[center removeObserver:self];

@gaaclarke
Copy link
Member Author

gaaclarke commented Oct 11, 2019

@gaaclarke and I were talking on chat, looks like the observer needs to be removed. Will hold off reviewing for now.

Thanks @jmagman I learned 2 things about notification center that I thought I knew.

  1. The object returned from addObserver has a retain on the block.
  2. The object returned from addObserver still has to be explicitly removed.

Both are not the decisions I would have made shrug.

jmagman
jmagman approved these changes Oct 11, 2019
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for explaining over chat!

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm
self.iosPlatformView->SetOwnerViewController(_viewController);
[self maybeSetupPlatformViewChannels];

__block FlutterEngine* blockSelf = self;
Copy link
Member

@jmagman jmagman Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔮 The retain cycle will be reintroduced when someone finally hits the "modernize to ARC" button without auditing all the blocks where weak memory management is expected. 🔮

Copy link
Member Author

@gaaclarke gaaclarke Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New word, "retro-modernize"? When we move to ARC it should work as well. There is a subtle difference between __block and __weak (like how they treat primitives). For an object pointer it should behave the same. I'm more sure about that than my understanding of the addObserverForName:object:queue:usingBlock: but obviously I've been wrong before, haha.

@gaaclarke gaaclarke merged commit 4243fdb into flutter:master Oct 11, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 11, 2019
@engine-flutter-autoroll
4243fdb Removed retain cycle from notification center. (flutter/engin…
32d0599 
…e#13073)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 11, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 14, 2019
@engine-flutter-autoroll
4243fdb Removed retain cycle from notification center. (flutter/engin…
094614d 
…e#13073)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 14, 2019
Merged
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Oct 15, 2019
@engine-flutter-autoroll
Roll engine eed171f..66bf00b (44 commits) (#42695)
1d4d63a 
[email protected]:flutter/engine.git/compare/eed171ff3538...66bf00b

git log eed171f..66bf00b --no-merges --oneline
2019-10-14 [email protected] refactoring chrome_installer (flutter/engine#13122)
2019-10-14 [email protected] Fire PlatformViewController FlutterView callbacks (flutter/engine#13015)
2019-10-14 [email protected] iOS Platform View: Fixed overrelease of the observer. (flutter/engine#13093)
2019-10-14 [email protected] [web] Add basic color per vertex drawVertices API support (flutter/engine#13066)
2019-10-14 [email protected] Support keyboard types on mobile browsers (flutter/engine#13044)
2019-10-14 [email protected] Change IO thread shader cache strategy (flutter/engine#13121)
2019-10-14 [email protected] Roll fuchsia/sdk/core/linux-amd64 from _GTls... to xRgq0... (flutter/engine#13115)
2019-10-14 [email protected] Roll src/third_party/skia 3838fe3c82b4..a7e1b45d9c28 (1 commits) (flutter/engine#13117)
2019-10-14 [email protected] Roll fuchsia/sdk/core/mac-amd64 from E3s7G... to Lk7iT... (flutter/engine#13116)
2019-10-14 [email protected] Roll src/third_party/dart 5fad012d02..892fcf2c45 (15 commits)
2019-10-14 [email protected] Integrate more SkParagraph builder patches (flutter/engine#13094)
2019-10-13 [email protected] Roll fuchsia/sdk/core/mac-amd64 from T3Xkz... to E3s7G... (flutter/engine#13114)
2019-10-12 [email protected] Roll fuchsia/sdk/core/linux-amd64 from lJPDX... to _GTls... (flutter/engine#13113)
2019-10-12 [email protected] Roll fuchsia/sdk/core/mac-amd64 from W9PBe... to T3Xkz... (flutter/engine#13112)
2019-10-12 [email protected] Roll fuchsia/clang/linux-amd64 from q4DVY... to 2EAvs... (flutter/engine#13111)
2019-10-12 [email protected] Roll fuchsia/clang/mac-amd64 from zpVtV... to xTJWs... (flutter/engine#13110)
2019-10-12 [email protected] Roll fuchsia/sdk/core/linux-amd64 from LsxeL... to lJPDX... (flutter/engine#13109)
2019-10-12 [email protected] Roll fuchsia/sdk/core/mac-amd64 from ShVbh... to W9PBe... (flutter/engine#13108)
2019-10-12 [email protected] Roll src/third_party/dart 90ff37e011..5fad012d02 (6 commits)
2019-10-12 [email protected] Roll src/third_party/dart 42dcdf903c..90ff37e011 (7 commits)
2019-10-12 [email protected] Roll src/third_party/skia 8c6c8b0c42e2..3838fe3c82b4 (3 commits) (flutter/engine#13105)
2019-10-12 [email protected] Analyze framework Dart code in presubmit tests (flutter/engine#13037)
2019-10-12 [email protected] [dart_aot_runner] Complete the port of dart_aot_runner (flutter/engine#13103)
2019-10-11 [email protected] Move initialization into FlutterEngine (flutter/engine#12806)
2019-10-11 [email protected] Update felt README (flutter/engine#13097)
2019-10-11 [email protected] Roll src/third_party/dart 9f33e8da04..42dcdf903c (7 commits)
2019-10-11 [email protected] [dart_aot_runner] Generate vmservice aotsnapshots (flutter/engine#13101)
2019-10-11 [email protected] ColorFilter matrix docs (flutter/engine#13100)
2019-10-11 [email protected] cleanup gen_package.py (flutter/engine#13089)
2019-10-11 [email protected] [dart_aot_runner] Use the host_toolchain to build kernels (flutter/engine#13096)
2019-10-11 [email protected] do not wrap font family name (flutter/engine#12801)
2019-10-11 [email protected] Roll src/third_party/skia df640e6d14a5..8c6c8b0c42e2 (12 commits) (flutter/engine#13098)
2019-10-11 [email protected] Remove persistent cache unittest timeout (flutter/engine#13091)
2019-10-11 [email protected] Added FlutterActivity and FlutterFragment hook to cleanUpFlutterEngine() as symmetry for configureFlutterEngine(). (#41943) (flutter/engine#12987)
2019-10-11 [email protected] use rest args for specifying test targets (flutter/engine#13088)
2019-10-11 [email protected] Snapshot the felt tool for faster start-up (flutter/engine#13090)
2019-10-11 [email protected] Roll src/third_party/dart 965b8cb1d8..9f33e8da04 (15 commits)
2019-10-11 [email protected] java imports/style (flutter/engine#13082)
2019-10-11 [email protected] Removed retain cycle from notification center. (flutter/engine#13073)
2019-10-11 [email protected] Roll fuchsia/sdk/core/mac-amd64 from U4IBp... to ShVbh... (flutter/engine#13092)
2019-10-11 [email protected] Roll fuchsia/sdk/core/linux-amd64 from fk6ou... to LsxeL... (flutter/engine#13083)
2019-10-11 [email protected] Roll src/third_party/skia 6c2b2bb02402..df640e6d14a5 (1 commits) (flutter/engine#13080)
2019-10-11 [email protected] Gen package output corrected (flutter/engine#13086)
2019-10-11 [email protected] Print more output when gen_package fails (flutter/engine#13085)

...
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
@engine-flutter-autoroll @Inconnu08
Roll engine eed171f..66bf00b (44 commits) (flutter#42695)
f7381cc 
[email protected]:flutter/engine.git/compare/eed171ff3538...66bf00b

git log eed171f..66bf00b --no-merges --oneline
2019-10-14 [email protected] refactoring chrome_installer (flutter/engine#13122)
2019-10-14 [email protected] Fire PlatformViewController FlutterView callbacks (flutter/engine#13015)
2019-10-14 [email protected] iOS Platform View: Fixed overrelease of the observer. (flutter/engine#13093)
2019-10-14 [email protected] [web] Add basic color per vertex drawVertices API support (flutter/engine#13066)
2019-10-14 [email protected] Support keyboard types on mobile browsers (flutter/engine#13044)
2019-10-14 [email protected] Change IO thread shader cache strategy (flutter/engine#13121)
2019-10-14 [email protected] Roll fuchsia/sdk/core/linux-amd64 from _GTls... to xRgq0... (flutter/engine#13115)
2019-10-14 [email protected] Roll src/third_party/skia 3838fe3c82b4..a7e1b45d9c28 (1 commits) (flutter/engine#13117)
2019-10-14 [email protected] Roll fuchsia/sdk/core/mac-amd64 from E3s7G... to Lk7iT... (flutter/engine#13116)
2019-10-14 [email protected] Roll src/third_party/dart 5fad012d02..892fcf2c45 (15 commits)
2019-10-14 [email protected] Integrate more SkParagraph builder patches (flutter/engine#13094)
2019-10-13 [email protected] Roll fuchsia/sdk/core/mac-amd64 from T3Xkz... to E3s7G... (flutter/engine#13114)
2019-10-12 [email protected] Roll fuchsia/sdk/core/linux-amd64 from lJPDX... to _GTls... (flutter/engine#13113)
2019-10-12 [email protected] Roll fuchsia/sdk/core/mac-amd64 from W9PBe... to T3Xkz... (flutter/engine#13112)
2019-10-12 [email protected] Roll fuchsia/clang/linux-amd64 from q4DVY... to 2EAvs... (flutter/engine#13111)
2019-10-12 [email protected] Roll fuchsia/clang/mac-amd64 from zpVtV... to xTJWs... (flutter/engine#13110)
2019-10-12 [email protected] Roll fuchsia/sdk/core/linux-amd64 from LsxeL... to lJPDX... (flutter/engine#13109)
2019-10-12 [email protected] Roll fuchsia/sdk/core/mac-amd64 from ShVbh... to W9PBe... (flutter/engine#13108)
2019-10-12 [email protected] Roll src/third_party/dart 90ff37e011..5fad012d02 (6 commits)
2019-10-12 [email protected] Roll src/third_party/dart 42dcdf903c..90ff37e011 (7 commits)
2019-10-12 [email protected] Roll src/third_party/skia 8c6c8b0c42e2..3838fe3c82b4 (3 commits) (flutter/engine#13105)
2019-10-12 [email protected] Analyze framework Dart code in presubmit tests (flutter/engine#13037)
2019-10-12 [email protected] [dart_aot_runner] Complete the port of dart_aot_runner (flutter/engine#13103)
2019-10-11 [email protected] Move initialization into FlutterEngine (flutter/engine#12806)
2019-10-11 [email protected] Update felt README (flutter/engine#13097)
2019-10-11 [email protected] Roll src/third_party/dart 9f33e8da04..42dcdf903c (7 commits)
2019-10-11 [email protected] [dart_aot_runner] Generate vmservice aotsnapshots (flutter/engine#13101)
2019-10-11 [email protected] ColorFilter matrix docs (flutter/engine#13100)
2019-10-11 [email protected] cleanup gen_package.py (flutter/engine#13089)
2019-10-11 [email protected] [dart_aot_runner] Use the host_toolchain to build kernels (flutter/engine#13096)
2019-10-11 [email protected] do not wrap font family name (flutter/engine#12801)
2019-10-11 [email protected] Roll src/third_party/skia df640e6d14a5..8c6c8b0c42e2 (12 commits) (flutter/engine#13098)
2019-10-11 [email protected] Remove persistent cache unittest timeout (flutter/engine#13091)
2019-10-11 [email protected] Added FlutterActivity and FlutterFragment hook to cleanUpFlutterEngine() as symmetry for configureFlutterEngine(). (flutter#41943) (flutter/engine#12987)
2019-10-11 [email protected] use rest args for specifying test targets (flutter/engine#13088)
2019-10-11 [email protected] Snapshot the felt tool for faster start-up (flutter/engine#13090)
2019-10-11 [email protected] Roll src/third_party/dart 965b8cb1d8..9f33e8da04 (15 commits)
2019-10-11 [email protected] java imports/style (flutter/engine#13082)
2019-10-11 [email protected] Removed retain cycle from notification center. (flutter/engine#13073)
2019-10-11 [email protected] Roll fuchsia/sdk/core/mac-amd64 from U4IBp... to ShVbh... (flutter/engine#13092)
2019-10-11 [email protected] Roll fuchsia/sdk/core/linux-amd64 from fk6ou... to LsxeL... (flutter/engine#13083)
2019-10-11 [email protected] Roll src/third_party/skia 6c2b2bb02402..df640e6d14a5 (1 commits) (flutter/engine#13080)
2019-10-11 [email protected] Gen package output corrected (flutter/engine#13086)
2019-10-11 [email protected] Print more output when gen_package fails (flutter/engine#13085)

...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jmagman jmagman jmagman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gaaclarke @jmagman @googlebot