Bug 35992

Summary:

Crash at MediaPlayer::duration()

Product:

WebKit

Reporter:

Hin-Chung Lam <hclam>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, eric.carlson, hclam

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Windows Vista

URL:

http://stewdio.org/pong/

Attachments:

Description	Flags
Patch	none
Patch	eric.carlson: review-
Patch	none

Hin-Chung Lam

Reported 2010-03-10 15:30:35 PST

To reproduce the crash, use Chrome 5.0.342.2 dev. 1. Open http://stewdio.org/pong/ 2. Click Play 3. While there is still sound playing, close the tab 4. Aw snap tab The following stack trace is shown: Call stack: 0x64db296d [chrome.dll - htmlmediaelement.cpp:1042] WebCore::HTMLMediaElement::duration() 0x64db3527 [chrome.dll - htmlmediaelement.cpp:1581] WebCore::HTMLMediaElement::endedPlayback() 0x64db2a54 [chrome.dll - htmlmediaelement.cpp:1137] WebCore::HTMLMediaElement::playInternal() 0x64db2a29 [chrome.dll - htmlmediaelement.cpp:1128] WebCore::HTMLMediaElement::play(bool) 0x64f7cc52 [chrome.dll - v8htmlmediaelement.cpp:322] WebCore::HTMLMediaElementInternal::playCallback 0x65322206 [chrome.dll - builtins.cc:451] v8::internal::HandleApiCallHelper<0> 0x653224de [chrome.dll - builtins.cc:468] v8::internal::Builtin_HandleApiCall The cause of failure is that: 1. Tab is closed 2. HTMLMediaElement::DocumentWillBecomeInactive() is called 3. HTMLMediaElement::m_player is clearer (set to NULL) 4. ... sometime later ... 5. A timer javascript is executed by window.setInterval() 6. The script calls audio.play() which goes into HTMLMediaElement::play() 7. null pointer exception in HTMLMediaElement::duration() when trying to call to it The starts to happen from Chrome revision 37051 which seems to be caused by Changeset 53780 http://trac.webkit.org/changeset/53780. The problem is that in HTMLMediaElement::ended(), the code is changed from: if (!m_player || m_readyState < HAVE_METADATA) return false; To float dur = duration(); if (!m_player || isnan(dur)) return false; It happens that inside duration(), m_player is used without being checked. This crash only happens after m_player is cleared by a document close and the javascript is still active, which doesn't seem to happen in WebKit but only in Chrome.

Attachments
Patch (3.94 KB, patch) 2010-03-10 19:12 PST, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (4.90 KB, patch) 2010-03-11 12:24 PST, Hin-Chung Lam	eric.carlson: review-	Details Formatted Diff Diff
Patch (4.64 KB, patch) 2010-03-11 14:06 PST, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Hin-Chung Lam

Comment 1 2010-03-10 17:53:50 PST

So the cause of this is as follows: 1. Page A opens a video and open a new window with Page B 2. Page B holds a reference to the video element in Page A 3. Page A closes, media engine is destroyed but HTMLMediaElement still alive 4. Page B tries to call that video element, which calls the internal m_player 5. Bang!

Hin-Chung Lam

Comment 2 2010-03-10 19:12:17 PST

Created attachment 50463 [details] Patch

Eric Carlson

Comment 3 2010-03-11 08:12:23 PST

