Bug 76105 – [mac] requestAnimationFrame sometimes stuck when page loads in a background tab

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[WebKit Review Bot]

[mac] requestAnimationFrame sometimes stuck when page loads in a background ta
Requested by jamesr_ on #webkit.

[James Robinson]

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.

[Chris Marrin]

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

[Radar WebKit Bug Importer]

<rdar://problem/11159733>

[Dean Jackson]

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.

[Dean Jackson]

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

[Dean Jackson]

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.

[Dean Jackson]

Created attachment 135480 [details]
Patch

[Dean Jackson]

This patch should also fix <rdar://problem/11103409>

Chris, return from the dead and review this!!! :)

[Early Warning System Bot]

Comment on attachment 135480 [details]
Patch

Attachment 135480 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12317621

[Build Bot]

Comment on attachment 135480 [details]
Patch

Attachment 135480 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12320563

[James Robinson]

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.

[Gustavo Noronha (kov)]

Comment on attachment 135480 [details]
Patch

Attachment 135480 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12317801

[Simon Fraser (smfr)]

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.

[Dean Jackson]

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

[Dean Jackson]

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

[James Robinson]

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

[Dean Jackson]

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.

[Dean Jackson]

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

[Dean Jackson]

Created attachment 135737 [details]
Patch

[Dean Jackson]

http://trac.webkit.org/changeset/113381

[Dean Jackson]

Build fix for Windows with WK2
http://trac.webkit.org/changeset/113388