Recover from incorrect APZ hit testing when dragging scrollbar thumb

RESOLVED FIXED in Firefox 60

Status

()

defect
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: botond, Assigned: botond)

Tracking

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments)

Assignee

Description

Last year
From time to time we have bugs in APZ hit testing code.

In the case of scrollbar dragging, such bugs can currently manifest as not being able to drag a scrollbar thumb at all, or dragging a scrollbar thumb causing a different scrollable element (commonly the parent) to be scrolled.

Recent examples where hit testing bugs have had this effect include bug 1429373 and bug 1434846. In the case of bug 1429373, the regression was shipped in several Firefox releases before it was noticed and fixed.

We could handle such situations more gracefully: since the main thread also performs a hit test (which is usually not buggy) and tells APZ about it, in cases where it gives a different result from APZ, APZ could just accept that it was wrong and use the main thread's hit test result. The resulting behaviour would basically be correct, except that drag events processed before APZ receives the main thread's notification would be dropped.

To avoid papering over the underlying APZ hit testing bugs, such inconsistencies could trigger a MOZ_DIAGNOSTIC_ASSERT, so that the issues are still noticed on Nightly, but in release builds we recover from the bug gracefully.
Assignee

Updated

Last year
See Also: → 1429373, 1434846
Comment hidden (mozreview-request)
Assignee

Comment 2

Last year
This patch doesn't limit the "recovery" codepath to *scrollbar* drags like we discussed on IRC, but it does limit it to drags in general. With some extra effort, we could limit it to scrollbar drags only.
Comment on attachment 8950424 [details]
Bug 1437694 - Gracefully recover from hit testing bugs affecting scrollbar dragging.

https://reviewboard.mozilla.org/r/219666/#review225596

Looks good, thanks!
Attachment #8950424 - Flags: review?(bugmail) → review+
(In reply to Botond Ballo [:botond] from comment #2)
> This patch doesn't limit the "recovery" codepath to *scrollbar* drags like
> we discussed on IRC, but it does limit it to drags in general. With some
> extra effort, we could limit it to scrollbar drags only.

I'm fine with the patch as-is. The diagnostic assert should help us catch any unexpected behaviour that might result.

Comment 5

Last year
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eedbb054f2ae
Gracefully recover from hit testing bugs affecting scrollbar dragging. r=kats
Backed out changeset eedbb054f2ae (bug 1437694) for failing Browser chrome on browser/components/search/test/browser_searchbar_openpopup.js

Log of the fail:
https://treeherder.mozilla.org/logviewer.html#?job_id=162335480&repo=autoland&lineNumber=5316

Backout changeset:
https://hg.mozilla.org/integration/autoland/rev/9c6aa0a5ec8d084a499eb3ff85f676ee29476769

Push that got backed out:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=eedbb054f2ae340e53723235b437598b10782619
Flags: needinfo?(botond)
Assignee

Comment 7

Last year
APZ is hitting scrollId=5, the main thread is hitting scrollId=3.

I note that the layer with scrollId=5 has an empty visible region but a nonempty hit region. Not sure whether that's expected.
Flags: needinfo?(botond)
(In reply to Botond Ballo [:botond] from comment #7)
> I note that the layer with scrollId=5 has an empty visible region but a
> nonempty hit region. Not sure whether that's expected.

That should be pretty rare. It might happen if the element has opacity:0 but NOT pointer-events:none (which makes it invisible, but still receive input events). However I know miko was making changes to the event regions stuff at around the same time and fixed some regression recently, so it's possible you got a bad base changeset where the event regions stuff had a bug. If you can repro locally I'd try on a newer base cset to see if the problem still happens, and if it does, also try on a cset from a month or ago or so to see if it happened before Miko's changes. It might be as-yet-unfixed regression from his changes too.
Priority: -- → P3
Whiteboard: [gfx-noted]
Assignee

Comment 9

Last year
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> However I know miko was making changes to the event regions stuff
> at around the same time and fixed some regression recently, so it's possible
> you got a bad base changeset where the event regions stuff had a bug. If you
> can repro locally I'd try on a newer base cset to see if the problem still
> happens, and if it does, also try on a cset from a month or ago or so to see
> if it happened before Miko's changes. It might be as-yet-unfixed regression
> from his changes too.

I did a couple of Try pushes [1] [2] to test this theory, but the error occurs both with today's m-c, and m-c as of about a month ago.

I'll try to do a Windows build on Monday to repro and debug locally.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=42570310cf1945afeec8e749e8886db25c2649ff
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=82b67f43d9d4eea0ccd66ae7a7df21b512468153
Assignee

Comment 10

Last year
(In reply to Botond Ballo [:botond] from comment #9)
> I'll try to do a Windows build on Monday to repro and debug locally.

I did a local Windows build, but I can't repro the issue there for some reason.
Comment hidden (mozreview-request)
Assignee

Comment 12

Last year
Comment on attachment 8950424 [details]
Bug 1437694 - Gracefully recover from hit testing bugs affecting scrollbar dragging.

Rather than sinking more time into investigating a failure I can't reproduce locally, I decided to change tacks and avoid the issue by restricting the change to only affecting scrollbar drags, as opposed to drags in general, as originally suggested (the failing test is not doing a scrollbar drag). This ended up being more straightforward than I thought.
Attachment #8950424 - Flags: review+ → review?(bugmail)
Comment on attachment 8950424 [details]
Bug 1437694 - Gracefully recover from hit testing bugs affecting scrollbar dragging.

https://reviewboard.mozilla.org/r/219666/#review230962

::: gfx/layers/apz/src/InputBlockState.cpp:399
(Diff revision 2)
>    if (apzc && aFirstInput) {
>      apzc = apzc->BuildOverscrollHandoffChain()->FindFirstScrollable(
>          *aFirstInput, &mAllowedScrollDirections);
>    }
>  
>    InputBlockState::SetConfirmedTargetApzc(apzc, aState, aFirstInput);

Even if we know aForScrollbarDrag here will always be false, it would be good to pass the argument to the parent InputBlockState::SetConfirmedTargetApzc function here. Ditto in PanGestureBlockState.

Actually given how few call sites there are of SetConfirmedTargetApzc, I'd prefer just making the arg non-optional and explicitly specifying it at all the call sites.
Attachment #8950424 - Flags: review?(bugmail) → review+
Comment hidden (mozreview-request)
Assignee

Comment 15

Last year
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> Even if we know aForScrollbarDrag here will always be false, it would be
> good to pass the argument to the parent
> InputBlockState::SetConfirmedTargetApzc function here. Ditto in
> PanGestureBlockState.
> 
> Actually given how few call sites there are of SetConfirmedTargetApzc, I'd
> prefer just making the arg non-optional and explicitly specifying it at all
> the call sites.

Done.

Comment 16

Last year
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0b2aa2dc564
Gracefully recover from hit testing bugs affecting scrollbar dragging. r=kats
https://hg.mozilla.org/mozilla-central/rev/c0b2aa2dc564
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1443518

Updated

Last year
Depends on: 1447131
Assignee

Updated

Last year
Depends on: 1455860
Assignee

Updated

Last year
Depends on: 1457603
Assignee

Updated

Last year
Depends on: 1459696
You need to log in before you can comment on or make changes to this bug.