1437694 - Recover from incorrect APZ hit testing when dragging scrollbar thumb

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Botond Ballo [:botond]]

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.

Comment 1

[Botond Ballo [:botond]]

Attachment: Bug 1437694 - Gracefully recover from hit testing bugs affecting scrollbar dragging.

The recovery is only enabled for release builds, to avoid papering over
such hit testing bugs. On nightly builds, a diagnostic assert is issued.

Review commit: https://reviewboard.mozilla.org/r/219666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219666/

Comment 2

[Botond Ballo [:botond]]

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 3

[Kartikaya Gupta (email:[e-mail])]

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!

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eedbb054f2ae
Gracefully recover from hit testing bugs affecting scrollbar dragging. r=kats

Comment 6

[Dorel Luca [:dluca]]

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

Comment 7

[Botond Ballo [:botond]]

Attachment: Layer dump for assertion failure in browser_searchbar_openpopup.js

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.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 9

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

Comment 10

[Botond Ballo [:botond]]

(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 11

[Botond Ballo [:botond]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219666/diff/1-2/

Comment 12

[Botond Ballo [:botond]]

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.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 14

[Botond Ballo [:botond]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219666/diff/2-3/

Comment 15

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0b2aa2dc564
Gracefully recover from hit testing bugs affecting scrollbar dragging. r=kats

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c0b2aa2dc564