[beta] [Impeller] Fix crash trying to check for duplicate vertices in shadow_path code (#180920)#181095
Conversation
…_path code (flutter#180920) The new general convex path shadow code was checking for duplicate vertices without actually checking if the vectors contained any vertices. Thus, <vector>.back() was being called on empty vectors, which is bad. This led to a crash in G3 as their code is being run with a vector implementation that protects against this, but apparently we do not have such a vector implementation in our local building and testing. No tests because this covered by existing test cases and a new FML_DCHECK, however I'm not sure if we have DCHECKs enabled in CI...?
|
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 fixes a crash that occurs when checking for duplicate vertices in the shadow path code. The crash was caused by calling .back() on empty vectors. The fix introduces a check to ensure the vectors are not empty before accessing their last element. The change is correct and effectively resolves the issue. I've added one comment to suggest a more concise way to write the check, which also removes a redundant debug assertion.
| if (index > 0u) { | ||
| FML_DCHECK(!gaussians_.empty() && !vertices_.empty()); | ||
| if (gaussian == gaussians_.back() && vertex == vertices_.back()) { | ||
| return index - 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
While the added check is correct and fixes the crash, it can be written more concisely by combining the conditions and relying on C++'s short-circuit evaluation for the && operator. This avoids the nested if and is a common idiom for this pattern.
The FML_DCHECK is also redundant here. The if (index > 0u) check already ensures the vectors are not empty, especially given the FML_DCHECK(index == gaussians_.size()) on line 1359 and that vertices_ and gaussians_ are kept in sync.
if (index > 0u && gaussian == gaussians_.back() && vertex == vertices_.back()) {
return index - 1;
}|
We do want to have this change in this beta (3.41) branch which will eventually become the stable. |
…vertex-check-crash
|
@reidbaker did this miss the cutoff? I apologize--I lost track of this. Maybe this can get merged for next week's release since yours is already underway. |
|
I reached out to the author this morning and let them know I could not merge it because of failing tests. I tried to land it but yes it has missed the cut for the next release. |
1 similar comment
|
I reached out to the author this morning and let them know I could not merge it because of failing tests. I tried to land it but yes it has missed the cut for the next release. |
The failing tests were already in the tree so I needed to wait for their fix to go in. I've merged to ToT to see if they pass now. |
|
Merging to ToT has passed all tests. This is ready to merge into the next release. |
eca1271
into
flutter:flutter-3.41-candidate.0
…ertices in shadow_path code (#180920) (flutter/flutter#181095)
The new general convex path shadow code was checking for duplicate vertices without actually checking if the vectors contained any vertices. Thus, .back() was being called on empty vectors, which is bad. This led to a crash in G3 as their code is being run with a vector implementation that protects against this, but apparently we do not have such a vector implementation in our local building and testing.
No tests because this covered by existing test cases and a new FML_DCHECK.
Fixes #180827
Impacted users: Any user who uses shadows (pretty much every Flutter app) on a platform that has address sanitization built into its
std::vectorimplementation, if they use Impeller (always used on iOS and the default on Android)Impact Description: Every shadow render operation in Impeller backend will access
back()on an emptystd::vectorwhich will crash immediately if the vector is using sanitization on that platform.Workaround: Potentially look into disabling address sanitization? On platforms that still support Skia, it can be used as a workaround, but that is not an option on iOS
Risk: The fix was obvious and non-controversial.
Test Coverage: Existing tests cover this case really well and there is now an FML_DCHECK (engine assert) to check if we trigger the condition under testing.
Validation: The original issue indicates an app that will trigger the sanitization crash, but only in G3. None of the platforms we support directly through flutter perform this sanitization.
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.