Bug 144718 – Throttle RequestAnimationFrame in subframes that are outside the viewport

• Component: Animations
• Status: RESOLVED FIXED

[Chris Dumez]

Throttle RequestAnimationFrame in subframes that are outside the viewport for performance / power reasons.

Radar: <rdar://problem/20688782>

[Chris Dumez]

Created attachment 252559 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 252559 [details]
Patch

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

r=me but please fix the red EWS.

> Source/WebCore/dom/ScriptedAnimationController.cpp:87
> +    LOG(Animations, "%p - Setting RequestAnimationFrame throttling state to %d in frame", this, isThrottled);

in frame what? Would be nice to know fi this is the main frame.

> Source/WebCore/page/FrameView.cpp:2181
> +        if (auto* scriptedAnimationController = frame().document()->scriptedAnimationController()) {

Just auto, right?

> Source/WebCore/page/FrameView.cpp:2182
> +            bool shouldThrottle = forceThrottling || windowToContents(windowClipRect()).isEmpty();

Since we're calling this just after resumeVisibleImageAnimationsIncludingSubframes(), we've probably computed windowToContents(windowClipRect()) already.

> Source/WebCore/page/FrameView.cpp:2190
> +    for (Frame* childFrame = frame().tree().firstChild(); childFrame; childFrame = childFrame->tree().nextSibling()) {

This makes me wonder if we do things in display:none iframes. I added firstRenderedChild()/nextRenderedSibling() to only hit iframes which are not display:none, but if you have a visible frame running animations, and you display:none it, what happens?

> Source/WebCore/page/FrameView.cpp:2191
> +        if (auto* childView = childFrame->view())

auto?

> Source/WebCore/testing/Internals.cpp:772
> +    auto* scriptedAnimationController = contextDocument()->scriptedAnimationController();

auto?

[Chris Dumez]

Comment on attachment 252559 [details]
Patch

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

>> Source/WebCore/page/FrameView.cpp:2181
>> +        if (auto* scriptedAnimationController = frame().document()->scriptedAnimationController()) {
> 
> Just auto, right?

I actually prefer auto* to make it clear it is a pointer. I have seen Darin make similar comments.

>> Source/WebCore/page/FrameView.cpp:2182
>> +            bool shouldThrottle = forceThrottling || windowToContents(windowClipRect()).isEmpty();
> 
> Since we're calling this just after resumeVisibleImageAnimationsIncludingSubframes(), we've probably computed windowToContents(windowClipRect()) already.

Yes, I'll try and leverage this.

>> Source/WebCore/page/FrameView.cpp:2190
>> +    for (Frame* childFrame = frame().tree().firstChild(); childFrame; childFrame = childFrame->tree().nextSibling()) {
> 
> This makes me wonder if we do things in display:none iframes. I added firstRenderedChild()/nextRenderedSibling() to only hit iframes which are not display:none, but if you have a visible frame running animations, and you display:none it, what happens?

Very good question. I add a test and update the code if needed.

[Chris Dumez]

Created attachment 252605 [details]
Patch

[WebKit Commit Bot]

Attachment 252605 [details] did not pass style-queue:


ERROR: Source/WebCore/page/FrameView.h:602:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/FrameView.cpp:2152:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 252605 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Tests: fast/animation/request-animation-frame-throttle-subframe-display-none.html

Added a new layout test for display: none subframes.

> Source/WebCore/dom/ScriptedAnimationController.cpp:102
> +#if USE(REQUEST_ANIMATION_FRAME_TIMER) && USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)

Should fix red ews.

> Source/WebCore/page/FrameView.cpp:1792
> +    applyRecursively([] (FrameView& frameView, IntRect visibleRect) {

Added a new applyRecursively() utility function so that the frame tree is only traversed once and we avoid redundant windowClipRect() computations.

> Source/WebCore/page/FrameView.cpp:2196
> +    bool shouldThrottle = !frame().ownerRenderer() || visibleRect.isEmpty();

Added this !frame().ownerRenderer() check to make the "display: none" subframe case work. Sadly, this is insufficient for subframes of a "display: none" frame because they get a non-null ownerRenderer. I am planning to look into this in a follow-up.

[Simon Fraser (smfr)]

Comment on attachment 252605 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2157
> +void FrameView::applyRecursively(const std::function<void (FrameView& frameView, IntRect windowClipRect)>& apply)

This name is a bit generic given the windowClipRect code that's here. If this is intended to be the One True Way to recurse through subframes, it should be a bit more generic. We should probably also have an argument, or variant that only hits rendered subframes (firstRenderedChild etc).

[Chris Dumez]

Comment on attachment 252605 [details]
Patch

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

>> Source/WebCore/page/FrameView.cpp:2157
>> +void FrameView::applyRecursively(const std::function<void (FrameView& frameView, IntRect windowClipRect)>& apply)
> 
> This name is a bit generic given the windowClipRect code that's here. If this is intended to be the One True Way to recurse through subframes, it should be a bit more generic. We should probably also have an argument, or variant that only hits rendered subframes (firstRenderedChild etc).

Ideally, I would find a less generic name for this, given the visibleRect argument. Maybe applyRecursivelyGivenVisibleRect()?

[Chris Dumez]

Comment on attachment 252605 [details]
Patch

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

>>> Source/WebCore/page/FrameView.cpp:2157
>>> +void FrameView::applyRecursively(const std::function<void (FrameView& frameView, IntRect windowClipRect)>& apply)
>> 
>> This name is a bit generic given the windowClipRect code that's here. If this is intended to be the One True Way to recurse through subframes, it should be a bit more generic. We should probably also have an argument, or variant that only hits rendered subframes (firstRenderedChild etc).
> 
> Ideally, I would find a less generic name for this, given the visibleRect argument. Maybe applyRecursivelyGivenVisibleRect()?

I don't really need a version that only traverses rendered frames at the moment. Also, if a client wanted to do so, it would be easy enough to do a frame().ownerRenderer() check in the lamdba.

[Chris Dumez]

Created attachment 252623 [details]
Patch

[WebKit Commit Bot]

Attachment 252623 [details] did not pass style-queue:


ERROR: Source/WebCore/page/FrameView.h:600:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/FrameView.cpp:2102:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 252623 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2157
> +void FrameView::applyRecursivelyGivenVisibleRect(const std::function<void (FrameView& frameView, IntRect visibleRect)>& apply)

I renamed this method to make it less generic. Let me know if you have a better naming proposal.

[Chris Dumez]

Created attachment 252679 [details]
Patch

[WebKit Commit Bot]

Attachment 252679 [details] did not pass style-queue:


ERROR: Source/WebCore/page/FrameView.h:600:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/FrameView.cpp:2107:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 252679 [details]
Patch

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

> Source/WebCore/page/FrameView.h:600
> +    void applyRecursivelyGivenVisibleRect(const std::function<void (FrameView& frameView, IntRect visibleRect)>&);

I'd say applyRecursivelyWithVisibleRect.

> Source/WebCore/page/FrameView.h:603
> +    void updateThrottledDOMTimersState(IntRect visibleRect);
> +    void resumeVisibleImageAnimations(IntRect visibleRect);
> +    void updateScriptedAnimationsThrottlingState(IntRect visibleRect);

Are we passing IntRects (32 bytes) by value now?

[Chris Dumez]

Created attachment 252680 [details]
Patch

[WebKit Commit Bot]

Attachment 252680 [details] did not pass style-queue:


ERROR: Source/WebCore/page/FrameView.h:600:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/FrameView.cpp:2107:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 252680 [details]
Patch

Clearing flags on attachment: 252680

Committed r183985: <http://trac.webkit.org/changeset/183985>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 144796

[Andreas Kling]

CRASHING TEST: loader/go-back-to-different-window-size.html

https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r183987%20(11604)/loader/go-back-to-different-window-size-crash-log.txt

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010eba1aba WTFCrash + 42 (Assertions.cpp:321)
1   com.apple.WebCore             	0x0000000110bbd315 WebCore::FrameView::windowClipRect() const + 85 (FrameView.cpp:3277)
2   com.apple.WebCore             	0x0000000110bbb570 WebCore::FrameView::applyRecursivelyWithVisibleRect(std::__1::function<void (WebCore::FrameView&, WebCore::IntRect const&)> const&) + 32 (FrameView.cpp:2109)
3   com.apple.WebCore             	0x0000000110bb4f43 WebCore::FrameView::viewportContentsChanged() + 99 (FrameView.cpp:1766)
4   com.apple.WebCore             	0x0000000110bb4e0f WebCore::FrameView::setFrameRect(WebCore::IntRect const&) + 239 (FrameView.cpp:466)
5   com.apple.WebCore             	0x0000000110b86912 WebCore::FrameLoader::open(WebCore::CachedFrameBase&) + 786 (FrameLoader.cpp:2073)
6   com.apple.WebCore             	0x000000011047f029 WebCore::CachedFrame::open() + 201 (CachedFrame.cpp:219)
7   com.apple.WebCore             	0x000000011048a768 WebCore::CachedPage::restore(WebCore::Page&) + 328 (CachedPage.cpp:87)
8   com.apple.WebCore             	0x0000000110b84da3 WebCore::FrameLoader::commitProvisionalLoad() + 2643 (FrameLoader.cpp:1796)
9   com.apple.WebCore             	0x0000000110b88c17 WebCore::FrameLoader::loadProvisionalItemFromCachedPage() + 295 (FrameLoader.cpp:3093)
10  com.apple.WebCore             	0x0000000110b83185 WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool, WebCore::AllowNavigationToInvalidURL) + 1029 (FrameLoader.cpp:2944)
11  com.apple.WebCore             	0x0000000110b8e36e WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>, WebCore::AllowNavigationToInvalidURL)::$_4::operator()(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool) const + 94 (FrameLoader.cpp:1458)
12  com.apple.WebCore             	0x0000000110b8e2f5 std::__1::__function::__func<WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>, WebCore::AllowNavigationToInvalidURL)::$_4, std::__1::allocator<WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>, WebCore::AllowNavigationToInvalidURL)::$_4>, void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>::operator()(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>&&, bool&&) + 229 (__functional_base:413)
13  com.apple.WebCore             	0x0000000111942077 std::__1::function<void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>::operator()(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool) const + 87 (functional:1755)
14  com.apple.WebCore             	0x0000000111941706 WebCore::PolicyCallback::call(bool) + 134 (PolicyCallback.cpp:95)
15  com.apple.WebCore             	0x0000000111942c9a WebCore::PolicyChecker::continueAfterNavigationPolicy(WebCore::PolicyAction) + 682 (PolicyChecker.cpp:206)
16  com.apple.WebCore             	0x0000000111946ace WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, WebCore::DocumentLoader*, WTF::PassRefPtr<WebCore::FormState>, std::__1::function<void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>)::$_1::operator()(WebCore::PolicyAction) const + 30 (PolicyChecker.cpp:124)
17  com.apple.WebCore             	0x0000000111946a9e std::__1::__function::__func<WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, WebCore::DocumentLoader*, WTF::PassRefPtr<WebCore::FormState>, std::__1::function<void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>)::$_1, std::__1::allocator<WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, WebCore::DocumentLoader*, WTF::PassRefPtr<WebCore::FormState>, std::__1::function<void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>)::$_1>, void (WebCore::PolicyAction)>::operator()(WebCore::PolicyAction&&) + 94 (functional:1370)
18  com.apple.WebKit              	0x000000010b5b615c std::__1::function<void (WebCore::PolicyAction)>::operator()(WebCore::PolicyAction) const + 44 (functional:1755)
19  com.apple.WebKit              	0x000000010b5bc52f WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(WebCore::NavigationAction const&, WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, std::__1::function<void (WebCore::PolicyAction)>) + 431 (WebFrameLoaderClient.cpp:756)
20  com.apple.WebCore             	0x0000000111942922 WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, WebCore::DocumentLoader*, WTF::PassRefPtr<WebCore::FormState>, std::__1::function<void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>) + 1442 (PolicyChecker.cpp:122)
21  com.apple.WebCore             	0x0000000110b82864 WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>, WebCore::AllowNavigationToInvalidURL) + 1668 (FrameLoader.cpp:1457)
22  com.apple.WebCore             	0x0000000110b7f0e6 WebCore::FrameLoader::loadDifferentDocumentItem(WebCore::HistoryItem&, WebCore::FrameLoadType, WebCore::FrameLoader::FormSubmissionCacheLoadPolicy) + 358 (FrameLoader.cpp:3159)
23  com.apple.WebCore             	0x0000000110b89966 WebCore::FrameLoader::loadItem(WebCore::HistoryItem&, WebCore::FrameLoadType) + 166 (FrameLoader.cpp:3245)
24  com.apple.WebCore             	0x0000000110c9ae60 WebCore::HistoryController::recursiveGoToItem(WebCore::HistoryItem&, WebCore::HistoryItem*, WebCore::FrameLoadType) + 96 (HistoryController.cpp:737)
25  com.apple.WebCore             	0x0000000110c9ac15 WebCore::HistoryController::goToItem(WebCore::HistoryItem&, WebCore::FrameLoadType) + 405 (HistoryController.cpp:302)
26  com.apple.WebCore             	0x00000001118ba319 WebCore::Page::goToItem(WebCore::HistoryItem&, WebCore::FrameLoadType) + 201 (Page.cpp:448)
27  com.apple.WebCore             	0x000000011042b3b4 WebCore::BackForwardController::goBackOrForward(int) + 228 (BackForwardController.cpp:78)
28  com.apple.WebCore             	0x000000011184e9e5 WebCore::ScheduledHistoryNavigation::fire(WebCore::Frame&) + 293 (NavigationScheduler.cpp:232)
29  com.apple.WebCore             	0x000000011184ab1a WebCore::NavigationScheduler::timerFired() + 570 (NavigationScheduler.cpp:486)
30  com.apple.WebCore             	0x00000001118511a3 std::__1::__function::__func<std::__1::__bind<void (WebCore::NavigationScheduler::*&)(), WebCore::NavigationScheduler*>, std::__1::allocator<std::__1::__bind<void (WebCore::NavigationScheduler::*&)(), WebCore::NavigationScheduler*> >, void ()>::operator()() + 259 (functional:1370)
31  com.apple.WebCore             	0x00000001102c317a std::__1::function<void ()>::operator()() const + 26 (functional:1755)
32  com.apple.WebCore             	0x00000001102c312c WebCore::Timer::fired() + 28 (Timer.h:134)
33  com.apple.WebCore             	0x00000001121f4e9c WebCore::ThreadTimers::sharedTimerFiredInternal() + 396 (ThreadTimers.cpp:135)
34  com.apple.WebCore             	0x00000001121f4b59 WebCore::ThreadTimers::sharedTimerFired() + 25 (ThreadTimers.cpp:108)
35  com.apple.WebCore             	0x0000000111398d1a WebCore::timerFired(__CFRunLoopTimer*, void*) + 42 (SharedTimerCF.cpp:83)
36  com.apple.CoreFoundation      	0x00007fff8b2a13e4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
37  com.apple.CoreFoundation      	0x00007fff8b2a0f1f __CFRunLoopDoTimer + 1151
38  com.apple.CoreFoundation      	0x00007fff8b3125aa __CFRunLoopDoTimers + 298
39  com.apple.CoreFoundation      	0x00007fff8b25c6a5 __CFRunLoopRun + 1525
40  com.apple.CoreFoundation      	0x00007fff8b25be75 CFRunLoopRunSpecific + 309
41  com.apple.HIToolbox           	0x00007fff85e17a0d RunCurrentEventLoopInMode + 226
42  com.apple.HIToolbox           	0x00007fff85e177b7 ReceiveNextEventCommon + 479
43  com.apple.HIToolbox           	0x00007fff85e175bc _BlockUntilNextEventMatchingListInModeWithFilter + 65
44  com.apple.AppKit              	0x00007fff84c9224e _DPSNextEvent + 1434
45  com.apple.AppKit              	0x00007fff84c9189b -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 122
46  com.apple.AppKit              	0x00007fff84c8599c -[NSApplication run] + 553
47  com.apple.AppKit              	0x00007fff84c70783 NSApplicationMain + 940
48  com.apple.XPCService          	0x00007fff90021c0f _xpc_main + 385
49  libxpc.dylib                  	0x00007fff901e0bde xpc_main + 399
50  com.apple.WebKit.WebContent.Development	0x0000000107393195 main + 37
51  libdyld.dylib                 	0x00007fff8709d5fd start + 1

