702463 - Make smooth scrolling use the refresh driver notifications instead of using its own timer

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Scott Johnson (:jwir3)]

Smooth scrolling can be slow and sluggish in some cases (see bug 202718 and bug 649770). If we use a method similar to bug 666446, we can probably connect this to the refresh driver to make this a bit better.

Comment 1

[(dormant account)]

Scott,
This is on snappy todo list for scrolling, can you take this on?

Comment 2

[Scott Johnson (:jwir3)]

(In reply to Taras Glek (:taras) from comment #1)
> Scott,
> This is on snappy todo list for scrolling, can you take this on?

Hi Taras:

I can probably take this on, but I'm pretty loaded until the source migration on the 20th. Is this something that will need to be done before then?

Comment 3

[(dormant account)]

There is no way we'd get all of our scrolling changes in before then. I'd like to aim for ff12

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Scott Johnson (:jwir3) from comment #2)
> I can probably take this on, but I'm pretty loaded until the source
> migration on the 20th.

Scott: Do you think you will have time to take this on now?

Comment 5

[Scott Johnson (:jwir3)]

(In reply to Jared Wein [:jwein and :jaws] from comment #4)

> Scott: Do you think you will have time to take this on now?

Probably not for FF12. I'm currently slated to help with font-inflation 100% right now, which is mission-critical to get into FF11 before Mobile World Congress (February). Our goal is to get this into FF11 before the beta-migration happens (I think Jan 31), and so we're all scrambling to get it finished. 

However, I'm not sure this bug will take a giant amount of time. I will start putting together a general design during some free time that I have, and we'll see how long it will take.

Comment 6

[(no longer active)]

Boris (or anyone else), can you please let us know what needs to be done in this bug?

Comment 7

[Timothy Nikkel (:tnikkel)]

For smooth scrolling we currently use a timer that fires every 1000 / 60 ms to do each small step of a single smooth scroll operation. We initialize it in nsGfxScrollFrameInner::ScrollTo (mAsyncScroll->mScrollTimer->InitWithFuncCallback...). I'm assuming this is about using the refresh driver instead of that separate timer.

Comment 8

[(no longer active)]

So is this only about replacing the timer with a call to AddRefreshObserver using Flush_Display (plus the necessary cruft, of course)?

Also, can somebody please let me know what smooth scrolling is?  :-)  (yeah, I know, sorry for my ignorance!)

Comment 9

[Timothy Nikkel (:tnikkel)]

If you scroll using the mouse wheel or whatever once, say you scroll down by 20 pixels, then instead of drawing the page, and then the page moved by 20 pixels smooth scrolling animates intermediate states.

Comment 10

[(no longer active)]

OK, I can work on this if nobody else finds enough time to do it, but not until next week I guess.  Lawrence, please let me know if somebody picks this up or not by let's say mid next week.  Thanks!

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

Ehsan, I don't see that anyone else has picked this up. Can you run with this?

Comment 12

[(no longer active)]

OK I thought about this a bunch and looked around the code base.  There's no way that fixing this is going to help with jank, as the smooth scrolling doesn't result in paints directly.  It just invalidates, and the results of that invalidation will be painted the next time that the refresh driver gets triggered.

I'm removing the [snappy] tag from this bug.

Comment 13

[(no longer active)]

Adjusting the summary to reflect what this bug is really about.

Comment 14

[(dormant account)]

(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> OK I thought about this a bunch and looked around the code base.  There's no
> way that fixing this is going to help with jank, as the smooth scrolling
> doesn't result in paints directly.  It just invalidates, and the results of
> that invalidation will be painted the next time that the refresh driver gets
> triggered.
> 

this bug is back on snappy agenda, Timothy said this bug is the next step in solving bug 728738

Comment 15

[(dormant account)]

lowering priority to p2 as this is likely to not be a sole cause of jank during scrolling.

Comment 16

[linux.user.since.2002]

What seems to be a subpixel hinting "shadow" appears around the text when scrolling at speeds faster than x.  When the "shadow" appears, legibility and readability of the text decreases significantly.

Will changing to the "refresh driver" address this issue?

hsync: 67.5ms
vsync: 60.0ms

$ xvidtune -show
"1920x1080"   148.50   1920 2008 2052 2200   1080 1084 1089 1125 +hsync +vsync

Also, what is "jank" in this context?  Visual artifacts?  Something else?  Perhaps "jerk"?
https://en.wikipedia.org/wiki/Jerk_%28physics%29

Comment 17

[Timothy Nikkel (:tnikkel)]

That sounds like an unrelated issue, please file a new bug for it.

Comment 18

[Scott Johnson (:jwir3)]

(In reply to linux.user.since.2002 from comment #16)
> Also, what is "jank" in this context?  Visual artifacts?  Something else? 

"Jank" is a term used at mozilla to indicate small (usually less than a second, but it could be more than that) freezes or pauses that make it seem as though the UI is unresponsive, or at least less responsive that is expected by the user.

E.g. I'm experiencing a form of jank as I type this reply, as I'll start typing, but some of the characters aren't showing in the text box as quickly as I type them, resulting in mistypes that would have been immediately corrected if I had been able to see what I was typing.

Comment 19

[Avi Halachmi (:avih)]

Attachment: Smooth scroll - Use refresh observer instead of a timer when possible

Comment 20

[Timothy Nikkel (:tnikkel)]

Comment on attachment 611251 [details] [diff] [review]
Smooth scroll - Use refresh observer instead of a timer when possible

>+      if (!mAsyncScroll->ObserveRefresh(mOuter, this)){
>+        // Timer fallback
>+        mAsyncScroll->mScrollTimer->InitWithFuncCallback(
>+          AsyncScrollCallback, this, 1000 / 60,
>+          nsITimer::TYPE_REPEATING_SLACK);
>+      }

Don't fall back on the old timer code, just rip out all of the old timer code.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 611251 [details] [diff] [review]
Smooth scroll - Use refresh observer instead of a timer when possible

Review of attachment 611251 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1347,5 @@
> +    }
> +  }
> +  
> +  bool ObserveRefresh(nsContainerFrame *aContainer, nsGfxScrollFrameInner *aCallee){
> +    mObserver = new ScrollRefreshObserver(aCallee);

Should we add an assert before assigning to check that mObserver == nsnull?

Comment 22

[Avi Halachmi (:avih)]

(In reply to Timothy Nikkel (:tn) from comment #20)
> Don't fall back on the old timer code, just rip out all of the old timer
> code.

The timer is still used for non-smooth async scrolls, do you want to remove those too and hook it to a single refresh driver notification instead? Otherwise, the change is only at the code you quoted, and the timer is not "ripped out" from the code.


(In reply to Jared Wein [:jaws] from comment #21)
> Should we add an assert before assigning to check that mObserver == nsnull?

Sounds good to me. Thanks.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Avi Halachmi (:avih) from comment #22)
> The timer is still used for non-smooth async scrolls, do you want to remove
> those too and hook it to a single refresh driver notification instead?
> Otherwise, the change is only at the code you quoted, and the timer is not
> "ripped out" from the code.

Yes please!

Comment 24

[Avi Halachmi (:avih)]

Attachment: Part 1 v2 - Smooth scroll - Use refresh observer instead of a timer

Comment 25

[Avi Halachmi (:avih)]

Attachment: Part 2 v2 - Replace non-smooth async one-shot timer with a refresh observer

Rip it out!!1! AARRGGGgghh.. mmm... yes.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 611262 [details] [diff] [review]
Part 1 v2 - Smooth scroll - Use refresh observer instead of a timer

Review of attachment 611262 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1432,5 @@
> +
> +  void RemoveObserver() {
> +    if (mObserver) {
> +      if (mContainer && mContainer->PresContext() && mContainer->PresContext()->RefreshDriver()) {
> +        mContainer->PresContext()->RefreshDriver()->RemoveRefreshObserver(mObserver, Flush_Display);

This is a bit over-complicated. PresContext() and RefreshDriver() can never be null. Instead of storing mContainer here, mObserver->mCallee->mOuter is a frame you can use to get to the refresh driver.

But why not just make AsyncScroll implement nsARefreshObserver directly?

Comment 27

[Avi Halachmi (:avih)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> Comment on attachment 611262 [details] [diff] [review]
> Part 1 v2 - Smooth scroll - Use refresh observer instead of a timer
> 
> Review of attachment 611262 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +1432,5 @@
> > +
> > +  void RemoveObserver() {
> > +    if (mObserver) {
> > +      if (mContainer && mContainer->PresContext() && mContainer->PresContext()->RefreshDriver()) {
> > +        mContainer->PresContext()->RefreshDriver()->RemoveRefreshObserver(mObserver, Flush_Display);
> 
> This is a bit over-complicated. PresContext() and RefreshDriver() can never
> be null.

OK, I didn't know that. I couldn't get a better answer than "usually they're not null", and I can't count on "usually".

> Instead of storing mContainer here, mObserver->mCallee->mOuter is a
> frame you can use to get to the refresh driver.

Indeed, but I prefer to not directly access member vars (mCallee, mOuter), to avoid breaking encapsulation. Getters would have been fine with me. I think I'll add them where needed.

> But why not just make AsyncScroll implement nsARefreshObserver directly?

AsyncScroll was a "clean" class that did only scroll related calculations, and held a timer (but not callbacks), so I hoped to keep it doing only this one thing. But seeing that the observer management turned into a noticeable chunk.. if I wouldn't find a nice way to keep AsyncScroll clean (also from observer management), then I'll just stuff everything into AsyncScroll.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Avi Halachmi (:avih) from comment #27)
> OK, I didn't know that. I couldn't get a better answer than "usually they're
> not null", and I can't count on "usually".

Getters that don't have "Get" in the name can't return null.

> > Instead of storing mContainer here, mObserver->mCallee->mOuter is a
> > frame you can use to get to the refresh driver.
> 
> Indeed, but I prefer to not directly access member vars (mCallee, mOuter),
> to avoid breaking encapsulation.

Good call.

> Getters would have been fine with me. I
> think I'll add them where needed.

Yeah, or you could have a RemoveObserver method on mObserver.

> > But why not just make AsyncScroll implement nsARefreshObserver directly?
> 
> AsyncScroll was a "clean" class that did only scroll related calculations,
> and held a timer (but not callbacks), so I hoped to keep it doing only this
> one thing. But seeing that the observer management turned into a noticeable
> chunk.. if I wouldn't find a nice way to keep AsyncScroll clean (also from
> observer management), then I'll just stuff everything into AsyncScroll.

Generally speaking, combining two objects that have a 1:1 relationship into a single object makes code simpler.

Comment 29

[Avi Halachmi (:avih)]

Attachment: Part 1 v3 - Smooth scroll - Use refresh observer instead of a timer

Comment 30

[Avi Halachmi (:avih)]

Attachment: Part 2 v3 - Replace non-smooth async one-shot timer with a refresh observer

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please merge these patches. It's just confusing to have them separated.

Comment 32

[Avi Halachmi (:avih)]

Attachment: (Part 1/1) v3 - Smooth scroll - Use refresh observer instead of a timer

Comment 33

[Avi Halachmi (:avih)]

Attachment: (Part 1/1) v5 - Smooth scroll - Use refresh observer instead of a timer

Oops, NS_INLINE_DECL_REFCOUNTING(AsyncScroll) had the previous class name.

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 611640 [details] [diff] [review]
(Part 1/1) v5 - Smooth scroll - Use refresh observer instead of a timer

Review of attachment 611640 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1384,5 @@
>  
>    void InitDuration(nsIAtom *aOrigin);
> +
> +// The next section is observer/callback management
> +// Boddies of WillRefresh and RefreshDriver contain nsGfxScrollFrameInner specific code.

s/Boddies/Bodies

@@ +1418,5 @@
> +    }
> +  }
> +
> +private:
> +  void *mCallee;

Can mCallee be declared as a nsGfxScrollFrameInner* ? If this is possible, then we can make RefreshDriver take a nsGfxScrollFrameInner* instead of void*.

Comment 35

[Avi Halachmi (:avih)]

(In reply to Jared Wein [:jaws] from comment #34)
> > +private:
> > +  void *mCallee;
> 
> Can mCallee be declared as a nsGfxScrollFrameInner* ? If this is possible,
> then we can make RefreshDriver take a nsGfxScrollFrameInner* instead of
> void*.

If I understand you correctly, then this was also my initial approach, but then I wanted to limit nsGfxScrollFrameInner specific code to only within RefreshDriver and WillRefresh, while leaving their signatures and the rest of AsyncScroll "callee agnostic".

But I don't mind changing it, just let me know if you still want that.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 611640 [details] [diff] [review]
(Part 1/1) v5 - Smooth scroll - Use refresh observer instead of a timer

Review of attachment 611640 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1395,5 @@
> +   * Return value: true on success, false otherwise.
> +   */
> +  bool SetRefreshObserver(nsGfxScrollFrameInner *aCallee) {
> +    if (!aCallee) {
> +      return false;

This patch can't be taken, aCallee is never null

@@ +1398,5 @@
> +    if (!aCallee) {
> +      return false;
> +    }
> +    if (mCallee) {
> +      RemoveObserver();

This path can't be taken, right? SetRefreshObserver only gets called immediately after construction.

@@ +1413,5 @@
> +    // The callback may release "this".
> +    // We don't touch members after returning, but use KungFu for good measure.
> +    nsRefPtr<AsyncScroll> kungFuDeathGrip = this;
> +    if (mCallee) {
> +      nsGfxScrollFrameInner::AsyncScrollCallback((nsGfxScrollFrameInner*)mCallee);

You should modify AsyncScrollCallback to take a TimeStamp parameter so you can pass aTime into it instead of using Now().

@@ +1694,3 @@
>      self->ScrollToImpl(destination);
>    } else {
> +    self->mAsyncScroll = nsnull; // Auto DTOR

Take out these "Auto DTOR" comments

::: layout/generic/nsGfxScrollFrame.h
@@ +289,5 @@
>    nsIBox* mResizerBox;
>    nsContainerFrame* mOuter;
> +  nsContainerFrame* ContainerFrame() {
> +    return mOuter;
> +  }

Don't add this

Comment 37

[Avi Halachmi (:avih)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> You should modify AsyncScrollCallback to take a TimeStamp parameter so you
> can pass aTime into it instead of using Now().

I have explicitly considered this improvement before posting the patch, and when I asked around if there's a good reason to use Now() instead of the refresh observer event timestamp, bz answered that 1) they're different, and 2) specifically, the refresh driver timeline can also be frozen. Since we want the document position right now (regardless of events queue lags or timeline freezes), I've chosen to keep this part of the code as is.

> ::: layout/generic/nsGfxScrollFrame.h
> @@ +289,5 @@
> >    nsIBox* mResizerBox;
> >    nsContainerFrame* mOuter;
> > +  nsContainerFrame* ContainerFrame() {
> > +    return mOuter;
> > +  }
> 
> Don't add this

So I should directly use mCallee->mOuter->... ?

Comment 38

[Avi Halachmi (:avih)]

Attachment: v6 - Smooth scroll - Use refresh observer instead of a timer

I've kept the TimeStanp::Now(), and now using mCalee->mOuter->... directly.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Avi Halachmi (:avih) from comment #37)
> I have explicitly considered this improvement before posting the patch, and
> when I asked around if there's a good reason to use Now() instead of the
> refresh observer event timestamp, bz answered that 1) they're different, and
> 2) specifically, the refresh driver timeline can also be frozen. Since we
> want the document position right now (regardless of events queue lags or
> timeline freezes), I've chosen to keep this part of the code as is.

The timeline is only frozen for background tabs, when we indeed don't want to be doing smooth scrolling! If animations are frozen, so should scrolling. So let's use aTime.

Comment 40

[Avi Halachmi (:avih)]

Attachment: v7 - Smooth scroll - Use refresh observer instead of a timer

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 611983 [details] [diff] [review]
v7 - Smooth scroll - Use refresh observer instead of a timer

Review of attachment 611983 [details] [diff] [review]:
-----------------------------------------------------------------

Everything else is fine.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1411,5 @@
> +    nsGfxScrollFrameInner::AsyncScrollCallback((nsGfxScrollFrameInner*)mCallee, aTime);
> +  }
> +
> +private:
> +  void *mCallee;

Make this nsGfxScrollFrameInner*.

Comment 42

[Avi Halachmi (:avih)]

Attachment: v8 - Smooth scroll - Use refresh observer instead of a timer

Comment 43

[Jared Wein [:jaws] (please needinfo? me)]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b8a666e351e

Comment 44

[Jared Wein [:jaws] (please needinfo? me)]

Backed out due to test failures on Linux mochitest-4. See here for test failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4e515b6a5a41

Backout changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01c41b9cd650

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Failures on mochitest-chrome as well.

Comment 46

[Avi Halachmi (:avih)]

FWIW:
1. Ms2ger was kind enough to push v6 of this patch to the try server. Results are similar ( https://tbpl.mozilla.org/?tree=Try&rev=3ff720feb208 ).

2. I tried to test locally one of the tests that fail on windows (mochitest-4), which gives the following errors:
mochitest-4 failed:
5263 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_bug549262.html | Page is scrolled down - didn't expect 0, but got it
5269 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_bug549262.html | Page is scrolled down - didn't expect 0, but got it
5271 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_bug549262.html | Page is not scrolled up - didn't expect 0, but got it
5273 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_bug549262.html | Page is scrolled up
14876 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug496275.html | test 9b: Wrong anchor offset. - got 3, expected 0
14878 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug496275.html | test 9b: Wrong focus offset. - got 3, expected 0
14880 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug496275.html | test 9c: Wrong anchor offset. - got 3, expected 0
14888 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug496275.html | test 10b: Wrong anchor offset. - got 3, expected 0
14890 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug496275.html | test 10b: Wrong focus offset. - got 3, expected 0
14892 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug496275.html | test 10c: Wrong anchor offset. - got 3, expected 0
To rerun your failures please run 'make mochitest-plain-rerun-failures'

So I looked at the test which seem related to scrolling: at test_bug549262.html, and ran only this test, and got 3-5 failures on each run, all similar to the above.

At the test file itself, I saw that all the tests are executed in instant succession with setTimeout(function(){...}, 0). So I changed the timeout to 100ms, all tests passed. Changed it to 20ms: all tests passed, changed to 15ms, 1-3 failures on each run. Again to 20ms: all tests passed (tried few runs with each timeout).

I have a slight suspicion that these tests don't expect async scroll (even if not smooth), and maybe the difference between the previous 16ms (fixed, by the timer) and the current 0-16ms (to the first refresh observer callback), is what breaking it.

Not sure how to proceed with this though, and if the other failures are similar in nature to this one.

Comment 47

[Markus Stange [:mstange]]

(In reply to Avi Halachmi (:avih) from comment #46)
> I have a slight suspicion that these tests don't expect async scroll

They do, that's what the setTimeout is used for here.

> and maybe the difference between the previous 16ms (fixed,
> by the timer)

The previous async scroll timer was a zero-timeout one-shot timer, iirc.

> Not sure how to proceed with this though

Using requestAnimationFrame instead of setTimeout should work, I think.

Comment 48

[Avi Halachmi (:avih)]

(In reply to Markus Stange from comment #47)
> The previous async scroll timer was a zero-timeout one-shot timer, iirc.

Indeed, I changed this code at this patch, and already forgot the async non smooth case ;)

> Using requestAnimationFrame instead of setTimeout should work, I think.

Unfortunately, this didn't work, but nextNextFrame did work (and all tests passed), like this:

var nextFrame = window.requestAnimationFrame || window.mozRequestAnimationFrame;
function nextNextFrame(func){
  nextFrame(function(){
    nextFrame(func);
  });
}; 

Is this expected? and If yes, how should I proceed with this? (also, some of the failures are on Linux only, so I can't personally verify that the tests pass, or fail, possibly due to similar reason).

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't understand why two frames are necessary. Maybe if you throw some dump()s into JS and some printfs into your code you can see which parts of the async scroll mechanism run between each requestAnimationFrame?

Comment 50

[Avi Halachmi (:avih)]

That's my nextNextFrame code with the dumps:

var nextFrame = window.requestAnimationFrame || window.mozRequestAnimationFrame;
function nextNextFrame(func){
  dump("[Direct] Issuing first nextFrame...\n");
  nextFrame(function(){
    dump("[Frame] Issuing second nextFrame...\n");
    nextFrame(function(){
      dump("[Frame] Issueing actual test...\n");
      func();
    });
  });
};  


And these are 2 outputs which exhibit different behavior (I added numbers to the tests outputs to make it easier to follow):

Example where one frame would have been enough, probably:

/* some trigger from the test */
SetRefreshObserver: added.
[Direct] Issuing first nextFrame...
AsyncScrollCallback: starting...
AsyncScrollCallback: ScrollToImpl(self->mDestination): issued.
RemoveObserver: removed.
[Frame] Issuing second nextFrame... //<-- testing here instead of waiting another frame would probably work.
[Frame] Issueing actual test...
TEST-PASS | unknown test url | 8. Page is scrolled down - 561 should not equal 0


This needs 2 frames. Refresh observer appears to be added on time, but the callback (scrolling) only happens on the 2nd frame.

/* some trigger from the test */
SetRefreshObserver: added.
[Direct] Issuing first nextFrame...
[Frame] Issuing second nextFrame... //<-- testing here would have failed (before actual scroll)
AsyncScrollCallback: starting...
AsyncScrollCallback: ScrollToImpl(self->mDestination): issued.
RemoveObserver: removed.
[Frame] Issueing actual test...
TEST-PASS | unknown test url | 4. Page is scrolled up - 0 should equal 0


Test 4 doesn't always require 2 frames. Sometimes its print is similar to the "correct" test 8. However, it seems that some tests always pass, while other tests only sometimes pass.

Comment 51

[Gary [:streetwolf52]]

First off, is this bug fix in the latest Nightly?  If so, I've been getting black areas in the editor used by the Stylish Add-On in it's standalone mode when I scroll.  If I move my pointer out of the edit window all the black areas go away after about 4 seconds and everything looks fine until I scroll again.  

I traced the regression to the changeset this bug fix is included with.  Could it be the cause of the problem? 


Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120405 Firefox/14.0a1

Comment 52

[Timothy Nikkel (:tnikkel)]

No it's not in any nightly. It got backed out before it could make it to one.

Comment 53

[Markus Stange [:mstange]]

> /* some trigger from the test */
> SetRefreshObserver: added.
> [Direct] Issuing first nextFrame...

Can you add another dump into nsRefreshDriver::ScheduleFrameRequestCallbacks, and print the address of the used refresh driver (both here and in SetRefreshObserver)? Maybe requestAnimationFrame uses a different refresh driver than the smooth scroll refresh observer.

Comment 54

[Avi Halachmi (:avih)]

(In reply to Markus Stange from comment #53)
> Can you add another dump into
> nsRefreshDriver::ScheduleFrameRequestCallbacks, and print the address of the
> used refresh driver (both here and in SetRefreshObserver)? Maybe
> requestAnimationFrame uses a different refresh driver than the smooth scroll
> refresh observer.

Indeed, they're not the same one:

SetRefreshObserver: added (for driver e6ead20).
[Direct] Issuing first nextFrame...
nsRefreshDriver::ScheduleFrameRequestCallbacks (this: e6ea880)
[Frame] Issuing second nextFrame...
nsRefreshDriver::ScheduleFrameRequestCallbacks (this: e6ea880)
AsyncScrollCallback: start.
AsyncScrollCallback: ScrollToImpl(self->mDestination): issued.
RemoveObserver: removed.
[Frame] Issueing actual test...
TEST-PASS | unknown test url | 4. Page is scrolled up - 0 should equal 0

Comment 55

[Avi Halachmi (:avih)]

I've added some timestamps to the cpp prints ([<[ms%1000>]). It appears that the driver which the document uses (not the smooth scroll one) if firing too rapidly. This is consistent among several runs.

This is where I added the print at nsRefreshDriver::Notify :

      for (PRUint32 i = 0; i < frameRequestCallbacks.Length(); ++i) {
        printf("[%d] Issuing frame request callback (%d)...\n",
               (int)((int)((TimeStamp::Now()-TimeStamp()).ToMilliseconds())%1000), (int)i);
        frameRequestCallbacks[i]->Sample(eventTime);
      }

And that's a typical result (specifically, the first frame callback is 1-3 ms after scheduling it, and the 2nd one is less than 16ms later):

[890] SetRefreshObserver: added (for driver eba4b98).
[Direct] Issuing first nextFrame...
[891] nsRefreshDriver::ScheduleFrameRequestCallbacks (this: eba46f8)
[894] Issuing frame request callback (0)...
[Frame] Issuing second nextFrame...
[894] nsRefreshDriver::ScheduleFrameRequestCallbacks (this: eba46f8)
[899] AsyncScrollCallback: starting...
AsyncScrollCallback: ScrollToImpl(self->mDestination): issued.
RemoveObserver: removed.
[905] Issuing frame request callback (0)...
[Frame] Issueing actual test...
TEST-PASS | unknown test url | 4. Page is scrolled up - 0 should equal 0

Comment 56

[Boris Zbarsky [:bzbarsky]]

Smooth scroll is almost certainly using the chrome refresh driver, yes?

Comment 57

[Timothy Nikkel (:tnikkel)]

(In reply to Boris Zbarsky (:bz) from comment #56)
> Smooth scroll is almost certainly using the chrome refresh driver, yes?

No, the patch in this bug makes it use the refresh driver of the document that contains the scroll frame.

Comment 58

[Avi Halachmi (:avih)]

So, how do we go on with this bug?

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We need to figure out why the refresh drivers are different. They should be the same; they should be the refresh driver for the prescontext containing the scrollframe.

Comment 60

[Avi Halachmi (:avih)]

I also see these issues:
- requestAnimationFrame seems to arrive too quickly (esp. the 2nd one).
- If they use the same driver, we might still need 2 frames, unless we can count on scroll happening first because we know that Flush_Display notifications come before frame notifications (or other similar knowledge).

However, I'm not sure I'm up for this task of refresh driver[s] analysis...

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

bz, any ideas?

Comment 62

[Timothy Nikkel (:tnikkel)]

You could try to figure out what these refresh drivers are. Check IsChrome() on the refresh drivers mPresContext.

Comment 63

[Boris Zbarsky [:bzbarsky]]

Or just look at prescontext's mDocument.mRawPtr->mDocumentURI.mRawPtr->mSpec.mData ?

Comment 64

[Avi Halachmi (:avih)]

I can try to follow your remote debugging instructions, one after the other, but I really think that someone who is familiar with the refresh driver should look at it.

The refresh driver and related code is not minor, and I have zero familiarity or even a mental model of it, pressContext, etc. It would be an exercise in frustration if I try to understand these issues myself...

Comment 65

[(dormant account)]

(In reply to Avi Halachmi (:avih) from comment #64)
> I can try to follow your remote debugging instructions, one after the other,
> but I really think that someone who is familiar with the refresh driver
> should look at it.

We can get someone on this after next week(if it doesn't get solved first). Our layout guys are busy in fennec-land atm.

Comment 66

[Jared Wein [:jaws] (please needinfo? me)]

I talked with Ehsan about this today and if it is only test failures that are holding up the patch, then we should take a finer look at the tests.

If the tests are poorly written and dependent on a timing situation that is broken based on the presence of multiple refresh drivers, then we should file a follow up bug to make the refresh driver unique.

If we do find that the tests are poorly written, then we should disable those tests or fix them by adjusting the timeouts in them.

I am repushing this patch to tryserver now to get an update on the test situation.

Comment 67

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 611994
	Branch: mozilla-central => try
Patch 611994 could not be applied to mozilla-central.
patching file layout/generic/nsGfxScrollFrame.cpp
Hunk #3 succeeded at 1658 with fuzz 1 (offset 21 lines).
Hunk #4 FAILED at 1661
1 out of 6 hunks FAILED -- saving rejects to file layout/generic/nsGfxScrollFrame.cpp.rej
patching file layout/generic/nsGfxScrollFrame.h
Hunk #1 FAILED at 173
1 out of 2 hunks FAILED -- saving rejects to file layout/generic/nsGfxScrollFrame.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patch 611994 could not be applied to mozilla-central.
patching file layout/generic/nsGfxScrollFrame.cpp
Hunk #1 FAILED at 1306
Hunk #2 FAILED at 1376
Hunk #3 FAILED at 1589
Hunk #4 FAILED at 1616
Hunk #5 FAILED at 1660
Hunk #6 FAILED at 1679
6 out of 6 hunks FAILED -- saving rejects to file layout/generic/nsGfxScrollFrame.cpp.rej
patching file layout/generic/nsGfxScrollFrame.h
Hunk #1 FAILED at 173
Hunk #2 FAILED at 282
2 out of 2 hunks FAILED -- saving rejects to file layout/generic/nsGfxScrollFrame.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.

Comment 68

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Rebased v8 of patch

Rebased and pushed to try server: https://tbpl.mozilla.org/?tree=Try&rev=c5efd5943cc3

Comment 69

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Increasing the timeout of the tests due to the presence of multiple refresh drivers.

This patch increases the timeouts of the failing tests to 20ms based on comment #46.

tryserver results: https://tbpl.mozilla.org/?tree=Try&rev=bfbf61671e45

Comment 70

[(no longer active)]

Comment on attachment 619353 [details] [diff] [review]
Increasing the timeout of the tests due to the presence of multiple refresh drivers.

Review of attachment 619353 [details] [diff] [review]:
-----------------------------------------------------------------

Please file a follow-up bug to fix the multiple refresh driver problem, and also to revert this patch once that happens.  Thanks!

Comment 71

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c70f3a9b31f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/06091f9ed1dd

Comment 72

[Dão Gottwald [:dao]]

Status flags are for aurora/beta/release branch landings and shouldn't be set when landing on integration branches such as inbound or fx-team anyway.

Comment 73

[Avi Halachmi (:avih)]

Thanks guys :)

Comment 74

[(no longer active)]

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

Comment 75

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/06091f9ed1dd