Bug 165319

Marked as flaky on mac-wk1 in http://trac.webkit.org/projects/webkit/changeset/209250

Running media/controls/track-menu.html before makes the test timeout 100% on Sierra release with WK1: Tools/Scripts/run-webkit-tests --release -1 --no-retry --no-sample-on-timeout --time-out-ms=5000 --child-processes=1 media/controls/track-menu.html media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html

<rdar://problem/30284104>

The root of the issue is that animations are suspended by media/controls/track-menu.html with a call to internals.suspendAnimations(), and that state isn't reset with a call to internals.resumeAnimations(). Then, media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html fails because the selection animation for the tracks panel menu item that is clicked never completes and the delegate to notify that an item in the tracks panel was selected is never fired, which leads to the test failure. We can fix do any of the following to fix the issue: 1. Add a call to internals.resumeAnimations() in media/controls/track-menu.html 2. Audit all usage of internals.suspendAnimations() and ensure tests that use it also call internals.resumeAnimations() 3. Ensure the state set by calling internals.suspendAnimations() in WebCore does not continue past one single page I think #3 is what we should do, it seems dangerous that a state set in one state could alter the behaviour of other tests, especially as the run order with multiple workers is unpredictable. While taking a look at calls to internals.resumeAnimations() in existing tests, I found a doozy, animations/added-while-suspended.html has this line: resume(); // Just in case. Yikes.

AnimationControllerPrivate::suspendAnimations() suspends animations in the current frame's document and its associated subframes, but also sets a global ivar to prevent further animations until AnimationControllerPrivate::resumeAnimations() is called: void AnimationControllerPrivate::suspendAnimations() { if (isSuspended()) return; suspendAnimationsForDocument(m_frame.document()); // Traverse subframes for (Frame* child = m_frame.tree().firstChild(); child; child = child->tree().nextSibling()) child->animation().suspendAnimations(); m_isSuspended = true; } The m_isSuspended flag was added for https://bugs.webkit.org/show_bug.cgi?id=114915. I think the internals method should not call directly into AnimationControllerPrivate::suspendAnimations() like it does now, but rather suspend animations for the current document like it used to pre-114915.

Created attachment 300207 [details] Patch

Comment on attachment 300207 [details] Patch Attachment 300207 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2979452 New failing tests: animations/added-while-suspended.html

Created attachment 300214 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 300207 [details] Patch Attachment 300207 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2979455 New failing tests: animations/added-while-suspended.html

Created attachment 300215 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 300207 [details] Patch Attachment 300207 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2979453 New failing tests: animations/added-while-suspended.html

Created attachment 300216 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 300207 [details] Patch Attachment 300207 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2979473 New failing tests: animations/added-while-suspended.html animations/stop-animation-on-suspend.html

Created attachment 300220 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Created attachment 300307 [details] Patch

Comment on attachment 300307 [details] Patch Attachment 300307 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2984731 New failing tests: imported/blink/fast/frames/navigation-in-pagehide.html fast/loader/unload-mutation-crash.html fast/loader/nested-document-handling.html

Created attachment 300311 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 300307 [details] Patch Attachment 300307 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2984753 New failing tests: imported/blink/fast/frames/navigation-in-pagehide.html fast/loader/unload-mutation-crash.html fast/loader/nested-document-handling.html

Created attachment 300313 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 300307 [details] Patch Attachment 300307 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2984751 New failing tests: imported/blink/fast/frames/navigation-in-pagehide.html fast/loader/unload-mutation-crash.html fast/loader/nested-document-handling.html

Created attachment 300314 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 300307 [details] Patch Attachment 300307 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2984833 New failing tests: imported/blink/fast/frames/navigation-in-pagehide.html animations/stop-animation-on-suspend.html fast/loader/unload-mutation-crash.html fast/loader/nested-document-handling.html

Created attachment 300315 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Created attachment 300317 [details] Patch

Created attachment 300318 [details] Patch

Comment on attachment 300318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300318&action=review > Source/WebCore/testing/Internals.cpp:770 > + for (Frame* child = document->frame()->tree().firstChild(); child; child = child->tree().nextSibling()) > + child->animation().resumeAnimationsForDocument(child->document()); I think this is better: for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) { if (Document* document = frame->document())

Created attachment 300340 [details] Patch for landing

Comment on attachment 300340 [details] Patch for landing Clearing flags on attachment: 300340 Committed r211501: <http://trac.webkit.org/changeset/211501>

All reviewed patches have been landed. Closing bug.

Comment on attachment 300340 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=300340&action=review > Source/WebCore/page/animation/AnimationController.h:78 > + WEBCORE_EXPORT void suspendAnimationsForDocument(Document*); > + WEBCORE_EXPORT void resumeAnimationsForDocument(Document*); > + WEBCORE_EXPORT bool animationsAreSuspendedForDocument(Document*); > + void detachFromDocument(Document*); I don't get these per-document APIs. AnimationController is owned by a Frame, which has a single Document. (Historically AC was on Page, which was a mistake.)

(In reply to comment #30) > I don't get these per-document APIs. AnimationController is owned by a > Frame, which has a single Document. (Historically AC was on Page, which was > a mistake.) But that Frame may have several Documents associated with it due to navigation, which was the root cause of this bug, DRT opening one test that called suspendAnimations() which would suspend animations for further documents unless resumeAnimations() was called.