RESOLVED FIXED 35992
Crash at MediaPlayer::duration() 
https://bugs.webkit.org/show_bug.cgi?id=35992
Summary Crash at MediaPlayer::duration()
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
DetailsFormatted DiffDiff
Patch (4.90 KB, patch)
2010-03-11 12:24 PST, Hin-Chung Lam 		eric.carlson: review-
DetailsFormatted DiffDiff
Patch (4.64 KB, patch)
2010-03-11 14:06 PST, Hin-Chung Lam 		no flags
DetailsFormatted DiffDiff
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.