Bug 95360 – requestAnimationFrame broken with subframes (DisplayRefreshMonitorManager::registerClient fails to register client)

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Arvid Nilsson]

If there's already one client registered for a specific display ID, any further clients with that display ID will fail to be added, because registerClient only calls ensureMonitorForClient, which only adds the client to the monitor when creating it. If a monitor already existed, it will fail to add the new client and just return the existing monitor.

[Simon Fraser (smfr)]

What are the user-visible effects of this bug?

[Arvid Nilsson]

It looks to me like it would be a problem if you managed to get two ScriptedAnimationController instances to request animation frames. The second one would fail to register with the monitor.

I'll see if I can put together something that manages to do that (it may be impossible due to more high level mechanisms, not sure).

The reason I ran into this is not a specific bug, but development on a new feature for the BlackBerry port, perform accelerated compositing sync on a requestAnimationFrame instead of a 0-timeout timer. The idea is to service all the JS animations first, and then perform the AC sync last, to make sure their most recent animation changes become visible immediately.

[Simon Fraser (smfr)]

So a page with an iframe, and both the parent and iframe doing RAF would see this?

[Arvid Nilsson]

(In reply to comment #3)
> So a page with an iframe, and both the parent and iframe doing RAF would see this?

Yes, I'll try a frameset or iframe testcase as a reduction. So far, this is all in theory :)

[Simon Fraser (smfr)]

Created attachment 161568 [details]
RAF test (to be used in a subframe)

[Simon Fraser (smfr)]

Created attachment 161569 [details]
Test with RAF in main document and iframe

[Simon Fraser (smfr)]

The attached second testcase certainly shows issues.

[Simon Fraser (smfr)]

Safari also feels sluggish after loading these testcases.

[Radar WebKit Bug Importer]

<rdar://problem/12210661>

[Andrew Lo]

taking

[Andrew Lo]

Created attachment 166058 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 166058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166058&action=review

> LayoutTests/fast/animation/request-animation-frame-iframe2.html:23
> +        window.webkitRequestAnimationFrame(function() {
> +            mainFrameCallbackInvoked = true;
> +        }, e);

requestAnimationFrame() does not take an element parameter. That subframe JS should be fixed too.

[Andrew Lo]

Created attachment 166069 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166069 [details]
Patch

Clearing flags on attachment: 166069

Committed r129808: <http://trac.webkit.org/changeset/129808>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 166069 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166069&action=review

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:83
> +    DisplayRefreshMonitorClientSet::iterator it = m_clients.find(client);
> +    if (it != m_clients.end())
> +        return;
> +
>      m_clients.add(client);

This change was not needed. The HashSet::add function already is guaranteed to safely do nothing if the pointer is already in the set.

Please roll this change out, since it all it does is add additional unnecessary code.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:143
> +    it->second.get()->addClient(client);

Should not need to explicitly call get() if we are going to use -> right afterward. Pleas omit the ".get()" here.

[Andrew Lo]

Reopening to attach new patch.

[Andrew Lo]

Created attachment 166118 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166118 [details]
Patch

Clearing flags on attachment: 166118

Committed r129842: <http://trac.webkit.org/changeset/129842>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.