Bug 206839

<rdar://problem/58930883>

This became flaky after r255158 landed.

Looking at the diff above, I can see that rAF of each of sub-frames is throttled regardless whether it is cross origin frame or not or whether it is interacted or not. The throttling reasons are the following: 1. VisuallyIdle 2. OutsideViewport 3. LowPowerMode 4. NonInteractedCrossOriginFrame The OutsideViewport and LowPowerMode should not be involved in this test. The page can show the two sub-frames. And the low power mode has to be turned on explicitly from the test which is not happening. NonInteractedCrossOriginFrame should be on for the "cross" frame at the beginning of the test. Then it should be turned off when UIHelper activates it at the end of the test. But it should be off for the "same" frame all the times. This leaves VisuallyIdle as the cause of the throttling. And this can happen only if the page activity state is set to VisuallyIdle. But it is never reset back. So we can fix this issue by forcing the visibility of the page at the beginning of the test by calling Internals::setPageVisibility(). But looking at the code I can see the following: void Internals::setPageVisibility(bool isVisible) { auto* document = contextDocument(); if (!document || !document->page()) return; auto& page = *document->page(); auto state = page.activityState(); if (!isVisible) state.remove(ActivityState::IsVisible); else state.add(ActivityState::IsVisible); page.setActivityState(state); } void Page::setIsVisible(bool isVisible) { auto state = m_activityState; if (isVisible) { state.remove(ActivityState::IsVisuallyIdle); state.add({ ActivityState::IsVisible, ActivityState::IsVisibleOrOccluded }); } else { state.add(ActivityState::IsVisuallyIdle); state.remove({ ActivityState::IsVisible, ActivityState::IsVisibleOrOccluded }); } setActivityState(state); } So the Internals API does not take ActivityState::IsVisuallyIdle into consideration when the visibility state changes. But the Page API does take it into consideration which I think is correct. I think Page::setIsVisible() should be called from Internals::setPageVisibility() to remove this discrepancy and the test itself should call Internals::setPageVisibility() to fix this issue.

Created attachment 389025 [details] Patch

Comment on attachment 389025 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389025&action=review > LayoutTests/http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html:19 > + internals.setPageVisibility(true); Aren't all tests supposed to have "visible" pages? Why is the default opposite to that?

Comment on attachment 389025 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389025&action=review >> LayoutTests/http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html:19 >> + internals.setPageVisibility(true); > > Aren't all tests supposed to have "visible" pages? Why is the default opposite to that? I did not mean to change the visibility of the page. I actually wanted to change the VisuallyIdle state of the page. While investigation the flakiness of https://bugs.webkit.org/show_bug.cgi?id=204713, I put some logging to see what throttling reasons were affecting the tests. https://ews-build.webkit.org/results/macOS-Mojave-Debug-WK1-Tests-EWS/r388698-1396/results.html is one of the runs. See the stderr output of inspector/audit/data-domNodes.html. You will see the following message only Page::setIsVisuallyIdleInternal() adding ThrottlingReason = ThrottlingReason::VisuallyIdle The test inspector/audit/data-domNodes.html ends without removing ThrottlingReason::VisuallyIdle. This means rAF should have been throttled to 10 seconds if it were called from this test. This what made me disable the throttling for all the tests except the throttling tests. This test raf-throttle-in-cross-origin-subframe.html is one of the tests that have to enable throttling. But the VisuallyIdle made it flaky the same way enabling throttling always made many tests flaky.

Created attachment 389075 [details] Patch

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

Comment on attachment 389075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389075&action=review > Source/WebCore/dom/ScriptedAnimationController.cpp:99 > +void ScriptedAnimationController::renderingUpdateThrottlingEnabledChanged() > +{ > + m_throttlingReasons = { }; > +} I think this should just be called clearThrottlingReasons. > Source/WebCore/page/Page.cpp:1168 > + m_throttlingReasonsOverrideForTestingMask.add(ThrottlingReason::LowPowerMode); This seems wrong if enabled is set but false. > Source/WebCore/page/Page.cpp:1382 > + // Currently this function can only be called through changing the Settings by a layout test. > + // So disable ThrottlingReason::VisuallyIdle always. > + m_throttlingReasonsOverrideForTestingMask = ThrottlingReason::VisuallyIdle; This function, Page::renderingUpdateThrottlingEnabledChanged(), is not obviously testing-related but has test-only behavior, so it probably needs to have "ForTesting" in the name. > Source/WebCore/page/Page.cpp:1392 > + scriptedAnimationController->renderingUpdateThrottlingEnabledChanged(); I'm not clear on why just clearing the throttling reasons on the scriptedAnimationControllers is correct. Might that not clobber some previous state set by a test? > Source/WebCore/page/Page.h:1001 > + OptionSet<ThrottlingReason> m_throttlingReasonsOverrideForTestingMask; Let's just call this m_throttlingReasonsOverridenForTesting.

