Bug 206839 – REGRESSION (r255158): http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html is a flaky failure

• Component: New Bugs
• Status: RESOLVED CONFIGURATION CHANGED

[Ryan Haddad]

Layout test http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html is a flaky failure

--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe-actual.txt
@@ -7,15 +7,15 @@
 Received message: frameload
 Checking that requestAnimationFrame is throttled in cross origin frame
 Received message: throttled[cross]: true
-Received message: throttled[same]: false
+Received message: throttled[same]: true
 PASS throttledState['cross'] is "true"
-PASS throttledState['same'] is "false"
+FAIL throttledState['same'] should be false. Was true.
 Interacted with cross-origin frame
 Interacted with same-origin frame
-Received message: throttled[cross]: false
-Received message: throttled[same]: false
-PASS throttledState['cross'] is "false"
-PASS throttledState['same'] is "false"
+Received message: throttled[cross]: true
+Received message: throttled[same]: true
+FAIL throttledState['cross'] should be false. Was true.
+FAIL throttledState['same'] should be false. Was true.
 PASS successfullyParsed is true
 
 TEST COMPLETE

https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Fframe-throttling%2Fraf-throttle-in-cross-origin-subframe.html

[Radar WebKit Bug Importer]

<rdar://problem/58930883>

[Ryan Haddad]

This became flaky after r255158 landed.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 389025 [details]
Patch

[Alexey Proskuryakov]

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?

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 389075 [details]
Patch

[Said Abou-Hallawa]

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

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

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.

[Simon Fraser (smfr)]

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!

[Said Abou-Hallawa]

Created attachment 389098 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 389098 [details]
Patch

Clearing flags on attachment: 389098

Committed r255338: <https://trac.webkit.org/changeset/255338>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jacob Uphoff]

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

[Ryan Haddad]

(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

[Ryan Haddad]

Reverted r255338 for reason:

Introduced flakiness in multiple throttling layout tests

Committed r255388: <https://trac.webkit.org/changeset/255388>

[Ryan Haddad]

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

[Said Abou-Hallawa]

Created attachment 390389 [details]
Patch

[Said Abou-Hallawa]

Created attachment 390395 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 390395 [details]
Patch

r=me but be on the lookout for PLT regressions

[Said Abou-Hallawa]

Created attachment 390415 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 390415 [details]
Patch

Clearing flags on attachment: 390415

Committed r256463: <https://trac.webkit.org/changeset/256463>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

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

[Truitt Savell]

Created attachment 390565 [details]
Test list

[Truitt Savell]

Reverted r256463 for reason:

Caused major flakiness on Mac wk2

Committed r256484: <https://trac.webkit.org/changeset/256484>

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

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