[mac] requestAnimationFrame sometimes stuck when page loads in a background ta Requested by jamesr_ on #webkit.
Try loading https://bugs.webkit.org/attachment.cgi?id=122073 in a background tab in a WebKit nightly r104705, sometimes the animation gets stuck and stops advancing.
(In reply to comment #1) > Try loading https://bugs.webkit.org/attachment.cgi?id=122073 in a background tab in a WebKit nightly r104705, sometimes the animation gets stuck and stops advancing. Thanks. If I drag this file to the empty space of the tab bar (thereby opening it in a background tab), it fails to animate. I tried 10 times in a row with the same results. If I then reload the page the animation occurs normally. I think what's happening is that we fail to process the first rAF when starting up in the background and therefore no subsequent rAF calls will ever fire. I'm not sure if this is because we are failing to recognize that an rAF need to be fired when we go out of suspend mode after startup, or if we're failing to come out of suspend mode.
<rdar://problem/11159733>
My debugging is showing me that rAF is in fact getting called constantly, even with the animation in a background tab. Or at least the ScriptedAnimationController is getting serviced constantly. Even when this is happening (visible or in bg), the page isn't updating.
Actually, it is called repeatedly while it is in the bg, and then freezes as soon as it comes to the foreground. It's doing the exact opposite of the intended behaviour :)
Fixing the non-resume is pretty easy. There is another bug which is that bg tabs are never told they are non-visible, and hence the animation is running from the moment they are created. That's not quite true, in WK1 they are never told. In WK2 they are told, but there is not yet a ScriptedAnimationController associated with the Document, so the suspend is ignored.
Created attachment 135480 [details] Patch
This patch should also fix <rdar://problem/11103409> Chris, return from the dead and review this!!! :)
Comment on attachment 135480 [details] Patch Attachment 135480 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12317621
Comment on attachment 135480 [details] Patch Attachment 135480 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12320563
I had a patch series here: https://bugs.webkit.org/show_bug.cgi?id=74232 and here: https://bugs.webkit.org/show_bug.cgi?id=74165 that moved the controller up to Page, which makes a hell of a lot more sense and makes this bugfix easier. I think that Chris Marrin didn't really like it, although I don't remember all the details. I haven't had time to revive it and beef up the tests but we need to do that anyway.
Comment on attachment 135480 [details] Patch Attachment 135480 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12317801
Comment on attachment 135480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135480&action=review You should look at James's patches and decide if their approach is better. > Source/WebCore/dom/Document.cpp:5606 > + if (!page() || page()->scriptedAnimationsSuspended()) > + m_scriptedAnimationController->suspend(); When we finally get a Page, are we doing to resume animations? > Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:440 > + if (m_webPage->windowIsVisible()) > + m_webPage->corePage()->resumeScriptedAnimations(); It's a bit odd that RAF policy (not drawing in background windows) is enforced up here in DrawingArea, and not down in code closer to the RAF implementation.
(In reply to comment #13) > (From update of attachment 135480 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135480&action=review > > You should look at James's patches and decide if their approach is better. I will. > > > Source/WebCore/dom/Document.cpp:5606 > > + if (!page() || page()->scriptedAnimationsSuspended()) > > + m_scriptedAnimationController->suspend(); > > When we finally get a Page, are we doing to resume animations? Yes. This will happen when the Page becomes visible. BTW - it's unlikely there won't be a Page. This is really testing for the case where the Page suspended the animations before the controller existed. > > Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:440 > > + if (m_webPage->windowIsVisible()) > > + m_webPage->corePage()->resumeScriptedAnimations(); > > It's a bit odd that RAF policy (not drawing in background windows) is enforced up here in DrawingArea, and not down in code closer to the RAF implementation. Yeah.
(In reply to comment #11) > I had a patch series here: > https://bugs.webkit.org/show_bug.cgi?id=74232 I just r+ed this one. > and here: > https://bugs.webkit.org/show_bug.cgi?id=74165 > > that moved the controller up to Page, which makes a hell of a lot more sense and makes this bugfix easier. I think that Chris Marrin didn't really like it, although I don't remember all the details. I haven't had time to revive it and beef up the tests but we need to do that anyway. I like this suggestion, so please revive it :) However, I'd like to land what I have now because we're getting a lot of crash reports internally. Yes, it will change a little with your patch but only a couple of lines. Now I'll take a look at why it fails some tests.
(In reply to comment #15) > (In reply to comment #11) > > I had a patch series here: > > https://bugs.webkit.org/show_bug.cgi?id=74232 > > I just r+ed this one. > > > and here: > > https://bugs.webkit.org/show_bug.cgi?id=74165 > > > > that moved the controller up to Page, which makes a hell of a lot more sense and makes this bugfix easier. I think that Chris Marrin didn't really like it, although I don't remember all the details. I haven't had time to revive it and beef up the tests but we need to do that anyway. > > I like this suggestion, so please revive it :) > > However, I'd like to land what I have now because we're getting a lot of crash reports internally. Yes, it will change a little with your patch but only a couple of lines. I approve of this plan! > > Now I'll take a look at why it fails some tests.
I think the test failures on win, qt and gtk are all unrelated to this. But I'm going to resubmit just in case. When testing locally I notice that fast/animation/request-animation-frame-during-modal.html can timeout, even though it works fine in Safari.
(In reply to comment #17) > When testing locally I notice that fast/animation/request-animation-frame-during-modal.html can timeout, even though it works fine in Safari. Ha. Of course this is exactly what this patch is fixing. The rAF callback shouldn't be called when the window is not visible (such as within DRT). The fact that it worked before is a bug. I'll have to work out how to get DRT/WTR to flag this case.
Created attachment 135737 [details] Patch
http://trac.webkit.org/changeset/113381
Build fix for Windows with WK2 http://trac.webkit.org/changeset/113388