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

[ios] introduce weak_nsobject #47947

Merged
merged 8 commits into from
Nov 15, 2023
Merged

[ios] introduce weak_nsobject #47947

merged 8 commits into from
Nov 15, 2023

Conversation

cyanglaz
Copy link
Contributor

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

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

@cyanglaz cyanglaz marked this pull request as ready for review November 11, 2023 04:27
@cyanglaz cyanglaz marked this pull request as draft November 13, 2023 18:26
@cyanglaz
Copy link
Contributor Author

I'm going to migrate all the weakPtr to weakNSObject in this PR, convert to draft until that;s done

stuartmorgan
stuartmorgan requested changes Nov 13, 2023
View reviewed changes
fml/platform/darwin/weak_nsobject.h Outdated
~WeakContainer() {}

std::shared_ptr<debug::DebugThreadChecker> checker_;
id object_;
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look ARC-safe to me; this is a strong reference under ARC unless I'm misremembering the semantics of id in ARC. I was confused by this so I looked at the history, and although:

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

that doesn't appear to have been an ARC-safe version. See https://codereview.chromium.org/2943263003 which landed much later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. I have adopted the change and updated the NSWeakObjectFactory accordingly.
I have also added a new unittests to test the NSWeakObject under ARC.
Also migrated a XCTest file that contains NSWeakObject to ARC.

@cyanglaz cyanglaz marked this pull request as ready for review November 14, 2023 20:57
@cyanglaz cyanglaz force-pushed the weak_nsobject branch 3 times, most recently from d5a4eae to f738bfd Compare November 15, 2023 14:44
stuartmorgan
stuartmorgan approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM

fml/platform/darwin/weak_nsobject.mm Outdated
}

- (void)dealloc {
self.container->nullify();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: container_; dealloc shouldn't use self.

@cyanglaz cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 15, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 15, 2023
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Nov 15, 2023

auto label is removed for flutter/engine/47947, due to - The status or check suite Linux Framework Smoke Tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@cyanglaz cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 15, 2023
@auto-submit auto-submit bot merged commit b29c564 into flutter:main Nov 15, 2023
26 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 16, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 16, 2023
@engine-flutter-autoroll
b29c56499 [ios] introduce weak_nsobject (flutter/engine#47947)
f4a4373
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 16, 2023
@engine-flutter-autoroll
Roll Flutter Engine from 3cfcdebe8623 to 8aff9c134b8f (18 revisions) (#…
396cd71 
…138529)

flutter/engine@3cfcdeb...8aff9c1

2023-11-16 skia-flutter-autoroll@skia.org Roll Dart SDK from cc6acfd7d57c to 5cccc24d127f (1 revision) (flutter/engine#48109)
2023-11-16 skia-flutter-autoroll@skia.org Roll Skia from 5d6bdbf69dea to add865f891c8 (1 revision) (flutter/engine#48108)
2023-11-16 skia-flutter-autoroll@skia.org Roll Dart SDK from 65819963fb17 to cc6acfd7d57c (5 revisions) (flutter/engine#48100)
2023-11-16 matanlurey@users.noreply.github.com Make `fml/status_or.h` compatible with `.clang_tidy`. (flutter/engine#48002)
2023-11-16 dnfield@google.com [Impeller] Gate Vulkan selection on API 29 (flutter/engine#48089)
2023-11-16 chris@bracken.jp [macOS] Clean up allocations in menu plugin test (flutter/engine#48093)
2023-11-16 matanlurey@users.noreply.github.com Re-land "Make `fml/...` compatible with `.clang_tidy` (flutter/engine#48030)
2023-11-15 ychris@google.com [ios] introduce weak_nsobject (flutter/engine#47947)
2023-11-15 skia-flutter-autoroll@skia.org Roll Skia from e954d1a1972c to 5d6bdbf69dea (2 revisions) (flutter/engine#48094)
2023-11-15 jonahwilliams@google.com [Impeller] add async command submission for blit pass. (flutter/engine#48040)
2023-11-15 matanlurey@users.noreply.github.com Make `lib/ui/compositing/...` compatible with `.clang_tidy`. (flutter/engine#48001)
2023-11-15 godofredoc@google.com Remove the linux fuchsia v1 build. (flutter/engine#48085)
2023-11-15 mdebbar@google.com [web] Apply global styles before inserting the DOM element (flutter/engine#48027)
2023-11-15 skia-flutter-autoroll@skia.org Roll Skia from b23074a79bda to e954d1a1972c (7 revisions) (flutter/engine#48092)
2023-11-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Make `lib/ui/{text|window}/...` compatible with `.clang_tidy`." (flutter/engine#48083)
2023-11-15 41930132+hellohuanlin@users.noreply.github.com [ios]fix ios 16 auto correction highlight showing on top left corner (flutter/engine#47279)
2023-11-15 skia-flutter-autoroll@skia.org Roll Skia from c42226314a4f to b23074a79bda (3 revisions) (flutter/engine#48081)
2023-11-15 matanlurey@users.noreply.github.com Make `lib/ui/{text|window}/...` compatible with `.clang_tidy`. (flutter/engine#48000)

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 jonahwilliams@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
@cyanglaz cyanglaz deleted the weak_nsobject branch November 16, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartmorgan stuartmorgan stuartmorgan approved these changes

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-ios
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cyanglaz @stuartmorgan