Bug 206839 - REGRESSION (r255158): http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html is a flaky failure
Summary: REGRESSION (r255158): http/tests/frame-throttling/raf-throttle-in-cross-origi...
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar, LayoutTestFailure
Depends on:
Blocks:
 
Reported: 2020-01-27 13:05 PST by Ryan Haddad
Modified: 2020-02-13 09:21 PST (History)
13 users (show)

See Also:


Attachments
Patch (3.40 KB, patch)
2020-01-28 10:14 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (14.52 KB, patch)
2020-01-28 15:19 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (16.90 KB, patch)
2020-01-28 18:03 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (22.02 KB, patch)
2020-02-11 10:44 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (22.89 KB, patch)
2020-02-11 11:19 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (29.05 KB, patch)
2020-02-11 13:46 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Test list (266.80 KB, text/plain)
2020-02-12 15:00 PST, Truitt Savell
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Haddad 2020-01-27 13:05:20 PST
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
Comment 1 Radar WebKit Bug Importer 2020-01-27 13:05:49 PST
<rdar://problem/58930883>
Comment 2 Ryan Haddad 2020-01-27 15:45:18 PST
This became flaky after r255158 landed.
Comment 3 Said Abou-Hallawa 2020-01-28 10:09:10 PST
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.
Comment 4 Said Abou-Hallawa 2020-01-28 10:14:27 PST
Created attachment 389025 [details]
Patch
Comment 5 Alexey Proskuryakov 2020-01-28 10:41:13 PST
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 6 Said Abou-Hallawa 2020-01-28 11:38:24 PST
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.
Comment 7 Said Abou-Hallawa 2020-01-28 15:19:24 PST
Created attachment 389075 [details]
Patch
Comment 8 Said Abou-Hallawa 2020-01-28 15:20:50 PST
*** Bug 206860 has been marked as a duplicate of this bug. ***
Comment 9 Simon Fraser (smfr) 2020-01-28 16:19:38 PST
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 10 Said Abou-Hallawa 2020-01-28 17:32:16 PST
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 11 Simon Fraser (smfr) 2020-01-28 17:34:57 PST
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!
Comment 12 Said Abou-Hallawa 2020-01-28 18:03:08 PST
Created attachment 389098 [details]
Patch
Comment 13 WebKit Commit Bot 2020-01-28 22:54:05 PST
Comment on attachment 389098 [details]
Patch

Clearing flags on attachment: 389098

Committed r255338: <https://trac.webkit.org/changeset/255338>
Comment 14 WebKit Commit Bot 2020-01-28 22:54:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Ryan Haddad 2020-01-29 10:53:32 PST
(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
Comment 17 Ryan Haddad 2020-01-29 14:49:25 PST
Reverted r255338 for reason:

Introduced flakiness in multiple throttling layout tests

Committed r255388: <https://trac.webkit.org/changeset/255388>
Comment 18 Ryan Haddad 2020-01-29 17:02:42 PST
Marked the test as flaky in https://trac.webkit.org/changeset/255405/webkit
Comment 19 Said Abou-Hallawa 2020-02-11 10:44:28 PST
Created attachment 390389 [details]
Patch
Comment 20 Said Abou-Hallawa 2020-02-11 11:19:12 PST
Created attachment 390395 [details]
Patch
Comment 21 Simon Fraser (smfr) 2020-02-11 11:31:53 PST
Comment on attachment 390395 [details]
Patch

r=me but be on the lookout for PLT regressions
Comment 22 Said Abou-Hallawa 2020-02-11 13:46:11 PST
Created attachment 390415 [details]
Patch
Comment 23 WebKit Commit Bot 2020-02-12 11:31:17 PST
Comment on attachment 390415 [details]
Patch

Clearing flags on attachment: 390415

Committed r256463: <https://trac.webkit.org/changeset/256463>
Comment 24 WebKit Commit Bot 2020-02-12 11:31:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Truitt Savell 2020-02-12 14:59:40 PST
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
Comment 26 Truitt Savell 2020-02-12 15:00:11 PST
Created attachment 390565 [details]
Test list
Comment 27 Truitt Savell 2020-02-12 15:03:41 PST
Reverted r256463 for reason:

Caused major flakiness on Mac wk2

Committed r256484: <https://trac.webkit.org/changeset/256484>
Comment 28 Said Abou-Hallawa 2020-02-13 09:15:29 PST
All the throttling patches were reverted. These test should be fixed now.
Comment 29 Said Abou-Hallawa 2020-02-13 09:21:33 PST
Committed r256512: <https://trac.webkit.org/changeset/256512>