1250550 - Scroll events appear delayed even with APZ disabled

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

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

In bug 1246290 :jryans discovered that even with APZ disabled, the devtools gutter detaches and doesn't stay "stuck" to the left edge where it's supposed to be. I haven't confirmed but this might be a regression from bug 1209970 which modified when scroll events are dispatched. My understanding was that the events would just be aligned to vsync, and they should fire before a paint actually happened. i.e. instead of this:

scrollTo scrollEvent scrollTo scrollEvent vsync paint

we'd get this:

scrollTo scrollTo vsync scrollEvent paint

However it seems like somehow we're getting a paint in before the scrollEvent. This is trivially reproducible by scrolling https://bug1209970.bmoattachments.org/attachment.cgi?id=8667908 with APZ disabled.

Comment 1

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

Confirmed as a regression from bug 1209970.

Comment 2

[Botond Ballo [:botond]]

Stealing with permission.

Comment 3

[Botond Ballo [:botond]]

Here's what's going on:

  - nsRefreshDriver maintains three lists of refresh observers, one for each
    flush type: Flush_Style, Flush_Layout, and Flush_Display.

  - During a tick, it runs through each list of observers, in order, and runs
    them. To iterate over each list, it uses an EndLimitedIterator, which is
    designed to iterate only over elements present when the iterator was
    created, not elements added afterwards. This means that, for a given flush
    type, a refresh observer added during the execution of another refresh
    observer of that flush type, will not run until the next tick.

  - During main-thread animation-driven scrolling, scroll events are *posted*
    by AsyncScroll::WillRefresh(). AsyncScroll registers itself as a Flush_Style
    refresh observer.

  - Posting a scroll event, as of bug 1209970, registers another Flush_Style 
    refresh observer, which *fires* the event when run. However, as we're
    already running through Flush_Style observers when posting the event, this
    observer will not run until the next refresh driver tick.

  - Painting happens near the end of the refresh driver tick, after all refresh
    observers have run. So, in this scenario, we'll get a paint after the scroll
    event is posted, but before it fires (which happens on the next tick).

Comment 4

[Botond Ballo [:botond]]

By contrast, prior to bug 1209970, posting a scroll event registered a "will paint observer", which get run later during the refresh driver tick, so that registering one during the execution of a refresh observer will result in it running during the same tick.

Comment 5

[Botond Ballo [:botond]]

Changing the refresh observer that fires the scroll event to have flush type Flush_Layout resolves the problem.

Comment 6

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1250550 - Ensure a scroll event posted during a refresh driver tick fires during that same tick. r=mats

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

Comment 7

[Botond Ballo [:botond]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=273840f35687

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #7)
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=273840f35687

This Try push has a number of failures, including some crashes in nsRefreshDriver::Tick().

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8723288 [details]
MozReview Request: Bug 1250550 - Ensure a scroll event posted during a refresh driver tick fires during that same tick. r=mats

Dropping review until we figure out the crashes.

Comment 10

[Botond Ballo [:botond]]

Oh, I made a silly mistake: I failed to change an occurrence of Flush_Style in the ScrollEvent destructor to Flush_Layout.

Comment 11

[Botond Ballo [:botond]]

New Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dda40dc2e26a

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8723288 [details]
MozReview Request: Bug 1250550 - Ensure a scroll event posted during a refresh driver tick fires during that same tick. r=mats

That's looking much better. Re-flagging for review.

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8723288 [details]
MozReview Request: Bug 1250550 - Ensure a scroll event posted during a refresh driver tick fires during that same tick. r=mats

https://reviewboard.mozilla.org/r/36445/#review33801

This looks good, but it's a bit hard to follow how all of this works.
So please add a code comment describing the overall design (perhaps as
a doc comment on class ScrollEvent in the header?).
I think you can basically paste comment 3 into that comment,
with reformatting / rephrasing as you see fit.  Thanks.

(sorry for the review delay)

Comment 14

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

Since Botond is at a conference this week, I'll land this in the interests of getting it in sooner and uplifting it to aurora before the merge. I'm adding the code comment as requested in comment 13.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/447498cdecee

Comment 16

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

(In reply to Botond Ballo [:botond] [C++ standards meeting Feb 29 - Mar 5] from comment #10)
> Oh, I made a silly mistake: I failed to change an occurrence of Flush_Style
> in the ScrollEvent destructor to Flush_Layout.

Whoops, I pulled the patch from reviewboard which didn't have this fix! Landing a follow-up...

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd104f75b8e8

Comment 18

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

^ That's the relanding with the correct patch. I backed out the original landing in https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4f419ba470

Comment 19

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

[Tracking Requested - why for this release]: regression in 46, pretty noticeable on some sites.

Comment 20

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

Comment on attachment 8723288 [details]
MozReview Request: Bug 1250550 - Ensure a scroll event posted during a refresh driver tick fires during that same tick. r=mats

Approval Request Comment
[Feature/regressing bug #]: bug 1209970
[User impact if declined]: even with APZ disabled the scroll event sent to content may be delayed, resulting in laggy scrolling effects
[Describe test coverage new/current, TreeHerder]: tested locally
[Risks and why]: small change, well understood code. pretty low risk
[String/UUID change made/needed]: none

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking, regression in 46 from apz.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8723288 [details]
MozReview Request: Bug 1250550 - Ensure a scroll event posted during a refresh driver tick fires during that same tick. r=mats

Fix for recent regression, go ahead and uplift.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ab5a7604001b

Comment 25

[Ada [:adalucinet]]

Reproduced with Nightly 47.0a1 (2016-02-28), under Windows 10 64-bit.
Verified fixed with 46.0b9 (Build ID: 20160407053945) and latest Aurora 47.0a2 (from 2016-04-10), under Windows 10 64-bit and Windows 7 64-bit - no delayed scroll events with the provider URL and APZ disabled.
Since this issue is still reproducible with APZ and e10s enabled (but *not* with e10s disabled), is there any bug under your radar?

Comment 26

[Botond Ballo [:botond]]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #25)
> Since this issue is still reproducible with APZ and e10s enabled (but *not*
> with e10s disabled), is there any bug under your radar?

I believe that With APZ enabled, this issue should have been fixed by bug 1250398 (and a regression in this behaviour fixed by bug 1256727).

If you're still able to reproduce this with APZ enabled, can you say with which build?

Comment 27

[Ada [:adalucinet]]

(In reply to Botond Ballo [:botond] from comment #26)
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #25)
> > Since this issue is still reproducible with APZ and e10s enabled (but *not*
> > with e10s disabled), is there any bug under your radar?
> 
> I believe that With APZ enabled, this issue should have been fixed by bug
> 1250398 (and a regression in this behaviour fixed by bug 1256727).
> 
> If you're still able to reproduce this with APZ enabled, can you say with
> which build?

I can reproduce with every build - 46 beta 9, latest Aurora 47.0a2 and Nightly 48.0a1 - but not with 45.0.2 (unable to activate e10s there), under Windows 7 64-bit and Windows 10 64-bit; reproducible *only* with APZ and e10s enabled.

Comment 28

[Botond Ballo [:botond]]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #27)
> I can reproduce with every build - 46 beta 9, latest Aurora 47.0a2 and
> Nightly 48.0a1 - but not with 45.0.2 (unable to activate e10s there), under
> Windows 7 64-bit and Windows 10 64-bit; reproducible *only* with APZ and
> e10s enabled.

I tried each of these builds, and I couldn't reproduce this problem.

For reference, here is the STR I was trying:

  1. Verify in about:support that e10s and APZ are enabled.
  2. Load a webpage that uses JS files with long lines (so they are
     horizontally scrollable in the debugger). Most web pages fit
     this bill, including google.com.
  3. Open the devtools, and switch to the Debugger panel.
  4. Scroll the code horizontally with the trackpad.

If you're using a different STR, could you say what you are doing differently? A video might be helpful.

Comment 29

[Petruta Horea [:phorea], Desktop QA]

(In reply to Botond Ballo [:botond] from comment #28)
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #27)
> > I can reproduce with every build - 46 beta 9, latest Aurora 47.0a2 and
> > Nightly 48.0a1 - but not with 45.0.2 (unable to activate e10s there), under
> > Windows 7 64-bit and Windows 10 64-bit; reproducible *only* with APZ and
> > e10s enabled.
> 
> I tried each of these builds, and I couldn't reproduce this problem.
> 
> For reference, here is the STR I was trying:
> 
>   1. Verify in about:support that e10s and APZ are enabled.
>   2. Load a webpage that uses JS files with long lines (so they are
>      horizontally scrollable in the debugger). Most web pages fit
>      this bill, including google.com.
>   3. Open the devtools, and switch to the Debugger panel.
>   4. Scroll the code horizontally with the trackpad.
> 
> If you're using a different STR, could you say what you are doing
> differently? A video might be helpful.

Thanks for the detailed steps. With those steps, we confirm this is indeed verified fixed.

We also tried the testcase from url: https://bug1209970.bmoattachments.org/attachment.cgi?id=8667908, that still doesn't properly work with apz enabled, but accordingly to https://bugzilla.mozilla.org/show_bug.cgi?id=1209970#c0 this is still expected.