Closed Bug 1468545 Opened 6 years ago Closed 6 years ago

StartScrollbarDrag message on inactive scrollframe arrives after SetTargetAPZC, so the drag metrics might be too late

Categories

(Core :: Panning and Zooming, enhancement)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(7 files, 1 obsolete file)

Bug 1468545 - Distinguish between upwards- and downwards-propagating information in InputAPZContext.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1468545 - Move early-exit condition earlier to avoid more work, remove unused variable.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1468545 - Ensure the drag metrics gets to APZ before the target APZC for inactive scrollframes.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1468545 - Remove incorrect comment.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1468545 - Add some logging for drag event handling.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1468545 - Treat all helper_* files as support files.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1468545 - Add a test for dragging the scrollbar on an inactive scrollframe.
59 bytes, text/x-review-board-request
botond
: review+
Details 
While writing a test for bug 1326290, I discovered another bug. The scenario is when the user tries to drag the scrollbar of an inactive scrollframe. Because the scrollframe is inactive, we need to layerize it before we can send the drag confirmation to APZ. The mechanism by which we do this is at [1] where if there is a pending layerization, we register a PostRefreshObserver to send the message (at [2]) after the refresh is done.

The problem with this that the PostRefreshObserver is registered *after* the one in [3], and so gets notified afterwards as well. This means the APZCTreeManager gets the SetTargetAPZC message before the AsyncDragMetrics, and will start processing the input events with a scrollthumb viewid of 0, which fails. If the AsyncDragMetrics arrive partway through the input handling (i.e. while the user still has the mouse button down) then it will find the thumb and start dragging the scrollbar properly. But in a testing situation we can easily construct a scenario where the entire drag is processed before the AsyncDragMetrics arrive at the compositor which makes the test fail.

[1] https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/layout/xul/nsSliderFrame.cpp#1142
[2] https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/layout/xul/nsSliderFrame.cpp#1018
[3] https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/dom/ipc/TabChild.cpp#1725
I could come up with only two main approaches to tackle this problem. One approach involves sending the AsyncDragMetrics before or at the same time as the SetTargetAPZC message. However I couldn't get this approach to work because we send the SetTargetAPZC before dispatching the event into the DOM ([1] vs [2]), and the way the code is structured that would be very hard to change.

The other approach involves telling APZ which drag blocks started async scrollbar dragging, and ensuring the APZ code waits for the drag metrics on those blocks before it processes the input events. This was also not as simple as I would have liked, but I got it working, I think. I'm going to clean up the patches a bit and test them some more.

[1] https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/dom/ipc/TabChild.cpp#1725
[2] https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/dom/ipc/TabChild.cpp#1739
Assignee: nobody → bugmail
Summary: StartScrollbarDrag message on inactive scrollframe can race with SetTargetAPZC → StartScrollbarDrag message on inactive scrollframe arrives after SetTargetAPZC, so the drag metrics might be too late
The above try push has a TV assertion failure in an assertion that I added. I did another try push with more logging to shed more light on that, the log is at [1] and seems to indicate that there are scenarios where APZ confirms the drag block, and then the main thread later also confirms the drag block, but with a different mScrollbarDragOffset in the drag metrics. In the case of the helper_bug1346632.html testcase, the APZ-confirmed metrics has mScrollbarDragOffset as 100 and the main thread metrics has mScrollbarDragOffset as 50. I imagine that this is because the main thread computation happens after APZ has processed all the input events and moved the thumb down by 50px. So the assertion I added is basically bogus and we should just keep the APZ metrics in this scenario. I'll update that and push the patches for review.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=183187914&repo=try&lineNumber=1599
Comment on attachment 8985350 [details]
Bug 1468545 - Distinguish between upwards- and downwards-propagating information in InputAPZContext.

https://reviewboard.mozilla.org/r/250972/#review257396

::: commit-message-91db0:7
(Diff revision 1)
> +
> +No functional changes here, but this updates the documentation in
> +InputAPZContext and separates the fields into two categories for easier
> +understanding. This is what I had in mind when I introduced this class
> +but never documented it anywhere, and so the "pending layerization" flag
> +didn't follow the convention that I had in mind. This cleans that up.

Sorry :) I see now in retrospect how that was confusing.
Attachment #8985350 - Flags: review?(botond) → review+
Comment on attachment 8985351 [details]
Bug 1468545 - Move early-exit condition earlier to avoid more work, remove unused variable.

https://reviewboard.mozilla.org/r/250974/#review257402
Attachment #8985351 - Flags: review?(botond) → review+
Comment on attachment 8985355 [details]
Bug 1468545 - Add some logging for drag event handling.

https://reviewboard.mozilla.org/r/250982/#review257538

Thanks, this is useful logging! (I should be better about landing stuff like this, I usually just add one-off log statements locally...)
Attachment #8985355 - Flags: review?(botond) → review+
Comment on attachment 8985356 [details]
Bug 1468545 - Treat all helper_* files as support files.

https://reviewboard.mozilla.org/r/250984/#review257540

Nice, didn't know you could do that!
Attachment #8985356 - Flags: review?(botond) → review+
Comment on attachment 8985357 [details]
Bug 1468545 - Add a test for dragging the scrollbar on an inactive scrollframe.

