Bug 165432 - REGRESSION: media/track LayoutTests are flaky failures
Summary: REGRESSION: media/track LayoutTests are flaky failures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-05 16:18 PST by Ryan Haddad
Modified: 2017-01-18 11:43 PST (History)
11 users (show)

See Also:


Attachments
Patch (6.47 KB, patch)
2016-12-06 12:56 PST, Sam Weinig
graouts: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Haddad 2016-12-05 16:18:34 PST
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.
Comment 1 Antoine Quint 2016-12-05 17:00:28 PST
Very strange, those tests shouldn't be running with modern media controls enabled.
Comment 2 Ryan Haddad 2016-12-05 17:13:09 PST
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
Comment 3 Antoine Quint 2016-12-05 18:18:58 PST
Can't reproduce the issue with media/track/audio-track.html on a release build.
Comment 4 Alexey Proskuryakov 2016-12-05 22:07:49 PST
How did you attempt to reproduce, what's the exact command line?
Comment 5 Antoine Quint 2016-12-05 22:32:16 PST
Tools/Scripts/run-webkit-tests --debug -1 --force media/track/audio-track.html
Comment 6 Alexey Proskuryakov 2016-12-05 22:53:44 PST
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
Comment 7 Alexey Proskuryakov 2016-12-05 22:57:52 PST
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;
Comment 8 Antoine Quint 2016-12-06 07:16:31 PST
Cc'ing Sam who implemented the original system to set runtime flags with HTML comments in tests.
Comment 9 Simon Fraser (smfr) 2016-12-06 08:27:18 PST
But we turn off autosaving for those WebPreferences. Maybe that doesn't work?
Comment 10 Simon Fraser (smfr) 2016-12-06 08:28:38 PST
Not sure why we don't set the DRT's -preferences explicitly
Comment 11 Sam Weinig 2016-12-06 12:43:24 PST
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.
Comment 12 Sam Weinig 2016-12-06 12:56:58 PST
Created attachment 296310 [details]
Patch
Comment 13 Antoine Quint 2016-12-06 13:49:20 PST
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.
Comment 14 Sam Weinig 2016-12-06 13:52:44 PST
Committed r209417: <http://trac.webkit.org/changeset/209417>
Comment 15 Alexey Proskuryakov 2016-12-06 16:08:58 PST
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.
Comment 16 Alexey Proskuryakov 2016-12-06 16:09:17 PST
Comment on attachment 296310 [details]
Patch

Clearing cq? from a landed patch.
Comment 17 Simon Fraser (smfr) 2017-01-18 11:43:20 PST
(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];