paint: Do not unwrap in stop_fling_if_needed to fix pinch zoom panic#42350
paint: Do not unwrap in stop_fling_if_needed to fix pinch zoom panic#42350yezhizhen merged 1 commit intoservo:mainfrom
stop_fling_if_needed to fix pinch zoom panic#42350Conversation
Signed-off-by: Euclid Ye <[email protected]>
bb14fc5 to
1126374
Compare
|
🔨 Triggering try run (#21697999343) for Android |
|
✨ Try run (#21697999343) succeeded. |
|
Tested on Android too. It also had the panic, which is gone with this PR. |
TimvdLippe
left a comment
There was a problem hiding this comment.
Can you add the test from the issue as WPT test?
|
@TimvdLippe Thanks. I wanted to add a crash test, but it seems pinch zoom cannot be triggered in headless window somehow: #42320 (comment) That would require investigation. |
|
🔨 Triggering try run (#21701260800) for Linux (WPT) |
|
Test results for linux-wpt from try job (#21701260800): Flaky unexpected result (30)
Stable unexpected results that are known to be intermittent (32)
|
|
✨ Try run (#21701260800) succeeded. |
|
I wonder how difficult it would be to add some unit-tests for the touch handler (with other components like script mocked). There's been quite a few issues with crashes due to wrong assumptions, so it might be worth to invest some time in adding comprehensive testing of this module. Not something in the scope of this PR, but perhaps something to think about for the future ... |
jschwe
left a comment
There was a problem hiding this comment.
I would appreciate the title being slightly more descriptive, e.g. Fix crash with multiple concucrrent touchups, which seems to be what caused the issue?
We have a roadmap to support all WPT touch tests coverage, which is also how the issue is found as we made progress. (As WebDriver operates much faster than human)
Once we solve this newly found issue, we will have more test coverage. |
stop_fling_if_needed due to race condition
|
I will remake the title. After walking through the flow again, this is actually not race condition, but a deterministic behaviour. Things here happen on the same thread. |
stop_fling_if_needed due to race conditionstop_fling_if_needed
stop_fling_if_neededstop_fling_if_needed to fix Pinch Zoom panic
stop_fling_if_needed to fix Pinch Zoom panicstop_fling_if_needed to fix pinch zoom panic
Touch sequence may have already been removed before stopping potential fling during Paint update.
This can happen when we touch up both fingers quickly after the touch move,
before updates are performed in Paint.
We should not play with fire and
unwrap.Testing: Tested with Android, ohos and WebDriver. I can no longer replicate the panic with Pinch Zoom.
Fixes: #42320, which is very verbose but provides the entire story.