Bug 35992

Summary: Crash at MediaPlayer::duration()
Product: WebKit Reporter: Hin-Chung Lam <hclam>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, hclam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows Vista   
URL: http://stewdio.org/pong/
Attachments:
Description Flags
Patch
none
Patch
eric.carlson: review-
Patch none

Description Hin-Chung Lam 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.
Comment 1 Hin-Chung Lam 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!
Comment 2 Hin-Chung Lam 2010-03-10 19:12:17 PST 
Created attachment 50463 [details]
Patch
Comment 3 Eric Carlson 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.
Comment 4 Hin-Chung Lam 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.
Comment 5 Hin-Chung Lam 2010-03-11 12:24:45 PST 
Created attachment 50529 [details]
Patch
Comment 6 Hin-Chung Lam 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.
Comment 7 Eric Carlson 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.
Comment 8 Eric Carlson 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)?
Comment 9 Hin-Chung Lam 2010-03-11 13:02:17 PST 
The setInterval trick is to simulate the failure, but onload can work better, will change that.
Comment 10 Hin-Chung Lam 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.
Comment 11 Eric Carlson 2010-03-11 14:26:39 PST 
Comment on attachment 50536 [details]
Patch

Thanks!
Comment 12 Hin-Chung Lam 2010-03-11 14:35:55 PST 
Thanks for the review!
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2010-03-12 11:03:16 PST 
All reviewed patches have been landed.  Closing bug.