Bug 61336 - REGRESSION: Media elements fail to fire ended event after changing src
Summary: REGRESSION: Media elements fail to fire ended event after changing src
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrew Scherkus
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-23 18:34 PDT by Andrew Scherkus
Modified: 2011-06-03 14:05 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.03 KB, patch)
2011-05-24 11:10 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.27 MB, application/zip)
2011-05-24 11:49 PDT, WebKit Review Bot
no flags Details
Patch (5.85 KB, patch)
2011-05-24 12:41 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff
Patch (4.37 KB, patch)
2011-05-25 10:30 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Scherkus 2011-05-23 18:34:38 PDT
Bisected builds all the way down to a seemingly innocuous change:
http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLMediaElement.cpp?rev=85488

Repro'd on Safari/Mac, Chromium/Mac and Chromium/Linux... I'm wagering it affects all ports since it's seems to be MediaPlayer agnostic.

What was happening is that a call to m_player->seek(0) triggers an immediate call to HTMLMediaElement::mediaPlayerTimeChanged(), which clears the m_sentEndEvent flag.

Without clearing that flag, if someone changes the src inside of an ended event callback, m_sentEndEvent remains true even though the source has changed.

Haven't dug into a fix yet but I'm assuming we're not clearing m_sentEndEvent after changing the src.
Comment 1 Eric Carlson 2011-05-23 21:15:05 PDT
(In reply to comment #0)
> 
> Haven't dug into a fix yet but I'm assuming we're not clearing m_sentEndEvent after changing the src.

Sure enough, it should be cleared in HTMLMediaElement::prepareForLoad where the other internal flags are reset.
Comment 2 Andrew Scherkus 2011-05-24 11:10:00 PDT
Created attachment 94643 [details]
Patch
Comment 3 WebKit Review Bot 2011-05-24 11:48:57 PDT
Comment on attachment 94643 [details]
Patch

Attachment 94643 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8726769

New failing tests:
media/video-ended.html
Comment 4 WebKit Review Bot 2011-05-24 11:49:02 PDT
Created attachment 94654 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 5 Andrew Scherkus 2011-05-24 12:41:58 PDT
Created attachment 94670 [details]
Patch
Comment 6 Andrew Scherkus 2011-05-24 12:42:37 PDT
Updated patch to mark test as slow.

Sadly the test does take ~6 seconds to run since the bug only repros if you don't seek.
Comment 7 Eric Carlson 2011-05-25 07:58:00 PDT
Comment on attachment 94670 [details]
Patch

Six seconds is too long for a layout test. silence.[wav,mpg,m4a,oga] is only one second long, can you use that instead?
Comment 8 Andrew Scherkus 2011-05-25 10:30:58 PDT
Created attachment 94806 [details]
Patch
Comment 9 WebKit Commit Bot 2011-05-25 14:45:55 PDT
The commit-queue encountered the following flaky tests while processing attachment 94806 [details]:

animations/suspend-resume-animation.html bug 48161 (authors: cmarrin@apple.com and simon.fraser@apple.com)
The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2011-05-25 14:47:51 PDT
Comment on attachment 94806 [details]
Patch

Clearing flags on attachment: 94806

Committed r87323: <http://trac.webkit.org/changeset/87323>
Comment 11 WebKit Commit Bot 2011-05-25 14:47:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Ademar Reis 2011-06-03 14:05:39 PDT
Revision r87323 cherry-picked into qtwebkit-2.2 with commit 2c79e55 <http://gitorious.org/webkit/qtwebkit/commit/2c79e55>