Fix status mismatch on multiple overscrolls [chromium/src : main]
Fuhsin Liao (Gerrit)
Fuhsin Liao voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fuhsin Liao
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
Hi I will be on a long weekend and will be back on Monday(June 22nd)
I will be unable to modify the code, but perhaps can reply to questions and messages. We can upload this first if the crash is urgent; We can also wait until I'm back for more discussion. Depends on the urgency~
Related details
- Grace Cham
- Jonathan Ross
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jonathan Ross
| Code-Review | +1 |
3 comments
LGTM with nit and forward looking question. I'll be on leave Jun 20-Aug 31 so won't be available for follow ups
We could do the following to fix the crash: 1. Clear the
active_action_'s state when the status is set to kDisabled 2. Add a ifShould we do this anyways for correctness? Should we consider having `active_action` and `status` in a struct if they are tightly coupled? To make it clear to update them together in the future
if (scroll_consumption_state_ !=
ScrollConsumptionState::kAwaitingScrollUpdateAck) {
return;
}This could just be able the `MaybeDisableScrollConsumption` call
Related details
- Fuhsin Liao
- Grace Cham
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fuhsin Liao (Gerrit)
Fuhsin Liao voted and added 2 comments![]( "Open in Gerrit")
Votes added by Fuhsin Liao
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
We could do the following to fix the crash: 1. Clear the
active_action_'s state when the status is set to kDisabled 2. Add a ifShould we do this anyways for correctness? Should we consider having `active_action` and `status` in a struct if they are tightly coupled? To make it clear to update them together in the future
Hi I just realized there's a logic mistake in my previous patch. Please checked the latest code and commit description. Sorry for the trouble!
Yes we can consider putting active_action and status together~ Unfortunately I need to get offline now :( Will fix this in either this cl or the next(Depends on the urgency)
if (scroll_consumption_state_ !=
ScrollConsumptionState::kAwaitingScrollUpdateAck) {
return;
}This could just be able the `MaybeDisableScrollConsumption` call
Same as aboved. I've changed the code due to my logic mistake in the previous patch, please check the latest patch & description, thanks~!
Related details
- Grace Cham
- Jonathan Ross
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jonathan Ross
| Code-Review | +1 |
- Fuhsin Liao
- Grace Cham
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Grace Cham (Gerrit)
Grace Cham added 3 comments![]( "Open in Gerrit")
Fix status mismatch on multiple overscrolls
This cl is to fix the crash at http://crash/bcba3320a4b640b2
The reason for the crash: In my previous cl, crrev.com/c/7931727,
overscroll_refresh.cc's line 77, the newly added var. active_action_ and
existing var. ScrollConsumptionState are coupled together. Basically
line 77 is saying active_action_ will only be not null if
ScrollConsumptionState is kEnabled. However, I omit a situation where
kDisabled and active_action_.has_value() can coexist. That is when a
user performs a valid overscroll for the first time, the status will be
set to kEnabled and active_action_.has_value(). Before releasing
fingers, if a user performs another invalid overscroll, then the status
will be set to kDisabled in MaybeDisableScrollConsumption() function,
while the active_action_ is left uncleared, and thus causing the crash
in line 77.
The current solution is to clear the active_action_ state when the
status is set to kDisabled. Since the whole
MaybeDisableScrollConsumption() function only checks for vertical
scroll, I add a if(action == kHistoryNavigation) return at the beginning
of the function, to avoid affecting horizontal scrolls' result.
With the current fix, the original logic is perfectly kept, and didn't
affect vertical/horizontal scrolls on touchpad nor on touchscreen.
I think the commit message currently is so verbose that makes it hard to understand what it is trying dto do, especially for future readers who only see this commit and not aware of the two prior CLs.
It will be more helpful to only include: what is wrong, and what was done to fix it. Maybe something like
```
Unset active action when disable on-going vertical overscroll
crrev.com/c/7931727 introduced a CHECK in OnScrollEnd() to make sure state management is accurate that there should be no active action if ScrollConsumptionState is not enabled.
The CHECK fails because MaybeDisableScrollConsumption() could set consumption state to disabled while there's an on-going overscroll.
```
Have you checked if there are other instances where only active_action_ is changed, or only scroll_consumption_state_ is changed, to make sure the CHECK won't crash for any other cases?
Agree with Jon that we should just couple the two if possible to avoid this happening in the future.
Test: Wrote temporary unit tests to successfully reproduce the crash, and use the same test to test against this cl, passed.Should we just keep the unit test?
if (active_action_.has_value() &&
active_action_->action == OverscrollAction::kHistoryNavigation) {The new comment below says the update is for vertical overscroll only, could it also early return here if there's no active_action_ then?
Stepping back, why do we need this new condition? Commit message says
```
Since the whole
MaybeDisableScrollConsumption() function only checks for vertical
scroll, I add a if(action == kHistoryNavigation) return at the beginning
of the function, to avoid affecting horizontal scrolls' result.
```
Digging the history a bit, this function `MaybeDisableScrollConsumption` was originally added to guard the case that an intermediate scroll update is against the action indicated by their start position (abort effect), for vertical scrolls specifically. See the diff in crrev.com/c/1249530.
At that time, the function is only called by `WillHandleScrollUpdate` when still `kAwaitingScrollUpdateAck`. `WillHandleScrollUpdate` basically splits a scroll sequence into two parts: (1) before overscroll, it might call change consumption state to disable (by `MaybeDisableScrollConsumption`) and not handle further scroll updates at all, (2) after overscroll, consumes scroll deltas and pass to java.
crrev.com/c/6352218 introduced `MaybeDisableScrollConsumption` call in `OnOverscrolled` but IIUC that only happens once in a scroll sequence when it first crosses the overscroll threshold, so it could conceptually be seen as (1) above.
I feel like `MaybeDisableScrollConsumption` implicitly assumed that there is no on going overscroll active action (hence the `is_pull_to_refresh` and `is_pull_from_bottom_edge` is a guess of what user was trying to do if they eventually overscrolls, instead of checking the OverscrollAction type/ edge which will only be set when they overscroll).
```
when a user performs a valid overscroll for the first time, the status will be
set to kEnabled and active_action_.has_value(). Before releasing
fingers, if a user performs another invalid overscroll, then the status
will be set to kDisabled in MaybeDisableScrollConsumption() function,
while the active_action_ is left uncleared, and thus causing the crash
in line 77.
```
you mentioned this in your commit message, how does user performs another invalid overscroll?
Related details
- Fuhsin Liao
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fuhsin Liao (Gerrit)
Fuhsin Liao voted and added 3 comments![]( "Open in Gerrit")
Votes added by Fuhsin Liao
| Auto-Submit | +0 |
This cl is to fix the crash at http://crash/bcba3320a4b640b2
The reason for the crash: In my previous cl, crrev.com/c/7931727,
overscroll_refresh.cc's line 77, the newly added var. active_action_ and
existing var. ScrollConsumptionState are coupled together. Basically
line 77 is saying active_action_ will only be not null if
ScrollConsumptionState is kEnabled. However, I omit a situation where
kDisabled and active_action_.has_value() can coexist. That is when a
user performs a valid overscroll for the first time, the status will be
set to kEnabled and active_action_.has_value(). Before releasing
fingers, if a user performs another invalid overscroll, then the status
will be set to kDisabled in MaybeDisableScrollConsumption() function,
while the active_action_ is left uncleared, and thus causing the crash
in line 77.
The current solution is to clear the active_action_ state when the
status is set to kDisabled. Since the whole
MaybeDisableScrollConsumption() function only checks for vertical
scroll, I add a if(action == kHistoryNavigation) return at the beginning
of the function, to avoid affecting horizontal scrolls' result.
With the current fix, the original logic is perfectly kept, and didn't
affect vertical/horizontal scrolls on touchpad nor on touchscreen.
I think the commit message currently is so verbose that makes it hard to understand what it is trying dto do, especially for future readers who only see this commit and not aware of the two prior CLs.
It will be more helpful to only include: what is wrong, and what was done to fix it. Maybe something like```
Unset active action when disable on-going vertical overscrollcrrev.com/c/7931727 introduced a CHECK in OnScrollEnd() to make sure state management is accurate that there should be no active action if ScrollConsumptionState is not enabled.
The CHECK fails because MaybeDisableScrollConsumption() could set consumption state to disabled while there's an on-going overscroll.
```Have you checked if there are other instances where only active_action_ is changed, or only scroll_consumption_state_ is changed, to make sure the CHECK won't crash for any other cases?
Agree with Jon that we should just couple the two if possible to avoid this happening in the future.
Yes I've checked both by myself and with AI that, this crash will very likely happen in this case~
If there're other cases coming after(tho can't think of any now), we can still fix them then!
Changed the cl description and merge 'scroll_consumption_state_' and 'active_action_' into a struct
Test: Wrote temporary unit tests to successfully reproduce the crash, and use the same test to test against this cl, passed.Should we just keep the unit test?
Done
if (active_action_.has_value() &&
active_action_->action == OverscrollAction::kHistoryNavigation) {The new comment below says the update is for vertical overscroll only, could it also early return here if there's no active_action_ then?
Stepping back, why do we need this new condition? Commit message says
```
Since the whole
MaybeDisableScrollConsumption() function only checks for vertical
scroll, I add a if(action == kHistoryNavigation) return at the beginning
of the function, to avoid affecting horizontal scrolls' result.
```Digging the history a bit, this function `MaybeDisableScrollConsumption` was originally added to guard the case that an intermediate scroll update is against the action indicated by their start position (abort effect), for vertical scrolls specifically. See the diff in crrev.com/c/1249530.
At that time, the function is only called by `WillHandleScrollUpdate` when still `kAwaitingScrollUpdateAck`. `WillHandleScrollUpdate` basically splits a scroll sequence into two parts: (1) before overscroll, it might call change consumption state to disable (by `MaybeDisableScrollConsumption`) and not handle further scroll updates at all, (2) after overscroll, consumes scroll deltas and pass to java.
crrev.com/c/6352218 introduced `MaybeDisableScrollConsumption` call in `OnOverscrolled` but IIUC that only happens once in a scroll sequence when it first crosses the overscroll threshold, so it could conceptually be seen as (1) above.I feel like `MaybeDisableScrollConsumption` implicitly assumed that there is no on going overscroll active action (hence the `is_pull_to_refresh` and `is_pull_from_bottom_edge` is a guess of what user was trying to do if they eventually overscrolls, instead of checking the OverscrollAction type/ edge which will only be set when they overscroll).
```
when a user performs a valid overscroll for the first time, the status will be
set to kEnabled and active_action_.has_value(). Before releasing
fingers, if a user performs another invalid overscroll, then the status
will be set to kDisabled in MaybeDisableScrollConsumption() function,
while the active_action_ is left uncleared, and thus causing the crash
in line 77.
```
you mentioned this in your commit message, how does user performs another invalid overscroll?
Actually OnOverscrolled will be called many times even after "it first crosses the overscroll threshold". I think OnOverscrolled will be called only once is the case before you fixed at crrev.com/c/7867154. Now whenever user overscrolls, before users lifting their fingers, the overscroll() function will be continuously called with new accumulated_overscroll values.
Related details
- Grace Cham
- Jonathan Ross
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fuhsin Liao (Gerrit)
Fuhsin Liao added 1 comment![]( "Open in Gerrit")
We could do the following to fix the crash: 1. Clear the
active_action_'s state when the status is set to kDisabled 2. Add a ifFuhsin LiaoShould we do this anyways for correctness? Should we consider having `active_action` and `status` in a struct if they are tightly coupled? To make it clear to update them together in the future
Hi I just realized there's a logic mistake in my previous patch. Please checked the latest code and commit description. Sorry for the trouble!
Yes we can consider putting active_action and status together~ Unfortunately I need to get offline now :( Will fix this in either this cl or the next(Depends on the urgency)
Done
Jinsuk Kim (Gerrit)
Jinsuk Kim voted Code-Review+1![]( "Open in Gerrit")
- Fuhsin Liao
- Grace Cham
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Grace Cham (Gerrit)
Grace Cham added 5 comments![]( "Open in Gerrit")
scroll_state_.scroll_consumption_state =
ScrollConsumptionState::kAwaitingScrollUpdateAck;Isn't it defeating the purpose of having ScrollState as a struct if the code could update the internal state independently without going through ScrollState anyways?
I think the properties should be write only through ScrollState methods so action and consumption state are consistent.
Alternatively, wondering if we should use std::variant.
CHECK(!scroll_state_.active_action.has_value());We wouldn't need this CHECK if it's enforced by ScrollState internally
if (scroll_state_.scroll_consumption_state !=
ScrollConsumptionState::kAwaitingScrollUpdateAck) {Not a regression but could probably use OverscrollRefresh::IsAwaitingScrollUpdateAck()?
if (active_action_.has_value() &&
active_action_->action == OverscrollAction::kHistoryNavigation) {Fuhsin LiaoThe new comment below says the update is for vertical overscroll only, could it also early return here if there's no active_action_ then?
Stepping back, why do we need this new condition? Commit message says
```
Since the whole
MaybeDisableScrollConsumption() function only checks for vertical
scroll, I add a if(action == kHistoryNavigation) return at the beginning
of the function, to avoid affecting horizontal scrolls' result.
```Digging the history a bit, this function `MaybeDisableScrollConsumption` was originally added to guard the case that an intermediate scroll update is against the action indicated by their start position (abort effect), for vertical scrolls specifically. See the diff in crrev.com/c/1249530.
At that time, the function is only called by `WillHandleScrollUpdate` when still `kAwaitingScrollUpdateAck`. `WillHandleScrollUpdate` basically splits a scroll sequence into two parts: (1) before overscroll, it might call change consumption state to disable (by `MaybeDisableScrollConsumption`) and not handle further scroll updates at all, (2) after overscroll, consumes scroll deltas and pass to java.
crrev.com/c/6352218 introduced `MaybeDisableScrollConsumption` call in `OnOverscrolled` but IIUC that only happens once in a scroll sequence when it first crosses the overscroll threshold, so it could conceptually be seen as (1) above.I feel like `MaybeDisableScrollConsumption` implicitly assumed that there is no on going overscroll active action (hence the `is_pull_to_refresh` and `is_pull_from_bottom_edge` is a guess of what user was trying to do if they eventually overscrolls, instead of checking the OverscrollAction type/ edge which will only be set when they overscroll).
```
when a user performs a valid overscroll for the first time, the status will be
set to kEnabled and active_action_.has_value(). Before releasing
fingers, if a user performs another invalid overscroll, then the status
will be set to kDisabled in MaybeDisableScrollConsumption() function,
while the active_action_ is left uncleared, and thus causing the crash
in line 77.
```
you mentioned this in your commit message, how does user performs another invalid overscroll?
Actually OnOverscrolled will be called many times even after "it first crosses the overscroll threshold". I think OnOverscrolled will be called only once is the case before you fixed at crrev.com/c/7867154. Now whenever user overscrolls, before users lifting their fingers, the overscroll() function will be continuously called with new accumulated_overscroll values.
I see, so this function is meant to be a recurring check throughout the scroll sequence before and after overscroll occurred. (Don't really see why crrev.com/c/7867154 will affect this though... That is independent to how overscroll works just determination of scroll update vs end events)
IIUC, prior to this new early return condition, the check means if user has started horizontal overscroll, then (while keeping fingers on scrollpad) started going all directions say drawing circles, the horizontal overscroll will be cancelled if the total accumulated y is larger than x, and the y direction (up/ down) is not consistent with the starting y-position (top/ bottom edge).
I agree with you that early returning for horizontal overscroll is correct in terms of user-facing behavior, since the cancellation conditions are very specific to vertical direction, just not sure if we want to include it in this change that's for fixing the state mismatch. No strong opinion.
if (scroll_state_.scroll_consumption_state ==
ScrollConsumptionState::kEnabled) {ditto: maybe use OverscrollRefresh::IsActive()?
Related details
- Fuhsin Liao
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fuhsin Liao (Gerrit)
Fuhsin Liao added 5 comments![]( "Open in Gerrit")
scroll_state_.scroll_consumption_state =
ScrollConsumptionState::kAwaitingScrollUpdateAck;Isn't it defeating the purpose of having ScrollState as a struct if the code could update the internal state independently without going through ScrollState anyways?
I think the properties should be write only through ScrollState methods so action and consumption state are consistent.
Alternatively, wondering if we should use std::variant.
Thanks for the suggestion! Never heard of std::variant before and try to use it for the first time. Please check if the current implementation is what you have in mind~
We wouldn't need this CHECK if it's enforced by ScrollState internally
Done
if (scroll_state_.scroll_consumption_state !=
ScrollConsumptionState::kAwaitingScrollUpdateAck) {Not a regression but could probably use OverscrollRefresh::IsAwaitingScrollUpdateAck()?
Done. Did the same thing for KEnabled too
if (active_action_.has_value() &&
active_action_->action == OverscrollAction::kHistoryNavigation) {Fuhsin LiaoThe new comment below says the update is for vertical overscroll only, could it also early return here if there's no active_action_ then?
Stepping back, why do we need this new condition? Commit message says
```
Since the whole
MaybeDisableScrollConsumption() function only checks for vertical
scroll, I add a if(action == kHistoryNavigation) return at the beginning
of the function, to avoid affecting horizontal scrolls' result.
```Digging the history a bit, this function `MaybeDisableScrollConsumption` was originally added to guard the case that an intermediate scroll update is against the action indicated by their start position (abort effect), for vertical scrolls specifically. See the diff in crrev.com/c/1249530.
At that time, the function is only called by `WillHandleScrollUpdate` when still `kAwaitingScrollUpdateAck`. `WillHandleScrollUpdate` basically splits a scroll sequence into two parts: (1) before overscroll, it might call change consumption state to disable (by `MaybeDisableScrollConsumption`) and not handle further scroll updates at all, (2) after overscroll, consumes scroll deltas and pass to java.
crrev.com/c/6352218 introduced `MaybeDisableScrollConsumption` call in `OnOverscrolled` but IIUC that only happens once in a scroll sequence when it first crosses the overscroll threshold, so it could conceptually be seen as (1) above.I feel like `MaybeDisableScrollConsumption` implicitly assumed that there is no on going overscroll active action (hence the `is_pull_to_refresh` and `is_pull_from_bottom_edge` is a guess of what user was trying to do if they eventually overscrolls, instead of checking the OverscrollAction type/ edge which will only be set when they overscroll).
```
when a user performs a valid overscroll for the first time, the status will be
set to kEnabled and active_action_.has_value(). Before releasing
fingers, if a user performs another invalid overscroll, then the status
will be set to kDisabled in MaybeDisableScrollConsumption() function,
while the active_action_ is left uncleared, and thus causing the crash
in line 77.
```
you mentioned this in your commit message, how does user performs another invalid overscroll?
Grace ChamActually OnOverscrolled will be called many times even after "it first crosses the overscroll threshold". I think OnOverscrolled will be called only once is the case before you fixed at crrev.com/c/7867154. Now whenever user overscrolls, before users lifting their fingers, the overscroll() function will be continuously called with new accumulated_overscroll values.
I see, so this function is meant to be a recurring check throughout the scroll sequence before and after overscroll occurred. (Don't really see why crrev.com/c/7867154 will affect this though... That is independent to how overscroll works just determination of scroll update vs end events)
IIUC, prior to this new early return condition, the check means if user has started horizontal overscroll, then (while keeping fingers on scrollpad) started going all directions say drawing circles, the horizontal overscroll will be cancelled if the total accumulated y is larger than x, and the y direction (up/ down) is not consistent with the starting y-position (top/ bottom edge).
I agree with you that early returning for horizontal overscroll is correct in terms of user-facing behavior, since the cancellation conditions are very specific to vertical direction, just not sure if we want to include it in this change that's for fixing the state mismatch. No strong opinion.
Exactly! I'd like to keep it in this cl so we don't have to review twice if that's okay~!
if (scroll_state_.scroll_consumption_state ==
ScrollConsumptionState::kEnabled) {ditto: maybe use OverscrollRefresh::IsActive()?
- Grace Cham
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Grace Cham (Gerrit)
Grace Cham voted and added 1 comment![]( "Open in Gerrit")
Votes added by Grace Cham
| Code-Review | +1 |
1 comment
Thank you~ (Might want to wait for @jins...@chromium.org to take another look before submitting since PS 6 to 7 changed quite a bit?)
Related details
- Fuhsin Liao
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fuhsin Liao (Gerrit)
Fuhsin Liao added 1 comment![]( "Open in Gerrit")
Hi @jins...@chromium.org, the cl has changed quite a bit since last patch, do you want to take a look again? Thanks~!
Related details
- Jinsuk Kim
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |