STR: (always reproducible on Win7, on the same fresh profile for Fx41,42 but not 39) 1.A) Open any page that takes long time to load (You can try these 2: http://geektimes.ru/ , http://www.twitch.tv/ . OR set privacy.trackingprotection.enabled -> true (that will force some pages with 3rd-party scripts to load infinitely) and test this page: http://tema.livejournal.com/109690.html 1.B) Open http://www.asdf.asdf/ and wait until you get error page. 2. Make sure that page is long enough for scrollbars to appear (you can change the window height/width) 3. Try autoscrolling RESULT: Page position is updated only once per ~1 second (see URL and attached video) EXPECTATIONS: It should be the same as when I autoscroll loaded pages.

I should've noted that if you choose A) scenario in STR then you should try autoscrolling BEFORE the page is loaded. That's what bug is about. In B) scenario you should try autoscrolling after you get error page. Watch both videos if you're unsure.

I have the same problem and found this report while searching for a fix!

I can now reproduce it on Release40 on WinXP. I'll check later if changing some preferences fixes it @Kiru: What keywords have you used for searching? I'll better add them to the title because I think that this bug will remain for long time. Please also specify your OS and Firefox version.

I tried many keywords before, but what lead me to this report was "autoscroll doesn't work while loading pictures". I already had this problem on Windows 8.1 after a specific firefox update, I think it was one of the version 40 updates. I currently use Windows 10, still bugged.

Oh forgot. I'm using Firefox 41.0b1.

Testcase: http://www.uvm.edu/~aguertin/test/slowimage/slowpage.html (Testcase needs custom php, so can't be attached to bugzilla. If it ever goes away, it's a php script that sleeps for 15 seconds, then sends an image, and an html page that has a bunch of text with the image at the bottom.) Does this testcase show the problem for you too? Using this testcase, I've found a regression range of 2015-04-21 - 2015-04-22. Whether or not e10s is enabled does NOT affect this. (Meaning some similar bugs are either different or wrong). Looking through the commits to find a likely candidate now.

(In reply to dolphinling from comment #6) > Testcase: http://www.uvm.edu/~aguertin/test/slowimage/slowpage.html > Does this testcase show the problem for you too? Yes, thanks fot the testcase. (I have a guess that smooth autoscroll doesn't work until 'load' event fires - that would explain my example with error page, because such pages don't fire 'load' events)

There's a lot of changesets in that period. I'm suspicious of bug 1145439, but it should probably have a proper bisect.

(In reply to dolphinling from comment #6) > Testcase: http://www.uvm.edu/~aguertin/test/slowimage/slowpage.html > Does this testcase show the problem for you too? Yes.

I bisected: The first bad revision is: changeset: 240253:19a79b7400fe user: Seth Fowler <mark.seth.fowler@gmail.com> date: Tue Apr 21 09:44:40 2015 -0700 summary: Bug 1145439 (Part 1) - Throttle requestAnimationFrame for non-visible iframes. r=mstange,mchang Added the patch author and reviewers to CC

check: bug 1185804

(In reply to dolphinling from comment #10) > I bisected: > > The first bad revision is: > changeset: 240253:19a79b7400fe > user: Seth Fowler <mark.seth.fowler@gmail.com> > date: Tue Apr 21 09:44:40 2015 -0700 > summary: Bug 1145439 (Part 1) - Throttle requestAnimationFrame for > non-visible iframes. r=mstange,mchang > > Added the patch author and reviewers to CC Hmm, maybe we are wrongly determine the content document is hidden during loading or something?

I took a look, and nsIDocument::ShouldThrottleFrameRequests() was indeed returning true for pages that are visible but still loading. This simple change of checking mVisible instead of mIsShowing fixes the issue for me, perhaps it is all that is needed.

Comment on attachment 8687022 [details] [diff] [review] Stop throttling frame requests for loading pages by checking if visible, not isShowing Nice find! I don't understand these two flags even after reading the relevant code and comments. I can't be certain that mVisible is what we want either. bz, could you shed some light or redirect to someone who knows these flags?

mIsShowing starts off false, becomes true when OnPageShow happens and becomes false when OnPageHide happens. So it's false before the initial load completes and when we're in bfcache or unloaded, true otherwise. mVisible starts off true, becomes false when OnPageHide happens, becomes true when OnPageShow happens. So it's false only when we're in bfcache or unloaded. Suggestions on better comments are very welcome. Neither one is affected by being in a background tab or the window being minimized or anything like that. If the intent is actually to check for things like background tab and whatnot, in addition to whether we're in bfcache/unloaded, then you probably want to use nsIDocument::Hidden(), which is the page visibility API boolean for this stuff. Of course at some point we may want to make that API take whether we're visible in the viewport into account, at which point we would probably just base our throttling on nsIDocument::Hidden() and push the logic currently in ShouldThrottleFrameRequests down into nsDocument::GetVisibilityState. Or something like that. Does that help?

(In reply to Boris Zbarsky [:bz] from comment #15) > mIsShowing starts off false, becomes true when OnPageShow happens and > becomes false when OnPageHide happens. So it's false before the initial > load completes and when we're in bfcache or unloaded, true otherwise. > > mVisible starts off true, becomes false when OnPageHide happens, becomes > true when OnPageShow happens. So it's false only when we're in bfcache or > unloaded. Hmm, the comments could definitely be improved here. =\ I'll write up a quick patch. > Neither one is affected by being in a background tab or the window being > minimized or anything like that. If the intent is actually to check for > things like background tab and whatnot, in addition to whether we're in > bfcache/unloaded, then you probably want to use nsIDocument::Hidden(), which > is the page visibility API boolean for this stuff. Of course at some point > we may want to make that API take whether we're visible in the viewport into > account, at which point we would probably just base our throttling on > nsIDocument::Hidden() and push the logic currently in > ShouldThrottleFrameRequests down into nsDocument::GetVisibilityState. Or > something like that. Yep. As you know there has been some discussion about whether to make OnPageHide/OnPageShow take viewport visibility into account for iframes, and I still think it's an avenue worth pursuing, in which case we'd definitely want to do a refactoring like that. The check for being in a background tab and the like is redundant with other refresh driver throttling code, but it seems semantically clear to just check that, so I suspect nsIDocument::Hidden() is our best bet.

Comment on attachment 8687022 [details] [diff] [review] Stop throttling frame requests for loading pages by checking if visible, not isShowing Review of attachment 8687022 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocument.cpp @@ +3718,5 @@ > // Even if we're not visible, a static clone may be, so run at full speed. > return false; > } > > + if (!mVisible) { So this should probably be replaced with a call to Hidden(). Tim, concur?

(That's assuming that Hidden() doesn't have the same problem that mIsShowing does, which should be checked!)

https://hg.mozilla.org/integration/mozilla-inbound/rev/e60fa9ef2762a3c9c6c705b90335dad54dc7832e Bug 1190093 (Comment Tweak) - Add better comments for nsIDocument::mIsShowing and mVisible. r=me DONTBUILD

^ Took the information about mIsShowing and mVisible from comment 15 and added it to nsIDocument.h. Hopefully this will make it easier for people to determine which one to use in the future.

(In reply to Seth Fowler [:seth] [:s2h] from comment #17) > Comment on attachment 8687022 [details] [diff] [review] > Stop throttling frame requests for loading pages by checking if visible, not > isShowing > > Review of attachment 8687022 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/base/nsDocument.cpp > @@ +3718,5 @@ > > // Even if we're not visible, a static clone may be, so run at full speed. > > return false; > > } > > > > + if (!mVisible) { > > So this should probably be replaced with a call to Hidden(). > > Tim, concur? Yeah, that seems correct. (In reply to Seth Fowler [:seth] [:s2h] from comment #18) > (That's assuming that Hidden() doesn't have the same problem that mIsShowing > does, which should be checked!) From reading the code it seems like it shouldn't.

Do you want to update your patch to use Hidden()?

Done. I'm new to Bugzilla so hopefully I've updated the patch in the correct manner.

I pushed the patch to try for you since I'm guessing you don't have try access. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d94d9e1936d

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a1948a077fed09ded935b586f033ff7c92942c1 Backed out changeset 9c8f83fb5930 (bug 1190093) for Gij(11) bustage on a CLOSED TREE

try to confirm it was this patch that caused the failure https://treeherder.mozilla.org/#/jobs?repo=try&revision=0221eeb712cc

Hmm, seems to be a real failure caused by this patch. There are intermittents filed on these failures already, so I'm guessing the patch just changes the timing to make the intermittent's perma-failures. Hubert, I see you've fixed failures in this test (marionette/Player_test.js) before, could you investigate?

I don't think it is the same failure. The heuristic to detect which bug is somewhat approximate so it doesn't turn the intermittent to perma-orange. It just cause a perma orange breakage. Basically the test is waiting for the "iframe" to become active (visible). This is where it is done in the frontend code: https://github.com/mozilla-b2g/gaia/blob/master/apps/music/elements/music-view-stack.js#L199

Thanks for the analysis and the link. I see that it is using requestAnimateFrame to detect when the iframe becomes visible. This patch has the effect of unthrottling rAF during loading. So is it possible that the rAF is now coming too soon and breaking things (because the page is not fully loaded or something like that)?

Referring to justin for that question in comment 31

And here is a try run with the regressing bug (bug 1145439) effectively backed out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3372d388f5b2 Which the test still fails. As soon as I can figure out how, I will be submitting a pull request to disable this test, since it is clearly broken. A broken test should not be blocking us fixing this regression.

I don't think it is a broken test.

The rAF :hub pointed to here -- https://github.com/mozilla-b2g/gaia/blob/master/apps/music/elements/music-view-stack.js#L199 This was primarily put there for robustness to prevent some weird flashes of incorrect styling with the <iframe>s. For example, if you are swapping two views with a cross-fade, it is expected that both <iframes> are aligned on top of each other on-screen and the view you are about to go to starts with an opacity of `0`. Without rAF, its possible that some jank may occur at the beginning of the transition (in my experience).

(In reply to Hubert Figuiere [:hub] from comment #34) > I don't think it is a broken test. Not throttling rAFs is very valid thing for a web engine to do. In fact it was the norm for us for a very long time. And might still be the norm in every other web engine. A test which depends on this behaviour is by definition broken.

Reading these logs, it seems that it fail to find the element via the selector '#tab-bar button[value="songs"]' that should be here. 11:35:39 INFO - at Object.songsTab (/home/worker/gaia/apps/music/test/marionette/lib/music.js:172:31) Said element is created in the html: https://github.com/mozilla-b2g/gaia/blob/master/apps/music/index.html#L81

it also does it for the other tab buttons. Not sure why I thought it was the iframe in comment 30, beside being an off by 1 error :-/

(In reply to Timothy Nikkel (:tn) from comment #31) > Thanks for the analysis and the link. > > I see that it is using requestAnimateFrame to detect when the iframe becomes > visible. This patch has the effect of unthrottling rAF during loading. So is > it possible that the rAF is now coming too soon and breaking things (because > the page is not fully loaded or something like that)? Is it possible that the rAF *never* gets called with this patch? What if the <iframe> where this code is running is hidden at startup? (just thinking out loud)

(In reply to Justin D'Arcangelo [:justindarc] from comment #39) > Is it possible that the rAF *never* gets called with this patch? What if the > <iframe> where this code is running is hidden at startup? (just thinking out > loud) The patch (and especially the try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=3372d388f5b2) should only have the effect of having more rAF calls, not less. (Unless something is *really* broken.)

(In reply to Hubert Figuiere [:hub] from comment #37) > Reading these logs, it seems that it fail to find the element via the > selector '#tab-bar button[value="songs"]' that should be here. The patch of this bug would likely have the effect of a rAF being called sooner (because it is no longer throttled). So maybe the rAF is called too soon and it fails because something isn't initialized that it is expecting? Can we add some logging and push to try with the logging to see what's happening?

Any updates on this? To be honest this bug should be considered as crucial. I personally use auto-scroll to quickly scroll to the bottom of page every day, and this is more than annoying.

This regression fix NEEDS TO LAND. What can we do to get there?

[Tracking Requested - why for this release]: a regression with a simple patch we shouldn't ship any longer than needed. can make firefox appear sluggish when in fact it is not.

Timothy, please land the patch. If the patch doesn't cause any regression in firefox, we should take it and deal with b2g regressions later.

Sorry for the delay here on addressing the NI. Justin is helping dive into the root cause for B2G Music App test issue. Created new bug for this: https://bugzilla.mozilla.org/show_bug.cgi?id=1236992

tnikkel: I need to check if the patch in Bug 1236992 fixes the issue with this patch applied. I'll check it once I get Gecko built tomorrow.

tnikkel: The patch to remove the requestAnimationFrame() in Bug 1236992 has landed. I think we *should* be ok to land this Gecko patch whenever you're ready.

tnikkel: Just a heads up.. This code never executes in Mulet with this patch applied: https://github.com/mozilla-b2g/gaia/blob/master/apps/music/components/gaia-toolbar/gaia-toolbar.js#L46 See the attached screenshot where the bottom toolbar appears truncated with a "...". NOTE: This only happens *sometimes*. This will likely cause a large number of intermittent failures in other parts of Gaia. I have a patch I'm about to land in Bug 1237721 that will work around this issue, but I thought I should bring it to your attention before you re-land this.

(In reply to Justin D'Arcangelo [:justindarc] from comment #49) > tnikkel: Just a heads up.. This code never executes in Mulet with this patch > applied: > > https://github.com/mozilla-b2g/gaia/blob/master/apps/music/components/gaia- > toolbar/gaia-toolbar.js#L46 I don't understand the structure of the gaia js. Are you suggesting there is a bug in platform with the patch from this bug applied? We should get to the bottom of this.

(In reply to Timothy Nikkel (:tnikkel) from comment #50) > (In reply to Justin D'Arcangelo [:justindarc] from comment #49) > > tnikkel: Just a heads up.. This code never executes in Mulet with this patch > > applied: > > > > https://github.com/mozilla-b2g/gaia/blob/master/apps/music/components/gaia- > > toolbar/gaia-toolbar.js#L46 > > I don't understand the structure of the gaia js. Are you suggesting there is > a bug in platform with the patch from this bug applied? We should get to the > bottom of this. Yes. This code is just a web component that uses `requestAnimationFrame()` in the `createdCallback()` to wait till the next frame before calculating if the toolbar should be truncated. However, with this patch applied to Mulet, it never gets called (sometimes) and the toolbar appears truncated. I think this is specific to Mulet though because I haven't seen this behavior when running B2G on a real device. I guess its possible that the device is running slower than Mulet on desktop and maybe that has something to do with it. It might be helpful to get someone who is more familiar with running Gaia on Mulet to look into why this is happening.

(In reply to Justin D'Arcangelo [:justindarc] from comment #51) > Yes. This code is just a web component that uses `requestAnimationFrame()` > in the `createdCallback()` to wait till the next frame before calculating if > the toolbar should be truncated. However, with this patch applied to Mulet, > it never gets called (sometimes) and the toolbar appears truncated. I think > this is specific to Mulet though because I haven't seen this behavior when > running B2G on a real device. I guess its possible that the device is > running slower than Mulet on desktop and maybe that has something to do with > it. It might be helpful to get someone who is more familiar with running > Gaia on Mulet to look into why this is happening. So a rAF isn't calledm after it's set? Are we sure the rAF callback is getting set? Or does the rAF fall to truncate the bar somehow when it is called? Or is it called later than expected?

Seems to have fixed the test failure though https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc8332a00f87 So I'll land when it merges to central/inbound.

Timothy, do you think we should uplift that to 45? Thanks

(In reply to Sylvestre Ledru [:sylvestre] from comment #56) > Timothy, do you think we should uplift that to 45? Thanks Yes, although this will need the gaia fix from bug 1236992 uplifted to. I don't know how uplift of gaia things works.

Comment on attachment 8689379 [details] [diff] [review] Stop throttling frame requests for loading pages by checking Hidden(), not isShowing Approval Request Comment [Feature/regressing bug #]: bug 1145439 [User impact if declined]: requestAnimateFrame will operate at 1fps while page is loading, making animations or anything using it seem slow and janky [Describe test coverage new/current, TreeHerder]: none [Risks and why]: before bug 1145439 we didn't throttle any rAFs. After bug 1145439 we throttled some rAFs, some that we should throttle, and some that we shouldn't. This patch makes us unthrottle the rAFs that we shouldn't be throttling. So it brings us closer to a situation we had before for a long time. The gaia issue I think is a bug in gaia, but no one has actually investigated. [String/UUID change made/needed]: none

Comment on attachment 8689379 [details] [diff] [review] Stop throttling frame requests for loading pages by checking Hidden(), not isShowing OK, let's try that!

I have reproduced this bug with Firefox nightly 42.0a1 (build id:20150801030211)on windows 7(64 bit) I have verified as fixed this bug with Firefox aurora 45.0a2(build id:20160114004007) Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0 [testday-20160115]

[bugday-20160323] Status: RESOLVED,FIXED -> VERIFIED Comments: Test Successful Component: Name Firefox Version 46.0b4 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: The autoscroller is working while page loading is in progress. Test successful. Actual Results: As expected