Bug 247209

Summary:	LayoutTests follow system power saving settings by default, video tests will time out when the computer is in power saving mode
Product:	WebKit	Reporter:	Alicia Boya García <aboya>
Component:	Tools / Tests	Assignee:	Nobody <webkit-unassigned>
Status:	NEW ---
Severity:	Normal	CC:	ap, bugs-noreply, cdumez, jbedard, mcatanzaro, ryanhaddad, simon.fraser, webkit-bug-importer
Priority:	P2	Keywords:	InRadar
Version:	WebKit Nightly Build
Hardware:	Unspecified
OS:	Unspecified

Description Alicia Boya García 2022-10-28 13:37:01 PDT

You can change your OS settings to "power save" to use less battery (in case of laptops), make your device quieter or just preserve energy.

WebKit reads this setting and prevents video playback not initiated by user gesture in this mode, to save power.

So far, so good. The problem is this is not being overriden in tests. Therefore, virtually any test that depends on mediaElement.play() will fail (often as a timeout) if the computer is in this mode, while passing with no issues if the computer is set to "performance" or any other mode. This is very unexpected, as a developer can very well want to run one or more WebKit LayoutTests in the test runner while using this mode, more often than not, unaware of this mode being engage or even this being an issue in the first place.

I am not the first person to notice testing problems with this feature, and inded there is an API for that: Page::setLowPowerModeEnabledOverrideForTesting(), which is exposed to JavaScript for tests through `internals.setLowPowerModeEnabled(<boolean>)`. This is used by a few LayoutTests to compare behavior between the two modes.

So, if this function exists, why is this a problem? The override is not engaged by default -- that is, the default is neither "power save mode" nor "not power save mode" but rather "follow the operating system", leading to unexpected variance in test results.

This is even written explicitly in Internals::resetToConsistentState(), who calls page.setLowPowerModeEnabledOverrideForTesting(std::nullopt); -- The nullopt means "follow the operating system". Why? I would argue that's not a sane default.

Also, tangentially: as of writing, Internals::resetToConsistentState() is only called from InjectedBundlePage::resetAfterTest(). I could confirm this indeed means it only gets called before the second test in the same worker. This is risky, as any divergence between the values set in Internals::resetToConsistentState() and those WebKit starts with will apply differently to the first test in a given worker. This is a very risky footgun, especially considering the many regular random flakes we have in EWS. Why do we do this? Does it take so much CPU time to reset these values that is worth it having flakes that depend solely on (largely random) worker test dispatch?

Comment 1 Michael Catanzaro 2022-10-28 17:26:41 PDT

(In reply to Alicia Boya García from comment #0)
> This is even written explicitly in Internals::resetToConsistentState(), who
> calls page.setLowPowerModeEnabledOverrideForTesting(std::nullopt); -- The
> nullopt means "follow the operating system". Why? I would argue that's not a
> sane default.

I agree. Let's just change it.
 
> Also, tangentially: as of writing, Internals::resetToConsistentState() is
> only called from InjectedBundlePage::resetAfterTest(). I could confirm this
> indeed means it only gets called before the second test in the same worker.
> This is risky, as any divergence between the values set in
> Internals::resetToConsistentState() and those WebKit starts with will apply
> differently to the first test in a given worker. This is a very risky
> footgun, especially considering the many regular random flakes we have in
> EWS. Why do we do this? Does it take so much CPU time to reset these values
> that is worth it having flakes that depend solely on (largely random) worker
> test dispatch?

That's surely a mistake. We should fix that too. Good catch.

Comment 2 Radar WebKit Bug Importer 2022-11-04 13:37:20 PDT

<rdar://problem/101973448>