WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED CONFIGURATION CHANGED
206839
REGRESSION (
r255158
): http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html is a flaky failure
https://bugs.webkit.org/show_bug.cgi?id=206839
Summary
REGRESSION (r255158): http/tests/frame-throttling/raf-throttle-in-cross-origi...
Ryan Haddad
Reported
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
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2020-01-27 13:05:49 PST
<
rdar://problem/58930883
>
Ryan Haddad
Comment 2
2020-01-27 15:45:18 PST
This became flaky after
r255158
landed.
Said Abou-Hallawa
Comment 3
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.
Said Abou-Hallawa
Comment 4
2020-01-28 10:14:27 PST
Created
attachment 389025
[details]
Patch
Alexey Proskuryakov
Comment 5
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?
Said Abou-Hallawa
Comment 6
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.
Said Abou-Hallawa
Comment 7
2020-01-28 15:19:24 PST
Created
attachment 389075
[details]
Patch
Said Abou-Hallawa
Comment 8
2020-01-28 15:20:50 PST
***
Bug 206860
has been marked as a duplicate of this bug. ***
Simon Fraser (smfr)
Comment 9
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.
Said Abou-Hallawa
Comment 10
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.
Simon Fraser (smfr)
Comment 11
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!
Said Abou-Hallawa
Comment 12
2020-01-28 18:03:08 PST
Created
attachment 389098
[details]
Patch
WebKit Commit Bot
Comment 13
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
>
WebKit Commit Bot
Comment 14
2020-01-28 22:54:07 PST
All reviewed patches have been landed. Closing bug.
Jacob Uphoff
Comment 15
2020-01-29 09:56:59 PST
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
Comment 16
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
Ryan Haddad
Comment 17
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
>
Ryan Haddad
Comment 18
2020-01-29 17:02:42 PST
Marked the test as flaky in
https://trac.webkit.org/changeset/255405/webkit
Said Abou-Hallawa
Comment 19
2020-02-11 10:44:28 PST
Created
attachment 390389
[details]
Patch
Said Abou-Hallawa
Comment 20
2020-02-11 11:19:12 PST
Created
attachment 390395
[details]
Patch
Simon Fraser (smfr)
Comment 21
2020-02-11 11:31:53 PST
Comment on
attachment 390395
[details]
Patch r=me but be on the lookout for PLT regressions
Said Abou-Hallawa
Comment 22
2020-02-11 13:46:11 PST
Created
attachment 390415
[details]
Patch
WebKit Commit Bot
Comment 23
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
>
WebKit Commit Bot
Comment 24
2020-02-12 11:31:19 PST
All reviewed patches have been landed. Closing bug.
Truitt Savell
Comment 25
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
Truitt Savell
Comment 26
2020-02-12 15:00:11 PST
Created
attachment 390565
[details]
Test list
Truitt Savell
Comment 27
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
>
Said Abou-Hallawa
Comment 28
2020-02-13 09:15:29 PST
All the throttling patches were reverted. These test should be fixed now.
Said Abou-Hallawa
Comment 29
2020-02-13 09:21:33 PST
Committed
r256512
: <
https://trac.webkit.org/changeset/256512
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug