- Revision
- 255338
- Author
- [email protected]
- Date
- 2020-01-28 22:53:59 -0800 (Tue, 28 Jan 2020)
Log Message
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
Patch by Said Abou-Hallawa <[email protected]> on 2020-01-28
Reviewed by Simon Fraser.
Logging on EWS has shown that in some cases ThrottlingReason::VisuallyIdle
was added to the Page::m_throttlingReasons but it was never removed. If
this happens renderingUpdate and requestAnimationFrame will be throttled
to 10 seconds which leads to test flakiness and timeout.
Currently the throttling reasons of Page or ScriptedAnimationController
can be changed by natural reasons (visibility, layout or device state)
unless Setting::renderingUpdateThrottlingEnabled is disabled. Low power
mode also can be overridden by a specific value set by the test.
To make things clearer, a new OptionSet<ThrottlingReason> will be added
to Page. Its name is m_throttlingReasonsOverrideForTestingMask. The
purpose of adding it is to allow disabling and overriding specific
ThrottlingReasons. The bits of m_throttlingReasonsOverrideForTestingMask
are not actually reasons. They prevent changing the corresponding bits in
Page::m_throttlingReasons by natural reasons. Here are the rules for
setting the bits of m_throttlingReasonsOverrideForTestingMask and
m_throttlingReasons:
-- Settings::renderingUpdateThrottlingEnabled is enabled: All the bits in
m_throttlingReasonsOverrideForTestingMask will be turned off expect
the bit of ThrottlingReason::VisuallyIdle. We need to disable it always
for testing. All the bits in m_throttlingReasons will be cleared.
ThrottlingReason::LowPowerMode will be added to m_throttlingReasons
if the device is already in low power mode.
-- Settings::renderingUpdateThrottlingEnabled is disabled: All the bits
in m_throttlingReasonsOverrideForTestingMask will be turned on. All the
bits in m_throttlingReasons will be cleared.
-- Low power mode is overridden: ThrottlingReason::LowPowerMode is added
to m_throttlingReasonsOverrideForTestingMask. The new overriding value
is set in m_throttlingReasons.
-- Low power mode is cleared: ThrottlingReason::LowPowerMode is removed
from m_throttlingReasonsOverrideForTestingMask. If the device is in
low power mode. ThrottlingReason::LowPowerMode will be added to
m_throttlingReasons
* dom/ScriptedAnimationController.cpp:
(WebCore::ScriptedAnimationController::addThrottlingReason):
(WebCore::ScriptedAnimationController::removeThrottlingReason):
Adding and removing ThrottlingReasons to ScriptedAnimationController will
be controlled by Page::m_throttlingReasonsOverrideForTestingMask. If the
bits of the corresponding reasons are on, no change will be allowed.
(WebCore::ScriptedAnimationController::clearThrottlingReasons):
(WebCore::ScriptedAnimationController::isThrottled const):
The bits in the m_throttlingReasons of Page and ScriptedAnimationController
reflect the state of the throttling. No need to check for the Settings.
(WebCore::ScriptedAnimationController::preferredScriptedAnimationInterval const): Deleted.
* dom/ScriptedAnimationController.h:
(WebCore::ScriptedAnimationController::preferredScriptedAnimationInterval const):
(WebCore::ScriptedAnimationController::addThrottlingReason): Deleted.
(WebCore::ScriptedAnimationController::removeThrottlingReason): Deleted.
* page/Page.cpp:
(WebCore::Page::Page):
Set the initial state of the low power mode throttling.
(WebCore::Page::setLowPowerModeEnabledOverrideForTesting):
This will override the low power mode state or clear it. If it overrides
it, no subsequent change will be allowed.
(WebCore::Page::renderingUpdateThrottlingEnabledChangedForTesting):
This called through changing the Settings from the tests only.
(WebCore::Page::setIsVisuallyIdleInternal):
(WebCore::Page::handleLowModePowerChange):
Prevent changing m_throttlingReasons if the throttling reasons can't be
altered.
(WebCore::Page::isLowPowerModeEnabled const): Deleted.
(WebCore::Page::renderingUpdateThrottlingEnabled const): Deleted.
(WebCore::Page::renderingUpdateThrottlingEnabledChanged): Deleted.
(WebCore::Page::isRenderingUpdateThrottled const): Deleted.
(WebCore::Page::preferredRenderingUpdateInterval const): Deleted.
* page/Page.h:
(WebCore::Page::isLowPowerModeEnabled const):
(WebCore::Page::canUpdateThrottlingReason const):
(WebCore::Page::isRenderingUpdateThrottled const):
(WebCore::Page::preferredRenderingUpdateInterval const):
* page/Settings.yaml:
* page/SettingsBase.cpp:
(WebCore::SettingsBase::renderingUpdateThrottlingEnabledChangedForTesting):
(WebCore::SettingsBase::renderingUpdateThrottlingEnabledChanged): Deleted.
* page/SettingsBase.h:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (255337 => 255338)
--- trunk/Source/WebCore/ChangeLog 2020-01-29 06:46:53 UTC (rev 255337)
+++ trunk/Source/WebCore/ChangeLog 2020-01-29 06:53:59 UTC (rev 255338)
@@ -1,3 +1,99 @@
+2020-01-28 Said Abou-Hallawa <[email protected]>
+
+ 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
+
+ Reviewed by Simon Fraser.
+
+ Logging on EWS has shown that in some cases ThrottlingReason::VisuallyIdle
+ was added to the Page::m_throttlingReasons but it was never removed. If
+ this happens renderingUpdate and requestAnimationFrame will be throttled
+ to 10 seconds which leads to test flakiness and timeout.
+
+ Currently the throttling reasons of Page or ScriptedAnimationController
+ can be changed by natural reasons (visibility, layout or device state)
+ unless Setting::renderingUpdateThrottlingEnabled is disabled. Low power
+ mode also can be overridden by a specific value set by the test.
+
+ To make things clearer, a new OptionSet<ThrottlingReason> will be added
+ to Page. Its name is m_throttlingReasonsOverrideForTestingMask. The
+ purpose of adding it is to allow disabling and overriding specific
+ ThrottlingReasons. The bits of m_throttlingReasonsOverrideForTestingMask
+ are not actually reasons. They prevent changing the corresponding bits in
+ Page::m_throttlingReasons by natural reasons. Here are the rules for
+ setting the bits of m_throttlingReasonsOverrideForTestingMask and
+ m_throttlingReasons:
+
+ -- Settings::renderingUpdateThrottlingEnabled is enabled: All the bits in
+ m_throttlingReasonsOverrideForTestingMask will be turned off expect
+ the bit of ThrottlingReason::VisuallyIdle. We need to disable it always
+ for testing. All the bits in m_throttlingReasons will be cleared.
+ ThrottlingReason::LowPowerMode will be added to m_throttlingReasons
+ if the device is already in low power mode.
+
+ -- Settings::renderingUpdateThrottlingEnabled is disabled: All the bits
+ in m_throttlingReasonsOverrideForTestingMask will be turned on. All the
+ bits in m_throttlingReasons will be cleared.
+
+ -- Low power mode is overridden: ThrottlingReason::LowPowerMode is added
+ to m_throttlingReasonsOverrideForTestingMask. The new overriding value
+ is set in m_throttlingReasons.
+
+ -- Low power mode is cleared: ThrottlingReason::LowPowerMode is removed
+ from m_throttlingReasonsOverrideForTestingMask. If the device is in
+ low power mode. ThrottlingReason::LowPowerMode will be added to
+ m_throttlingReasons
+
+ * dom/ScriptedAnimationController.cpp:
+ (WebCore::ScriptedAnimationController::addThrottlingReason):
+ (WebCore::ScriptedAnimationController::removeThrottlingReason):
+ Adding and removing ThrottlingReasons to ScriptedAnimationController will
+ be controlled by Page::m_throttlingReasonsOverrideForTestingMask. If the
+ bits of the corresponding reasons are on, no change will be allowed.
+
+ (WebCore::ScriptedAnimationController::clearThrottlingReasons):
+ (WebCore::ScriptedAnimationController::isThrottled const):
+ The bits in the m_throttlingReasons of Page and ScriptedAnimationController
+ reflect the state of the throttling. No need to check for the Settings.
+
+ (WebCore::ScriptedAnimationController::preferredScriptedAnimationInterval const): Deleted.
+ * dom/ScriptedAnimationController.h:
+ (WebCore::ScriptedAnimationController::preferredScriptedAnimationInterval const):
+ (WebCore::ScriptedAnimationController::addThrottlingReason): Deleted.
+ (WebCore::ScriptedAnimationController::removeThrottlingReason): Deleted.
+
+ * page/Page.cpp:
+ (WebCore::Page::Page):
+ Set the initial state of the low power mode throttling.
+
+ (WebCore::Page::setLowPowerModeEnabledOverrideForTesting):
+ This will override the low power mode state or clear it. If it overrides
+ it, no subsequent change will be allowed.
+
+ (WebCore::Page::renderingUpdateThrottlingEnabledChangedForTesting):
+ This called through changing the Settings from the tests only.
+
+ (WebCore::Page::setIsVisuallyIdleInternal):
+ (WebCore::Page::handleLowModePowerChange):
+ Prevent changing m_throttlingReasons if the throttling reasons can't be
+ altered.
+
+ (WebCore::Page::isLowPowerModeEnabled const): Deleted.
+ (WebCore::Page::renderingUpdateThrottlingEnabled const): Deleted.
+ (WebCore::Page::renderingUpdateThrottlingEnabledChanged): Deleted.
+ (WebCore::Page::isRenderingUpdateThrottled const): Deleted.
+ (WebCore::Page::preferredRenderingUpdateInterval const): Deleted.
+ * page/Page.h:
+ (WebCore::Page::isLowPowerModeEnabled const):
+ (WebCore::Page::canUpdateThrottlingReason const):
+ (WebCore::Page::isRenderingUpdateThrottled const):
+ (WebCore::Page::preferredRenderingUpdateInterval const):
+ * page/Settings.yaml:
+ * page/SettingsBase.cpp:
+ (WebCore::SettingsBase::renderingUpdateThrottlingEnabledChangedForTesting):
+ (WebCore::SettingsBase::renderingUpdateThrottlingEnabledChanged): Deleted.
+ * page/SettingsBase.h:
+
2020-01-28 Simon Fraser <[email protected]>
Add logging to detect cause of rare crash at RenderLayer::calculateLayerBounds const
Modified: trunk/Source/WebCore/dom/ScriptedAnimationController.cpp (255337 => 255338)
--- trunk/Source/WebCore/dom/ScriptedAnimationController.cpp 2020-01-29 06:46:53 UTC (rev 255337)
+++ trunk/Source/WebCore/dom/ScriptedAnimationController.cpp 2020-01-29 06:53:59 UTC (rev 255338)
@@ -52,13 +52,6 @@
return m_document ? m_document->page() : nullptr;
}
-Seconds ScriptedAnimationController::preferredScriptedAnimationInterval() const
-{
- if (auto* page = this->page())
- return page->renderingUpdateThrottlingEnabled() ? preferredFrameInterval(m_throttlingReasons) : FullSpeedAnimationInterval;
- return FullSpeedAnimationInterval;
-}
-
Seconds ScriptedAnimationController::interval() const
{
if (auto* page = this->page())
@@ -82,10 +75,35 @@
scheduleAnimation();
}
+void ScriptedAnimationController::addThrottlingReason(ThrottlingReason reason)
+{
+ if (auto* page = this->page()) {
+ if (!page->canUpdateThrottlingReason(reason))
+ return;
+ }
+ m_throttlingReasons.add(reason);
+}
+
+void ScriptedAnimationController::removeThrottlingReason(ThrottlingReason reason)
+{
+ if (auto* page = this->page()) {
+ if (!page->canUpdateThrottlingReason(reason))
+ return;
+ }
+ m_throttlingReasons.remove(reason);
+}
+
+void ScriptedAnimationController::clearThrottlingReasons()
+{
+ m_throttlingReasons = { };
+}
+
bool ScriptedAnimationController::isThrottled() const
{
+ if (!m_throttlingReasons.isEmpty())
+ return true;
if (auto* page = this->page())
- return page->renderingUpdateThrottlingEnabled() && (page->isRenderingUpdateThrottled() || !m_throttlingReasons.isEmpty());
+ return page->isRenderingUpdateThrottled();
return false;
}
Modified: trunk/Source/WebCore/dom/ScriptedAnimationController.h (255337 => 255338)
--- trunk/Source/WebCore/dom/ScriptedAnimationController.h 2020-01-29 06:46:53 UTC (rev 255337)
+++ trunk/Source/WebCore/dom/ScriptedAnimationController.h 2020-01-29 06:53:59 UTC (rev 255338)
@@ -61,8 +61,9 @@
void suspend();
void resume();
- void addThrottlingReason(ThrottlingReason reason) { m_throttlingReasons.add(reason); }
- void removeThrottlingReason(ThrottlingReason reason) { m_throttlingReasons.remove(reason); }
+ void addThrottlingReason(ThrottlingReason);
+ void removeThrottlingReason(ThrottlingReason);
+ void clearThrottlingReasons();
WEBCORE_EXPORT bool isThrottled() const;
private:
@@ -69,7 +70,7 @@
ScriptedAnimationController(Document&);
Page* page() const;
- Seconds preferredScriptedAnimationInterval() const;
+ Seconds preferredScriptedAnimationInterval() const { return preferredFrameInterval(m_throttlingReasons); }
bool isThrottledRelativeToPage() const;
bool shouldRescheduleRequestAnimationFrame(DOMHighResTimeStamp) const;
void scheduleAnimation();
Modified: trunk/Source/WebCore/page/Page.cpp (255337 => 255338)
--- trunk/Source/WebCore/page/Page.cpp 2020-01-29 06:46:53 UTC (rev 255337)
+++ trunk/Source/WebCore/page/Page.cpp 2020-01-29 06:53:59 UTC (rev 255338)
@@ -335,6 +335,9 @@
m_corsDisablingPatterns.uncheckedAppend(WTFMove(parsedPattern));
}
m_corsDisablingPatterns.shrinkToFit();
+
+ if (m_lowPowerModeNotifier->isLowPowerModeEnabled())
+ m_throttlingReasons.add(ThrottlingReason::LowPowerMode);
}
Page::~Page()
@@ -1160,20 +1163,18 @@
return !isUtilityPage() && nonUtilityPageCount == 1;
}
-bool Page::isLowPowerModeEnabled() const
+void Page::setLowPowerModeEnabledOverrideForTesting(Optional<bool> isEnabled)
{
- if (m_lowPowerModeEnabledOverrideForTesting)
- return m_lowPowerModeEnabledOverrideForTesting.value();
+ // Remove ThrottlingReason::LowPowerMode so handleLowModePowerChange() can do its work.
+ m_throttlingReasonsOverridenForTesting.remove(ThrottlingReason::LowPowerMode);
- return m_lowPowerModeNotifier->isLowPowerModeEnabled();
+ if (isEnabled) {
+ handleLowModePowerChange(isEnabled.value());
+ m_throttlingReasonsOverridenForTesting.add(ThrottlingReason::LowPowerMode);
+ } else
+ handleLowModePowerChange(m_lowPowerModeNotifier->isLowPowerModeEnabled());
}
-void Page::setLowPowerModeEnabledOverrideForTesting(Optional<bool> isEnabled)
-{
- m_lowPowerModeEnabledOverrideForTesting = isEnabled;
- handleLowModePowerChange(m_lowPowerModeEnabledOverrideForTesting.valueOr(false));
-}
-
void Page::setTopContentInset(float contentInset)
{
if (m_topContentInset == contentInset)
@@ -1377,47 +1378,52 @@
});
}
-bool Page::renderingUpdateThrottlingEnabled() const
+void Page::renderingUpdateThrottlingEnabledChangedForTesting()
{
- return m_settings->renderingUpdateThrottlingEnabled();
-}
+ m_throttlingReasons = { };
-void Page::renderingUpdateThrottlingEnabledChanged()
-{
- renderingUpdateScheduler().adjustRenderingUpdateFrequency();
-}
+ // This function can only be called through changing the Settings from the layout tests.
+ // So disable ThrottlingReason::VisuallyIdle always.
+ m_throttlingReasonsOverridenForTesting = ThrottlingReason::VisuallyIdle;
-bool Page::isRenderingUpdateThrottled() const
-{
- return renderingUpdateThrottlingEnabled() && !m_throttlingReasons.isEmpty();
-}
+ if (m_settings->renderingUpdateThrottlingEnabled()) {
+ if (m_lowPowerModeNotifier->isLowPowerModeEnabled())
+ m_throttlingReasons.add(ThrottlingReason::LowPowerMode);
+ } else {
+ m_throttlingReasonsOverridenForTesting.add({ ThrottlingReason::OutsideViewport, ThrottlingReason::LowPowerMode, ThrottlingReason::NonInteractedCrossOriginFrame });
-Seconds Page::preferredRenderingUpdateInterval() const
-{
- return renderingUpdateThrottlingEnabled() ? preferredFrameInterval(m_throttlingReasons) : FullSpeedAnimationInterval;
+ forEachDocument([] (Document& document) {
+ if (auto* scriptedAnimationController = document.scriptedAnimationController())
+ scriptedAnimationController->clearThrottlingReasons();
+ });
+ }
+
+ renderingUpdateScheduler().adjustRenderingUpdateFrequency();
}
void Page::setIsVisuallyIdleInternal(bool isVisuallyIdle)
{
+ if (!canUpdateThrottlingReason(ThrottlingReason::VisuallyIdle))
+ return;
+
if (isVisuallyIdle == m_throttlingReasons.contains(ThrottlingReason::VisuallyIdle))
return;
m_throttlingReasons = m_throttlingReasons ^ ThrottlingReason::VisuallyIdle;
-
- if (renderingUpdateThrottlingEnabled())
- renderingUpdateScheduler().adjustRenderingUpdateFrequency();
+ renderingUpdateScheduler().adjustRenderingUpdateFrequency();
}
void Page::handleLowModePowerChange(bool isLowPowerModeEnabled)
{
+ if (!canUpdateThrottlingReason(ThrottlingReason::LowPowerMode))
+ return;
+
if (isLowPowerModeEnabled == m_throttlingReasons.contains(ThrottlingReason::LowPowerMode))
return;
m_throttlingReasons = m_throttlingReasons ^ ThrottlingReason::LowPowerMode;
+ renderingUpdateScheduler().adjustRenderingUpdateFrequency();
- if (renderingUpdateThrottlingEnabled())
- renderingUpdateScheduler().adjustRenderingUpdateFrequency();
-
if (!RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled())
mainFrame().animation().updateThrottlingState();
Modified: trunk/Source/WebCore/page/Page.h (255337 => 255338)
--- trunk/Source/WebCore/page/Page.h 2020-01-29 06:46:53 UTC (rev 255337)
+++ trunk/Source/WebCore/page/Page.h 2020-01-29 06:53:59 UTC (rev 255338)
@@ -698,13 +698,13 @@
bool isOnlyNonUtilityPage() const;
bool isUtilityPage() const { return m_isUtilityPage; }
- bool isLowPowerModeEnabled() const;
+ bool isLowPowerModeEnabled() const { return m_throttlingReasons.contains(ThrottlingReason::LowPowerMode); }
WEBCORE_EXPORT void setLowPowerModeEnabledOverrideForTesting(Optional<bool>);
- bool renderingUpdateThrottlingEnabled() const;
- void renderingUpdateThrottlingEnabledChanged();
- bool isRenderingUpdateThrottled() const;
- Seconds preferredRenderingUpdateInterval() const;
+ void renderingUpdateThrottlingEnabledChangedForTesting();
+ bool canUpdateThrottlingReason(ThrottlingReason reason) const { return !m_throttlingReasonsOverridenForTesting.contains(reason); }
+ bool isRenderingUpdateThrottled() const { return !m_throttlingReasons.isEmpty(); }
+ Seconds preferredRenderingUpdateInterval() const { return preferredFrameInterval(m_throttlingReasons); }
WEBCORE_EXPORT void applicationWillResignActive();
WEBCORE_EXPORT void applicationDidEnterBackground();
@@ -969,7 +969,6 @@
std::unique_ptr<PerformanceMonitor> m_performanceMonitor;
std::unique_ptr<LowPowerModeNotifier> m_lowPowerModeNotifier;
- Optional<bool> m_lowPowerModeEnabledOverrideForTesting;
Optional<Navigation> m_navigationToLogWhenVisible;
@@ -1004,6 +1003,7 @@
Vector<UserContentURLPattern> m_corsDisablingPatterns;
OptionSet<ThrottlingReason> m_throttlingReasons;
+ OptionSet<ThrottlingReason> m_throttlingReasonsOverridenForTesting;
};
inline PageGroup& Page::group()
Modified: trunk/Source/WebCore/page/Settings.yaml (255337 => 255338)
--- trunk/Source/WebCore/page/Settings.yaml 2020-01-29 06:46:53 UTC (rev 255337)
+++ trunk/Source/WebCore/page/Settings.yaml 2020-01-29 06:53:59 UTC (rev 255338)
@@ -767,7 +767,7 @@
renderingUpdateThrottlingEnabled:
initial: true
- onChange: renderingUpdateThrottlingEnabledChanged
+ onChange: renderingUpdateThrottlingEnabledChangedForTesting
storageBlockingPolicy:
type: SecurityOrigin::StorageBlockingPolicy
Modified: trunk/Source/WebCore/page/SettingsBase.cpp (255337 => 255338)
--- trunk/Source/WebCore/page/SettingsBase.cpp 2020-01-29 06:46:53 UTC (rev 255337)
+++ trunk/Source/WebCore/page/SettingsBase.cpp 2020-01-29 06:53:59 UTC (rev 255338)
@@ -407,10 +407,10 @@
m_page->hiddenPageCSSAnimationSuspensionStateChanged();
}
-void SettingsBase::renderingUpdateThrottlingEnabledChanged()
+void SettingsBase::renderingUpdateThrottlingEnabledChangedForTesting()
{
if (m_page)
- m_page->renderingUpdateThrottlingEnabledChanged();
+ m_page->renderingUpdateThrottlingEnabledChangedForTesting();
}
void SettingsBase::resourceUsageOverlayVisibleChanged()
Modified: trunk/Source/WebCore/page/SettingsBase.h (255337 => 255338)
--- trunk/Source/WebCore/page/SettingsBase.h 2020-01-29 06:46:53 UTC (rev 255337)
+++ trunk/Source/WebCore/page/SettingsBase.h 2020-01-29 06:53:59 UTC (rev 255338)
@@ -194,7 +194,7 @@
void scrollingPerformanceLoggingEnabledChanged();
void hiddenPageDOMTimerThrottlingStateChanged();
void hiddenPageCSSAnimationSuspensionEnabledChanged();
- void renderingUpdateThrottlingEnabledChanged();
+ void renderingUpdateThrottlingEnabledChangedForTesting();
void resourceUsageOverlayVisibleChanged();
void iceCandidateFilteringEnabledChanged();
#if ENABLE(TEXT_AUTOSIZING)
