Closed Bug 1250550 Opened 4 years ago Closed 4 years ago

Scroll events appear delayed even with APZ disabled

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox45 --- unaffected
firefox46 + verified
firefox47 --- verified

People

(Reporter: kats, Assigned: botond)

References

()

Details

(Keywords: perf, regression, Whiteboard: [gfx-noted])

Attachments

(1 file)

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.
Stealing with permission.
Assignee: bugmail.mozilla → botond
Keywords: perf
Whiteboard: [gfx-noted]
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).
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.
Changing the refresh observer that fires the scroll event to have flush type Flush_Layout resolves the problem.
(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 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.
Attachment #8723288 - Flags: review?(mats)
Oh, I made a silly mistake: I failed to change an occurrence of Flush_Style in the ScrollEvent destructor to Flush_Layout.
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.
Attachment #8723288 - Flags: review?(mats)
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)
Attachment #8723288 - Flags: review?(mats) → review+
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.
(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...
^ That's the relanding with the correct patch. I backed out the original landing in https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4f419ba470
[Tracking Requested - why for this release]: regression in 46, pretty noticeable on some sites.
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
Attachment #8723288 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/fd104f75b8e8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Tracking, regression in 46 from apz.
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.
Attachment #8723288 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
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?
Status: RESOLVED → VERIFIED
Flags: qe-verify+ → needinfo?(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?
Flags: needinfo?(botond) → needinfo?(alexandra.lucinet)
(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.
Flags: needinfo?(alexandra.lucinet)
(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.
Flags: needinfo?(alexandra.lucinet)
(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.
Flags: needinfo?(alexandra.lucinet)
You need to log in before you can comment on or make changes to this bug.