[CP-beta]Ensure content-sizing resize is run on UI thread#181732
Conversation
While testing with a different add-to-app flutter/samples#2787, an exception is sometimes thrown when the resize is attempted because the callback comes from a different thread. It is expected that the raster thread calls this but with previous [testing](https://github.com/mboetger/test-add-to-app/tree/content-sizing) never caused this exception. This PR ensures the resize happens on the UI thread. It also a flag to ensure content sizing is disabled. Fixes: flutter#181573 ## 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.
|
@mboetger please fill out the PR description above, afterwards the release team will review this request. |
|
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. |
github-actions
bot
added
platform-android
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a threading issue by ensuring that a JNI call for resizing a surface view is executed on the UI thread. This is achieved by posting the call as a task to the platform task runner in C++. The changes also introduce a new feature flag, io.flutter.embedding.android.EnableContentSizing, to conditionally enable content sizing logic in various Android views. The unit tests have been updated to accommodate both the asynchronous nature of the fix and the new feature flag. The changes appear to be correct and consistently applied.
| } catch (NameNotFoundException ex) { | ||
| Log.e(TAG, "Could not get metadata"); | ||
| } |
There was a problem hiding this comment.
For better diagnostics, it's recommended to include the exception object in the log call. This will print the stack trace and provide more context when a NameNotFoundException occurs, making it easier to debug.
| } catch (NameNotFoundException ex) { | |
| Log.e(TAG, "Could not get metadata"); | |
| } | |
| } catch (NameNotFoundException e) { | |
| Log.e(TAG, "Could not get metadata", e); | |
| } |
References
- As per the style guide (line 32), error messages should be useful. Including the exception in the log provides valuable context like the stack trace, which is crucial for debugging. (link)
There was a problem hiding this comment.
This is a good suggestion but should be made on master.
|
Approved as the android tl, Still needs approval from a release engineer (who is not me) @mboetger can I get you to do a second pass on your changelog description. "[flutter/181573] When resize JNI method is called on Android, apps will crash." What condition causes the resize jni method to be called? |
mboetger
added
the
autosubmit
auto-submit
bot
merged commit 55bea55
into
flutter:flutter-3.41-candidate.0
4 participants
This pull request is created by automatic cherry pick workflow
Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request.
Cherry-pick of #181686
Issue Link:
What is the link to the issue this cherry-pick is addressing?
#181573
Impact Description:
What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)?
Crash
Does it impact development (ex. flutter doctor crashes when Android Studio is installed),
or the shipping of production apps (the app crashes on launch).
App crashes
This information is for domain experts and release engineers to understand the consequences of saying yes or no to the cherry pick.
Apps running this code will crash: https://buganizer.corp.google.com/issues/478788744
Changelog Description:
Explain this cherry pick:
See best practices for examples.
Android Apps
< Replace with changelog description here >
[flutter/181573] When resize JNI method is called on Android, apps will crash.
Workaround:
Is there a workaround for this issue?
No.
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?
Run the sample app flutter/samples#2787 and check it does not crash.