https://reviewboard.mozilla.org/r/250986/#review257558
Attachment #8985357 - Flags: review?(botond) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> I could come up with only two main approaches to tackle this problem. One
> approach involves sending the AsyncDragMetrics before or at the same time as
> the SetTargetAPZC message. However I couldn't get this approach to work
> because we send the SetTargetAPZC before dispatching the event into the DOM
> ([1] vs [2]), and the way the code is structured that would be very hard to
> change.

On IRC, Botond suggested this approach to accomplishing this:

Maybe<RefreshObserver> obs = SendSetTarget...();
Dispatch();
if (obs) obs->Register();

I implemented it, and it seems to work fine. It's also not as ugly as I thought it would be, although it involves touching the other call sites of SendSetTargetAPZCNotification. And it does avoid the hackery with overloading the preventDefault flag, so that's good.

Try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=383037d76895e1d4b5ab37b0f6b40e97868ee839
Attachment #8985354 - Attachment is obsolete: true
Attachment #8985354 - Flags: review?(botond)
Comment on attachment 8985352 [details]
Bug 1468545 - Ensure the drag metrics gets to APZ before the target APZC for inactive scrollframes.

https://reviewboard.mozilla.org/r/250976/#review257608

Thanks!

::: dom/ipc/TabChild.cpp:1746
(Diff revision 2)
> +  // mouse event triggered a post-refresh AsyncDragMetrics message to be sent
> +  // to APZ (from scrollbar dragging in nsSliderFrame), then that will reach
> +  // APZ before the SetTargetAPZC message. This ensures the drag input block
> +  // gets the drag metrics before handling the input events.
> +  if (postLayerization && postLayerization->Register()) {
> +    Unused << postLayerization.release();

Is there a reason we need to release() sooner than the UniquePtr goes out of scope?
Attachment #8985352 - Flags: review?(botond) → review+
Comment on attachment 8985353 [details]
Bug 1468545 - Remove incorrect comment.

https://reviewboard.mozilla.org/r/250978/#review257610

::: commit-message-69996:3
(Diff revision 2)
> +Bug 1468545 - Remove incorrect comment. r?botond
> +
> +This comment is wrong because the scrollbar might have scheduled a

I think it would make sense to put this explanation into a replacement comment.
Attachment #8985353 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #26)
> 
> Is there a reason we need to release() sooner than the UniquePtr goes out of
> scope?

The release() is so that the UniquePtr doesn't destroy the object, which is what wood happen if it went out of scope.

(In reply to Botond Ballo [:botond] from comment #27)
> > +This comment is wrong because the scrollbar might have scheduled a
> 
> I think it would make sense to put this explanation into a replacement
> comment.

Well, I didn't want to have a comment that says "the comment that was here before was wrong..." And the current state of affairs doesn't seem comment-worthy. i.e. "we should not pass preventDefault=true because that would cancel the event block" is obvious and what would expect so it doesn't deserve a comment.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #28)
> (In reply to Botond Ballo [:botond] from comment #26)
> > 
> > Is there a reason we need to release() sooner than the UniquePtr goes out of
> > scope?
> 
> The release() is so that the UniquePtr doesn't destroy the object, which is
> what wood happen if it went out of scope.

Oh whoops, I misread release() as reset()... never mind.

> (In reply to Botond Ballo [:botond] from comment #27)
> > > +This comment is wrong because the scrollbar might have scheduled a
> > 
> > I think it would make sense to put this explanation into a replacement
> > comment.
> 
> Well, I didn't want to have a comment that says "the comment that was here
> before was wrong..." And the current state of affairs doesn't seem
> comment-worthy. i.e. "we should not pass preventDefault=true because that
> would cancel the event block" is obvious and what would expect so it doesn't
> deserve a comment.

We shouldn't pass true, but a reader might wonder why we're not passing aEvent.DefaultPrevented(), like in ProcessWheelEvent().

(Actually, we probably could, since if we hit a scrollbar no one would have preventDefault()ed the event anyways?)

Anyways, this is a fairly minor detail, and readers can check the blame, so it's fine to leave as is.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c880ca3a66e1
Distinguish between upwards- and downwards-propagating information in InputAPZContext. r=botond
https://hg.mozilla.org/integration/autoland/rev/d8d2085c29c2
Move early-exit condition earlier to avoid more work, remove unused variable. r=botond
https://hg.mozilla.org/integration/autoland/rev/1deb0d199abe
Ensure the drag metrics gets to APZ before the target APZC for inactive scrollframes. r=botond
https://hg.mozilla.org/integration/autoland/rev/ebbe89ecd4ff
Remove incorrect comment. r=botond
https://hg.mozilla.org/integration/autoland/rev/32857be02587
Add some logging for drag event handling. r=botond
https://hg.mozilla.org/integration/autoland/rev/98d43578939d
Treat all helper_* files as support files. r=botond
https://hg.mozilla.org/integration/autoland/rev/79067eeb997d
Add a test for dragging the scrollbar on an inactive scrollframe. r=botond
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: