Bug 76105 - [mac] requestAnimationFrame sometimes stuck when page loads in a background tab
: [mac] requestAnimationFrame sometimes stuck when page loads in a background tab
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2012-01-11 14:40 PST by
Modified: 2012-04-05 15:42 PST (History)


Attachments
Patch (7.53 KB, patch)
2012-04-03 18:46 PST, Dean Jackson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.98 KB, patch)
2012-04-04 18:19 PST, Dean Jackson
simon.fraser: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-01-11 14:40:05 PST
[mac] requestAnimationFrame sometimes stuck when page loads in a background ta
Requested by jamesr_ on #webkit.
------- Comment #1 From 2012-01-11 14:41:25 PST -------
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.
------- Comment #2 From 2012-01-12 13:04:52 PST -------
(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.
------- Comment #3 From 2012-03-30 16:56:47 PST -------
<rdar://problem/11159733>
------- Comment #4 From 2012-03-30 17:52:29 PST -------
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.
------- Comment #5 From 2012-03-30 18:05:31 PST -------
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 :)
------- Comment #6 From 2012-04-03 14:56:19 PST -------
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.
------- Comment #7 From 2012-04-03 18:46:03 PST -------
Created an attachment (id=135480) [details]
Patch
------- Comment #8 From 2012-04-03 18:47:28 PST -------
This patch should also fix <rdar://problem/11103409>

Chris, return from the dead and review this!!! :)
------- Comment #9 From 2012-04-03 18:56:07 PST -------
(From update of attachment 135480 [details])
Attachment 135480 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12317621
------- Comment #10 From 2012-04-03 19:09:54 PST -------
(From update of attachment 135480 [details])
Attachment 135480 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12320563
------- Comment #11 From 2012-04-03 20:26:21 PST -------
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 #12 From 2012-04-04 02:06:15 PST -------
(From update of attachment 135480 [details])
Attachment 135480 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12317801
------- Comment #13 From 2012-04-04 12:04:18 PST -------
(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.

> 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.
------- Comment #14 From 2012-04-04 15:10:14 PST -------
(In reply to comment #13)
> (From update of attachment 135480 [details] [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.
------- Comment #15 From 2012-04-04 15:57:19 PST -------
(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.
------- Comment #16 From 2012-04-04 16:17:28 PST -------
(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.
------- Comment #17 From 2012-04-04 18:03:32 PST -------
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.
------- Comment #18 From 2012-04-04 18:11:50 PST -------
(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.
------- Comment #19 From 2012-04-04 18:19:47 PST -------
Created an attachment (id=135737) [details]
Patch
------- Comment #20 From 2012-04-05 14:30:03 PST -------
http://trac.webkit.org/changeset/113381
------- Comment #21 From 2012-04-05 15:42:58 PST -------
Build fix for Windows with WK2
http://trac.webkit.org/changeset/113388