Bug 165319 – [mac-wk1] LayoutTest media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html is a flaky timeout

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

[Ryan Haddad]

LayoutTest media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html is a flaky timeout

This test has been timing out more often than it passes on mac-wk1 since it was added in https://trac.webkit.org/changeset/209227

https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK1%20(Tests)/builds/11875

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=media%2Fmodern-media-controls%2Ftracks-support%2Ftracks-support-click-track-in-panel.html

[Ryan Haddad]

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

[Antoine Quint]

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

[Radar WebKit Bug Importer]

<rdar://problem/30284104>

[Antoine Quint]

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.

[Antoine Quint]

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.

[Antoine Quint]

Created attachment 300207 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Antoine Quint]

Created attachment 300307 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Antoine Quint]

Created attachment 300317 [details]
Patch

[Antoine Quint]

Created attachment 300318 [details]
Patch

[Dean Jackson]

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())

[Antoine Quint]

Created attachment 300340 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 300340 [details]
Patch for landing

Clearing flags on attachment: 300340

Committed r211501: <http://trac.webkit.org/changeset/211501>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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.)

[Antoine Quint]

(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.