Bug 165319 - [mac-wk1] LayoutTest media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html is a flaky timeout
Summary: [mac-wk1] LayoutTest media/modern-media-controls/tracks-support/tracks-suppor...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari Technology Preview
Hardware: Macintosh macOS 10.12
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks: 164784
  Show dependency treegraph
 
Reported: 2016-12-02 11:26 PST by Ryan Haddad
Modified: 2017-03-02 16:34 PST (History)
12 users (show)

See Also:


Attachments
Patch (5.21 KB, patch)
2017-01-31 06:00 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (890.21 KB, application/zip)
2017-01-31 07:06 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (840.17 KB, application/zip)
2017-01-31 07:08 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (1.63 MB, application/zip)
2017-01-31 07:12 PST, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (806.38 KB, application/zip)
2017-01-31 07:30 PST, Build Bot
no flags Details
Patch (11.14 KB, patch)
2017-02-01 01:48 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.07 MB, application/zip)
2017-02-01 02:59 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.02 MB, application/zip)
2017-02-01 03:02 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.83 MB, application/zip)
2017-02-01 03:08 PST, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (1.09 MB, application/zip)
2017-02-01 03:15 PST, Build Bot
no flags Details
Patch (11.19 KB, patch)
2017-02-01 04:06 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (11.68 KB, patch)
2017-02-01 04:24 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (11.70 KB, patch)
2017-02-01 11:17 PST, Antoine Quint
no flags 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-02 11:26:16 PST
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
Comment 1 Ryan Haddad 2016-12-02 11:35:20 PST
Marked as flaky on mac-wk1 in http://trac.webkit.org/projects/webkit/changeset/209250
Comment 2 Antoine Quint 2017-01-31 04:36:29 PST
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
Comment 3 Radar WebKit Bug Importer 2017-01-31 04:51:17 PST
<rdar://problem/30284104>
Comment 4 Antoine Quint 2017-01-31 04:57:24 PST
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.
Comment 5 Antoine Quint 2017-01-31 05:15:36 PST
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.
Comment 6 Antoine Quint 2017-01-31 06:00:29 PST
Created attachment 300207 [details]
Patch
Comment 7 Build Bot 2017-01-31 07:06:30 PST
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
Comment 8 Build Bot 2017-01-31 07:06:35 PST
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 9 Build Bot 2017-01-31 07:08:33 PST
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
Comment 10 Build Bot 2017-01-31 07:08:38 PST
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 11 Build Bot 2017-01-31 07:12:20 PST
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
Comment 12 Build Bot 2017-01-31 07:12:25 PST
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 13 Build Bot 2017-01-31 07:30:25 PST
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
Comment 14 Build Bot 2017-01-31 07:30:29 PST
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
Comment 15 Antoine Quint 2017-02-01 01:48:25 PST
Created attachment 300307 [details]
Patch
Comment 16 Build Bot 2017-02-01 02:59:35 PST
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
Comment 17 Build Bot 2017-02-01 02:59:40 PST
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 18 Build Bot 2017-02-01 03:02:14 PST
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
Comment 19 Build Bot 2017-02-01 03:02:19 PST
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 20 Build Bot 2017-02-01 03:08:16 PST
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
Comment 21 Build Bot 2017-02-01 03:08:21 PST
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 22 Build Bot 2017-02-01 03:15:02 PST
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
Comment 23 Build Bot 2017-02-01 03:15:08 PST
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
Comment 24 Antoine Quint 2017-02-01 04:06:12 PST
Created attachment 300317 [details]
Patch
Comment 25 Antoine Quint 2017-02-01 04:24:29 PST
Created attachment 300318 [details]
Patch
Comment 26 Dean Jackson 2017-02-01 10:24:14 PST
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())
Comment 27 Antoine Quint 2017-02-01 11:17:57 PST
Created attachment 300340 [details]
Patch for landing
Comment 28 WebKit Commit Bot 2017-02-01 11:55:51 PST
Comment on attachment 300340 [details]
Patch for landing

Clearing flags on attachment: 300340

Committed r211501: <http://trac.webkit.org/changeset/211501>
Comment 29 WebKit Commit Bot 2017-02-01 11:56:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Simon Fraser (smfr) 2017-02-01 11:59:30 PST
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.)
Comment 31 Antoine Quint 2017-02-01 12:44:42 PST
(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.