Comment on attachment 50463 [details] Patch > float HTMLMediaElement::duration() const > { > - if (m_readyState >= HAVE_METADATA) > + if (m_player && m_readyState >= HAVE_METADATA) > return m_player->duration(); While this is a sensible change, it shouldn't be necessary to prevent this crash because m_readyState should be HAVE_NOTHING if m_player is NULL. I haven't looked, but I suspect there are more problems waiting to be discovered when the the player has been destroyed and the state hasn't been reset.Can you please reset m_readyState and m_networkState to their default values when the player is destroyed? > Index: LayoutTests/http/tests/media/video-cancel-load.html I would *really* like to see this test reformatted to have some structure (text in the <body>, <script> in <head> etc). I know that many layout tests do not do this, but I find that it is often much easier to understand what a test is supposed to do if has some structure.

Hin-Chung Lam

Comment 4 2010-03-11 11:50:20 PST

(In reply to comment #3) > (From update of attachment 50463 [details]) > > float HTMLMediaElement::duration() const > > { > > - if (m_readyState >= HAVE_METADATA) > > + if (m_player && m_readyState >= HAVE_METADATA) > > return m_player->duration(); > > While this is a sensible change, it shouldn't be necessary to prevent this > crash because m_readyState should be HAVE_NOTHING if m_player is NULL. I > haven't looked, but I suspect there are more problems waiting to be discovered > when the the player has been destroyed and the state hasn't been reset.Can you > please reset m_readyState and m_networkState to their default values when the > player is destroyed? The spec actually doesn't say that we should reset m_readyState to default value, like HAVE_NOTHING. It specifies how m_networkState should be assigned though. Though I think setting m_readyState to HAVE_NOTHING is sensible, but i wasn't sure. Any thoughts? > > > > Index: LayoutTests/http/tests/media/video-cancel-load.html > > I would *really* like to see this test reformatted to have some structure (text > in the <body>, <script> in <head> etc). I know that many layout tests do not do > this, but I find that it is often much easier to understand what a test is > supposed to do if has some structure. Will do.

Hin-Chung Lam

Comment 5 2010-03-11 12:24:45 PST

Created attachment 50529 [details] Patch

Hin-Chung Lam

Comment 6 2010-03-11 12:27:59 PST

Modified the patch and test according to Eric's comments. I'm not very sure about setting m_readyState to HAVE_NOTHING since it isn't in the spec. But testing it with existing tests doesn't seem to reveal a problem.

Eric Carlson

Comment 7 2010-03-11 12:37:38 PST

(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 50463 [details] [details]) > > > float HTMLMediaElement::duration() const > > > { > > > - if (m_readyState >= HAVE_METADATA) > > > + if (m_player && m_readyState >= HAVE_METADATA) > > > return m_player->duration(); > > > > While this is a sensible change, it shouldn't be necessary to prevent this > > crash because m_readyState should be HAVE_NOTHING if m_player is NULL. I > > haven't looked, but I suspect there are more problems waiting to be discovered > > when the the player has been destroyed and the state hasn't been reset.Can you > > please reset m_readyState and m_networkState to their default values when the > > player is destroyed? > > The spec actually doesn't say that we should reset m_readyState to default > value, like HAVE_NOTHING. It specifies how m_networkState should be assigned > though. Though I think setting m_readyState to HAVE_NOTHING is sensible, but i > wasn't sure. Any thoughts? > It does though, HAVE_NOTHING is defined as: No information regarding the media resource is available. No data for the current playback position is available. and we have not data at all if the media engine has been destroyed.

Eric Carlson

Comment 8 2010-03-11 12:54:45 PST

Comment on attachment 50529 [details] Patch 22 function duration() 23 { 24 var d = video.duration; 25 if (count++ >= 10 && window.layoutTestController) 26 layoutTestController.notifyDone(); 27 } I should have noticed this the first time around, but what is magic about 10, what makes you certain that this will work on both fast and slow machines? Can you trigger the crash by getting the duration from an onload handler in the child document (or from a function called via settimout() called from an onload handler)?

Hin-Chung Lam

Comment 9 2010-03-11 13:02:17 PST

The setInterval trick is to simulate the failure, but onload can work better, will change that.

Hin-Chung Lam

Comment 10 2010-03-11 14:06:35 PST

Created attachment 50536 [details] Patch Changed setInterval to setTimeout, onload doesn't work since it runs too quickly and the parent document hasn't changed, so allow 50ms for parent to switch to a different document.

Eric Carlson

Comment 11 2010-03-11 14:26:39 PST

Comment on attachment 50536 [details] Patch Thanks!

Hin-Chung Lam

Comment 12 2010-03-11 14:35:55 PST

Thanks for the review!

WebKit Commit Bot

Comment 13 2010-03-12 11:03:11 PST

Comment on attachment 50536 [details] Patch Clearing flags on attachment: 50536 Committed r55917: <http://trac.webkit.org/changeset/55917>

WebKit Commit Bot

Comment 14 2010-03-12 11:03:16 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.