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.

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

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

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

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

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

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

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.

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!)

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.

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!

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

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.

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

(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

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

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

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

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

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

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

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

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?

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

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

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

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

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

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

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

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

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

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

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

Failures on mochitest-chrome as well.

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.

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

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

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?

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.

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

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

> /* 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.

(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

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

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

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

So, how do we go on with this bug?

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.

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

bz, any ideas?

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

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

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

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

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.

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.

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

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 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!

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

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.

Thanks guys :)