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

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

Development

Successfully merging this pull request may close these issues.

testInteractiveKeyboardDidResignFirstResponderDelegateisCalledAfterDismissedKeyboard engine test failing on Xcode 15 in CI

2 participants