[CP-beta][Android] Refactor ImageReaderSurfaceProducer restoration after app resumes
#177121
Conversation
… resumes (flutter#175937) Refactors `FlutterRenderer` callback on app resume to restore its `ImageReaderSurfaceProducer`s to a public API that Flutter activities and fragments can call themselves `onResume` to fix a memory leak caused by a potentially infinite number of callbacks being created as new instances of the `FlutterEngine` are created. Fixes flutter#173770. Directly follows the @/jason-simmons recommendation in flutter#173770 (comment). ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
There was a problem hiding this comment.
Code Review
This pull request refactors the restoration logic for ImageReaderSurfaceProducer to address a memory leak on Android. The change moves the restoration logic from a ProcessLifecycleOwner observer to a new public method restoreSurfaceProducers() in FlutterRenderer. This new method is then explicitly called from FlutterActivityAndFragmentDelegate.onResume(). This is a good approach to tie the restoration to the activity lifecycle and should resolve the issue. The tests have been updated accordingly to cover the new behavior.
I've found a potential concurrency issue in the new restoreSurfaceProducers method related to iterating over a non-thread-safe collection. Please see my detailed comment.
| for (ImageReaderSurfaceProducer producer : imageReaderProducers) { | ||
| if (producer.callback != null && producer.notifiedDestroy) { | ||
| producer.notifiedDestroy = false; | ||
| producer.callback.onSurfaceAvailable(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop iterates over imageReaderProducers, which is not a thread-safe collection. The list can be modified from other threads. For example, ImageReaderSurfaceProducer.finalize() can call releaseInternal(), which removes an item from the list on a finalizer thread. Since restoreSurfaceProducers() is called on the main thread, this can lead to a ConcurrentModificationException.
To address this, imageReaderProducers should be a thread-safe collection, for example, a CopyOnWriteArrayList. All additions and removals from the list should also be handled in a thread-safe manner.
…n after app resumes (#177112) > [!NOTE] > To release engineer: this can definitely wait to be included in a hotfix beta release. I also want to CP this to 3.38 beta as part of a hotfix release: #177121 This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request) Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request. ### Issue Link: What is the link to the issue this cherry-pick is addressing? #173770 ### Changelog Description: Explain this cherry pick in one line that is accessible to most Flutter developers. See [best practices](https://github.com/flutter/flutter/blob/main/docs/releases/Hotfix-Documentation-Best-Practices.md) for examples Mitigates a memory leak that occurs on Android, when `Activities` are not kept upon exit and an Activity is exited and re-entered. ### Impact Description: What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping production app (the app crashes on launch) Memory leak that will occur in production app but does not impact development. ### Workaround: Is there a workaround for this issue? Not that I know of. ### Risk: What is the risk level of this cherry-pick? ### Test Coverage: Are you confident that your fix is well-tested by automated tests? ### Validation Steps: What are the steps to validate that this fix works? Follow the steps in the [issue description](#173770 (comment)), roughly: 1. Create a Flutter project. 2. Enter the developer options, set not to keep activities, (destroy each activity as soon as the user leaves it) 3. Entering the activity, clicking the home button, exiting the activity, and then entering the activity again will trigger the destruction and reconstruction of the activity. 4. Check memory leaks through profiler and find memory leaks.
53f2819
into
flutter:flutter-3.38-candidate.0
…storation after app resumes (flutter/flutter#177121)
Note
To release engineer: this can definitely wait to be included in a hotfix beta release. I also want to CP this to stable: #177112
Issue Link:
What is the link to the issue this cherry-pick is addressing?
#173770
Changelog Description:
Explain this cherry pick in one line that is accessible to most Flutter developers. See best practices for examples
Mitigates a memory leak that occurs on Android, when
Activitiesare not kept upon exit and an Activity is exited and re-entered.Impact Description:
What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping production app (the app crashes on launch)
Memory leak that will occur in production app but does not impact development.
Workaround:
Is there a workaround for this issue?
Not that I know of.
Risk:
What is the risk level of this cherry-pick?
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
Validation Steps:
What are the steps to validate that this fix works?
Follow the steps in the issue description, roughly:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.