Bug 76105 - [mac] requestAnimationFrame sometimes stuck when page loads in a background tab
Summary: [mac] requestAnimationFrame sometimes stuck when page loads in a background tab
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-01-11 14:40 PST by WebKit Review Bot
Modified: 2012-04-05 15:42 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 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 James Robinson 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 Chris Marrin 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 Radar WebKit Bug Importer 2012-03-30 16:56:47 PDT
<rdar://problem/11159733>
Comment 4 Dean Jackson 2012-03-30 17:52:29 PDT
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 Dean Jackson 2012-03-30 18:05:31 PDT
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 Dean Jackson 2012-04-03 14:56:19 PDT
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 Dean Jackson 2012-04-03 18:46:03 PDT
Created attachment 135480 [details]
Patch
Comment 8 Dean Jackson 2012-04-03 18:47:28 PDT
This patch should also fix <rdar://problem/11103409>

Chris, return from the dead and review this!!! :)
Comment 9 Early Warning System Bot 2012-04-03 18:56:07 PDT
Comment on attachment 135480 [details]
Patch

Attachment 135480 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12317621
Comment 10 Build Bot 2012-04-03 19:09:54 PDT
Comment on attachment 135480 [details]
Patch

Attachment 135480 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12320563
Comment 11 James Robinson 2012-04-03 20:26:21 PDT
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 Gustavo Noronha (kov) 2012-04-04 02:06:15 PDT
Comment on attachment 135480 [details]
Patch

Attachment 135480 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12317801
Comment 13 Simon Fraser (smfr) 2012-04-04 12:04:18 PDT
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.
Comment 14 Dean Jackson 2012-04-04 15:10:14 PDT
(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.
Comment 15 Dean Jackson 2012-04-04 15:57:19 PDT
(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 James Robinson 2012-04-04 16:17:28 PDT
(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 Dean Jackson 2012-04-04 18:03:32 PDT
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 Dean Jackson 2012-04-04 18:11:50 PDT
(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 Dean Jackson 2012-04-04 18:19:47 PDT
Created attachment 135737 [details]
Patch
Comment 20 Dean Jackson 2012-04-05 14:30:03 PDT
http://trac.webkit.org/changeset/113381
Comment 21 Dean Jackson 2012-04-05 15:42:58 PDT
Build fix for Windows with WK2
http://trac.webkit.org/changeset/113388