[Andreas Kling]

Also broke some tests on Windows:

https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r183985%20(51686)/results.html

[Chris Dumez]

(In reply to comment #22)
> Also broke some tests on Windows:
> 
> https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/
> r183985%20(51686)/results.html

Yes, those are the 2 new tests my patch is adding. They are timing out on Windows, probably due to DRT limitations as they are passing elsewhere and the code is cross-platform.

[Chris Dumez]

(In reply to comment #23)
> (In reply to comment #22)
> > Also broke some tests on Windows:
> > 
> > https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/
> > r183985%20(51686)/results.html
> 
> Yes, those are the 2 new tests my patch is adding. They are timing out on
> Windows, probably due to DRT limitations as they are passing elsewhere and
> the code is cross-platform.

It is because USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) is only enabled on COCOA.

[Chris Dumez]

Created attachment 252727 [details]
Patch

[WebKit Commit Bot]

Attachment 252727 [details] did not pass style-queue:


ERROR: Source/WebCore/page/FrameView.h:600:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/FrameView.cpp:2107:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 252728 [details]
Patch

[WebKit Commit Bot]

Attachment 252728 [details] did not pass style-queue:


ERROR: Source/WebCore/page/FrameView.h:600:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/page/FrameView.cpp:2107:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 252728 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:2076
> -    if (m_frame.view())
> -        view->setFrameRect(m_frame.view()->frameRect());
> +    Optional<IntRect> previousViewFrameRect = m_frame.view() ?  m_frame.view()->frameRect() : Optional<IntRect>(Nullopt);
>      m_frame.setView(view);
> +
> +    // Use the previous ScrollView's frame rect.
> +    if (previousViewFrameRect)
> +        view->setFrameRect(previousViewFrameRect.value());

I'm slightly nervous about behavior changes here. Any code that consulted the view's frameRect inside of setView() is now getting an empty rect (but there doesn't seem to be much).

FYI, I added FrameView::isViewForDocumentInFrame() to detect the state of switching views.

[WebKit Commit Bot]

Comment on attachment 252728 [details]
Patch

Clearing flags on attachment: 252728

Committed r183998: <http://trac.webkit.org/changeset/183998>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

*** Bug 165695 has been marked as a duplicate of this bug. ***

[Chris Dumez]

*** Bug 100257 has been marked as a duplicate of this bug. ***