Bug 165432

Summary: REGRESSION: media/track LayoutTests are flaky failures
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: MediaAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, dino, esprehn+autocc, glenn, graouts, graouts, jeremyj-wk, kondapallykalyan, sam, simon.fraser
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch graouts: review+

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