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.

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

Fix iOS unittests leak in shared.invoke method channel that causes crash #45416

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented Sep 2, 2023

Fixes flutter/flutter#133776

"The crash is due to testShareScreenInvoked presents the activityViewController, which prevents the engineViewController to be dealloc'd in the test: https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Source/FlutterPlatformPlugin.mm#L161C4-L161C24"

This fix stubbing the presentViewController call, which prevents the viewController being alloc'd, thus preventing the leak. It is probably sufficient for this test.

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 September 2, 2023 18:34
@LouiseHsu
Copy link
Contributor

I didn't realize this was something I should watch out for since all the tests passed locally/on github 😬... I will keep this in mind in the future lol. Thank you so much for fixing this!

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Sep 5, 2023

I didn't realize this was something I should watch out for since all the tests passed locally/on github 😬... I will keep this in mind in the future lol. Thank you so much for fixing this!

This is a tricky one and very hard to detect. It could def be improved after we migrate to ARC.

@cyanglaz cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 5, 2023
@auto-submit auto-submit bot merged commit 5b2cc9d into flutter:main Sep 5, 2023
25 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 5, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 5, 2023
…134080)

flutter/engine@98b036a...5b2cc9d

2023-09-05 ychris@google.com Fix iOS unittests leak in shared.invoke method channel that causes crash (flutter/engine#45416)
2023-09-05 skia-flutter-autoroll@skia.org Roll Skia from 7e80aedd05b6 to 2b9fc6a2c250 (1 revision) (flutter/engine#45465)

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 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-ios
Projects
None yet
2 participants