Bug 46777 - CRASH at WebCore::HTMLSourceElement::scheduleErrorEvent
Summary: CRASH at WebCore::HTMLSourceElement::scheduleErrorEvent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 51249
  Show dependency treegraph
 
Reported: 2010-09-28 19:28 PDT by Joseph Pecoraro
Modified: 2011-01-24 13:07 PST (History)
10 users (show)

See Also:


Attachments
[PATCH] Clear Load State in userCancelledLoad (1.44 KB, patch)
2010-09-28 19:45 PDT, Joseph Pecoraro
simon.fraser: review+
Details | Formatted Diff | Diff
[PATCH] Another State and ASSERT / Early Return to stop Crashes (1.93 KB, patch)
2010-10-11 11:10 PDT, Joseph Pecoraro
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2010-09-28 19:28:02 PDT
Non-reproducible crash seen at:

Thread 2 Crashed:
0   WebCore                       	0x31c958ec WebCore::HTMLSourceElement::scheduleErrorEvent() (HTMLSourceElement.cpp:94)
1   WebCore                       	0x31a710ac WebCore::HTMLMediaElement::setNetworkState(WebCore::MediaPlayer::NetworkState) (HTMLMediaElement.cpp:852)
2   WebCore                       	0x31c8bfac non-virtual thunk to WebCore::HTMLMediaElement::mediaPlayerNetworkStateChanged(WebCore::MediaPlayer*) + 36
3   WebCore                       	0x31a71010 WebCore::MediaPlayer::networkStateChanged() (MediaPlayer.cpp:607)
4   WebCore                       	0x31a70fac WebCore::MediaPlayerPrivateX::deliverNotification(MediaPlayerProxyNotificationType) (MediaPlayerPrivateX.mm:713)
5   WebCore                       	0x31a70cf4 WebCore::MediaPlayer::deliverNotification(MediaPlayerProxyNotificationType) (MediaPlayer.cpp:556)
6   WebCore                       	0x31a70cbc WebCore::HTMLMediaElement::deliverNotification(MediaPlayerProxyNotificationType) (HTMLMediaElement.cpp:2097)
7   WebKit                        	0x305849ac -[WebPluginController _webPluginContainerPostMediaPlayerNotification:forElement:] (WebPluginController.mm:651)
...
Comment 1 Joseph Pecoraro 2010-09-28 19:35:29 PDT
I believe this could happen if:

  - The markup has a <video> with <source src="unsupported-video.video">
  - ENABLE(PLUGIN_PROXY_FOR_VIDEO) is enabled

Sequence of events:

  - HTMLMediaElement::loadNextSourceChild triggers a load
    - m_loadState = LoadingFromSourceElement
  - Somehow documentWillBecomeInactive or userCancelledLoad is triggered
    - m_currentSourceNode = 0
    - m_readyState = HAVE_NOTHING
  - A web plugin post media player notification comes in and triggers setNetworkState calling:

        if (m_readyState < HAVE_METADATA && m_loadState == LoadingFromSourceElement) {
            m_currentSourceNode->scheduleErrorEvent(); // <-- with a null pointer

I haven't been able to create a test case for this. However, it looks like m_loadState shouldn't
have LoadingFromSourceElement if the current source node is null. In numerous other places
when the currentSourceNode is set to 0 the load state is also changed to WaitingForSource.
I think the case in userCancelledLoad is missing this change.
Comment 2 Joseph Pecoraro 2010-09-28 19:45:33 PDT
Created attachment 69155 [details]
[PATCH] Clear Load State in userCancelledLoad

See comment #1 above for details on how this might have happened and why I think this fix is correct.
Comment 3 Joseph Pecoraro 2010-09-29 14:00:03 PDT
Thanks!

Committed r68682
	M	WebCore/ChangeLog
	M	WebCore/html/HTMLMediaElement.cpp
r68682 = 717390534b65d85cd07cc3df4992c84918a38e9f
http://trac.webkit.org/changeset/68682
Comment 4 Alexey Proskuryakov 2010-09-30 12:52:57 PDT
Hmm, this is the stack trace I saw on test case from bug 46763 in debug mode.

> Thread 2 Crashed:

?!
Comment 5 Joseph Pecoraro 2010-09-30 13:56:28 PDT
(In reply to comment #4)
> Hmm, this is the stack trace I saw on test case from bug 46763 in debug mode.

Really? Thats good to know, because I created that test case on that bug
specifically to try and reproduce _this_ crash. However I never reproduced
it, just always hit that crash. So if that is the case, than although I don't
have a reduction, it reaffirms that this was the correct fix.

If you test with a debug nightly past r68682 can you reproduce the crash?
Comment 6 Joseph Pecoraro 2010-09-30 14:08:16 PDT
Its possible the same fix would need to be applied to:
HTMLMediaElement::prepareForLoad

Setting the m_loadState = WaitingForSource whenever
we set m_currentSourceNode to 0.
Comment 7 Alexey Proskuryakov 2010-09-30 17:41:05 PDT
I'm still seeing this stack trace on the test from bug 46763. These two bugs are in confusing state - something was fixed without a test case, but the one with test case still shows a similar problem to the one that should have been fixed.
Comment 8 Joseph Pecoraro 2010-10-11 11:10:01 PDT
Created attachment 70455 [details]
[PATCH] Another State and ASSERT / Early Return to stop Crashes

This is another patch, to catch a similar unbalanced state. It also adds an ASSERT and early
return to prevent crashes.
Comment 9 Darin Adler 2010-10-11 11:12:10 PDT
Comment on attachment 70455 [details]
[PATCH] Another State and ASSERT / Early Return to stop Crashes

The bug fix looks fine, but I don’t see a test case. Can we create a test case for this?
Comment 10 Joseph Pecoraro 2010-10-11 11:17:35 PDT
(In reply to comment #9)
> (From update of attachment 70455 [details])
> The bug fix looks fine, but I don’t see a test case. Can we create a test case for this?

I haven't been able to reproduce the crash myself. Alexey was able to reproduce it
(with a test case that I created to try to reproduce this) using MallocScribble and a
debug build. I tried to use the same settings and still was not able to reproduce it.
It may be based on timers.
Comment 11 Joseph Pecoraro 2010-10-11 12:03:43 PDT
Committed r69514
	M	WebCore/ChangeLog
	M	WebCore/html/HTMLMediaElement.cpp
r69514 = 41102af89bdf6311aed9cf2d153ab8bc422caea5
Comment 12 Joseph Pecoraro 2010-10-11 12:07:28 PDT
http://trac.webkit.org/changeset/69514
Comment 13 WebKit Review Bot 2010-10-11 14:29:21 PDT
http://trac.webkit.org/changeset/69514 might have broken GTK Linux 32-bit Release
The following tests are not passing:
fast/js/basic-strict-mode.html
Comment 14 Ademar Reis 2011-01-24 07:45:26 PST
Revision r68682 cherry-picked into qtwebkit-2.2 with commit 9211999 <http://gitorious.org/webkit/qtwebkit/commit/9211999>
Comment 15 Ademar Reis 2011-01-24 12:16:57 PST
Revision r68682 cherry-picked into qtwebkit-2.2 with commit 9211999 <http://gitorious.org/webkit/qtwebkit/commit/9211999>
Comment 16 Ademar Reis 2011-01-24 13:07:56 PST
Revision r69514 cherry-picked into qtwebkit-2.2 with commit 3a5ef1b <http://gitorious.org/webkit/qtwebkit/commit/3a5ef1b>