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
Patch (14.52 KB, patch)
2020-01-28 15:19 PST, Said Abou-Hallawa
no flags
Patch (16.90 KB, patch)
2020-01-28 18:03 PST, Said Abou-Hallawa
no flags
Patch (22.02 KB, patch)
2020-02-11 10:44 PST, Said Abou-Hallawa
no flags
Patch (22.89 KB, patch)
2020-02-11 11:19 PST, Said Abou-Hallawa
no flags
Patch (29.05 KB, patch)
2020-02-11 13:46 PST, Said Abou-Hallawa
no flags
Test list (266.80 KB, text/plain)
2020-02-12 15:00 PST, Truitt Savell
no flags
Radar WebKit Bug Importer
Comment 1 2020-01-27 13:05:49 PST
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
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
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
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.
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
Said Abou-Hallawa
Comment 19 2020-02-11 10:44:28 PST
Said Abou-Hallawa
Comment 20 2020-02-11 11:19:12 PST
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
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
Note You need to log in before you can comment on or make changes to this bug.