Bug 165432 – REGRESSION: media/track LayoutTests are flaky failures

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Ryan Haddad]

media/track LayoutTests are flaky failures

See the failures in these two examples:
https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r209351%20(10119)/results.html
https://build.webkit.org/results/Apple%20Sierra%20Release%20WK1%20(Tests)/r209359%20(1986)/results.html

This console message appears in the diff for many of the tests:

--- /Volumes/Data/slave/elcapitan-debug-tests-wk1/build/layout-test-results/media/track/audio-track-expected.txt
+++ /Volumes/Data/slave/elcapitan-debug-tests-wk1/build/layout-test-results/media/track/audio-track-actual.txt
@@ -1,3 +1,4 @@
+CONSOLE MESSAGE: line 2390: TypeError: undefined is not an object (evaluating 'this.controlsBar.visible')
 
 In-band audio tracks.

[Antoine Quint]

Very strange, those tests shouldn't be running with modern media controls enabled.

[Ryan Haddad]

Same console message appearing here causing media/modern-media-controls/placard-support/placard-support-airplay.html to fail (though, maybe this is more reasonable since it is a modern-media-controls test):

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r209353%20(10120)/results.html

[Antoine Quint]

Can't reproduce the issue with media/track/audio-track.html on a release build.

[Alexey Proskuryakov]

How did you attempt to reproduce, what's the exact command line?

[Antoine Quint]

Tools/Scripts/run-webkit-tests --debug -1 --force media/track/audio-track.html

[Alexey Proskuryakov]

I suspect that there is some global state changed when media controls tests are running. I cannot reproduce with one process, but can very easily reproduce when running tests in parallel.

run-webkit-tests --debug -1 media/modern-media-controls/audio/audio-controls-metrics.html media/modern-media-controls/volume-slider/volume-slider-value.html media/modern-media-controls/volume-slider/volume-slider.html media/modern-media-controls/volume-support/volume-support-media-api-mute.html media/modern-media-controls/volume-support/volume-support-media-api.html media/track/audio-track.html --iter=100 -f --child-processes=2

[Alexey Proskuryakov]

Modern media controls are enabled in DumpRenderTree via a WebPreference, so all the parallel processes are racing to change it! This same mistake is repeated for intersection observer and for pointer lock:

    preferences.intersectionObserverEnabled = options.enableIntersectionObserver;
    preferences.modernMediaControlsEnabled = options.enableModernMediaControls;
    preferences.pointerLockEnabled = options.enablePointerLock;

[Antoine Quint]

Cc'ing Sam who implemented the original system to set runtime flags with HTML comments in tests.

[Simon Fraser (smfr)]

But we turn off autosaving for those WebPreferences. Maybe that doesn't work?

[Simon Fraser (smfr)]

Not sure why we don't set the DRT's -preferences explicitly

[Sam Weinig]

Looks like there are two issues:
1) No one is initializing RuntimeEnabledFeatures::m_areModernMediaControlsEnabled to false.
2) But much more importantly, RenderThemeMac::mediaControlsScript() is caching the first script built. Instead, we need two caches, one for modern controls, one for the old controls.  We need this for the style sheet cached as well.

[Sam Weinig]

Created attachment 296310 [details]
Patch

[Antoine Quint]

Comment on attachment 296310 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=296310&action=review

> Source/WebCore/ChangeLog:10
> +        we can just cached both.

s/cached/cache.

[Sam Weinig]

Committed r209417: <http://trac.webkit.org/changeset/209417>

[Alexey Proskuryakov]

Interesting that this could be addressed without fixing WebPreferences misuse.

The design is still not quite sound though:

1. Applying per-test options in a function named resetWebPreferencesToConsistentValues() is clearly wrong.

2. As DumpRenderTree does write out preferences to disk, it is fragile to rely on them not affecting other processes.

[Alexey Proskuryakov]

Comment on attachment 296310 [details]
Patch

Clearing cq? from a landed patch.

[Simon Fraser (smfr)]

(In reply to comment #15)
> 2. As DumpRenderTree does write out preferences to disk, it is fragile to
> rely on them not affecting other processes.

Can we make it not do so? It looks like we try:
    [[WebPreferences standardPreferences] setAutosaves:NO];