Comment on attachment 389075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389075&action=review >> Source/WebCore/dom/ScriptedAnimationController.cpp:99 >> +} > > I think this should just be called clearThrottlingReasons. Fixed. >> Source/WebCore/page/Page.cpp:1168 >> + m_throttlingReasonsOverrideForTestingMask.add(ThrottlingReason::LowPowerMode); > > This seems wrong if enabled is set but false. My understanding is when isEnabled is set, its value has to override the device state regardless whether it is true or false. Setting ThrottlingReason::LowPowerMode in m_throttlingReasonsOverrideForTestingMask indicates the value of this reason in m_throttlingReasons should not be changed. handleLowModePowerChange() does nothing if canUpdateThrottlingReason(ThrottlingReason::LowPowerMode) is false. And this happens only because of this statement. >> Source/WebCore/page/Page.cpp:1382 >> + m_throttlingReasonsOverrideForTestingMask = ThrottlingReason::VisuallyIdle; > > This function, Page::renderingUpdateThrottlingEnabledChanged(), is not obviously testing-related but has test-only behavior, so it probably needs to have "ForTesting" in the name. Fixed. >> Source/WebCore/page/Page.cpp:1392 >> + scriptedAnimationController->renderingUpdateThrottlingEnabledChanged(); > > I'm not clear on why just clearing the throttling reasons on the scriptedAnimationControllers is correct. Might that not clobber some previous state set by a test? You are right. Clearing the throttling reasons on the scriptedAnimationController should only happen when disabling the renderingUpdateThrottling. I moved the forEachDocument inside the else statement. >> Source/WebCore/page/Page.h:1001 >> + OptionSet<ThrottlingReason> m_throttlingReasonsOverrideForTestingMask; > > Let's just call this m_throttlingReasonsOverridenForTesting. Fixed.

Comment on attachment 389075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389075&action=review >>> Source/WebCore/page/Page.cpp:1168 >>> + m_throttlingReasonsOverrideForTestingMask.add(ThrottlingReason::LowPowerMode); >> >> This seems wrong if enabled is set but false. > > My understanding is when isEnabled is set, its value has to override the device state regardless whether it is true or false. Setting ThrottlingReason::LowPowerMode in m_throttlingReasonsOverrideForTestingMask indicates the value of this reason in m_throttlingReasons should not be changed. handleLowModePowerChange() does nothing if canUpdateThrottlingReason(ThrottlingReason::LowPowerMode) is false. And this happens only because of this statement. Oh, I misread. You're setting the value in the override mask. Fine. But you removed it three lines above!

Created attachment 389098 [details] Patch

Comment on attachment 389098 [details] Patch Clearing flags on attachment: 389098 Committed r255338: <https://trac.webkit.org/changeset/255338>

All reviewed patches have been landed. Closing bug.

After the fix landed for this bug we started getting failures on several throttling test. History: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&test=fast%2Fdom%2Fnested-timer-visible-element-throttling.html&test=fast%2Fdom%2Frepeating-timer-element-overflow-throttling.html&test=fast%2Fdom%2Frepeating-timer-element-overflowing-child-own-layer-throttling.html&test=fast%2Fdom%2Frepeating-timer-mixed-visible-display-none-elements-throttling.html&test=fast%2Fdom%2Frepeating-timer-visible-element-throttling.html&test=fast%2Fdom%2Ftimer-height-change-no-throttling.html

(In reply to Jacob Uphoff from comment #15) > After the fix landed for this bug we started getting failures on several > throttling test. Tracking with https://bugs.webkit.org/show_bug.cgi?id=206947

Reverted r255338 for reason: Introduced flakiness in multiple throttling layout tests Committed r255388: <https://trac.webkit.org/changeset/255388>

Marked the test as flaky in https://trac.webkit.org/changeset/255405/webkit

Created attachment 390389 [details] Patch

Created attachment 390395 [details] Patch

Comment on attachment 390395 [details] Patch r=me but be on the lookout for PLT regressions

Created attachment 390415 [details] Patch

Comment on attachment 390415 [details] Patch Clearing flags on attachment: 390415 Committed r256463: <https://trac.webkit.org/changeset/256463>

All reviewed patches have been landed. Closing bug.

The changes in r256463 has caused severe flakiness in testing on Mac wk2 I was able to reproduce the first group of flakes seen here: https://build.webkit.org/builders/Apple-Catalina-Release-WK2-Tests/builds/2901 by running a test list on a single process. command used: run-webkit-tests --test-list test_list --child-processes 1

Created attachment 390565 [details] Test list

Reverted r256463 for reason: Caused major flakiness on Mac wk2 Committed r256484: <https://trac.webkit.org/changeset/256484>

All the throttling patches were reverted. These test should be fixed now.

Committed r256512: <https://trac.webkit.org/changeset/256512>