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
flutter-team-archive:masterfrom
gaaclarke:flutter-engine-cycle
Oct 11, 2019
Merged

Removed retain cycle from notification center.#13073
gaaclarke merged 2 commits into
flutter-team-archive:masterfrom
gaaclarke:flutter-engine-cycle

Conversation

@gaaclarke

@gaaclarke gaaclarke commented Oct 11, 2019

Copy link
Copy Markdown
Contributor

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

jmagman commented Oct 11, 2019

Copy link
Copy Markdown
Contributor

@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

@jmagman jmagman left a comment

Copy link
Copy Markdown
Contributor

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

gaaclarke commented Oct 11, 2019

Copy link
Copy Markdown
Contributor Author

@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

@jmagman jmagman left a comment

Copy link
Copy Markdown
Contributor

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!

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

__block FlutterEngine* blockSelf = self;

@jmagman jmagman Oct 11, 2019

Copy link
Copy Markdown
Contributor

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. 🔮

@gaaclarke gaaclarke Oct 11, 2019

Copy link
Copy Markdown
Contributor Author

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-team-archive: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-team-…
32d0599 
…archive/engine#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-team-…
094614d 
…archive/engine#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 
git@github.com:flutter/engine.git/compare/eed171ff3538...66bf00b

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

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gaaclarke @jmagman @googlebot