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

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
@cyanglaz cyanglaz requested a review from LouiseHsu September 2, 2023 19:31
@cyanglaz cyanglaz force-pushed the xcode_15_unittest_crash branch from 412de09 to 6b4a4d4 Compare September 5, 2023 17:14
@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
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 [email protected] Fix iOS unittests leak in shared.invoke method channel that causes crash (flutter/engine#45416)
2023-09-05 [email protected] 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 [email protected],[email protected] 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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-ios
Projects
None yet
2 participants