Bug 46777 – CRASH at WebCore::HTMLSourceElement::scheduleErrorEvent

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Joseph Pecoraro]

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)
...

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Thanks!

Committed r68682
	M	WebCore/ChangeLog
	M	WebCore/html/HTMLMediaElement.cpp
r68682 = 717390534b65d85cd07cc3df4992c84918a38e9f
http://trac.webkit.org/changeset/68682

[Alexey Proskuryakov]

Hmm, this is the stack trace I saw on test case from bug 46763 in debug mode.

> Thread 2 Crashed:

?!

[Joseph Pecoraro]

(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?

[Joseph Pecoraro]

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.

[Alexey Proskuryakov]

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.

[Joseph Pecoraro]

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.

[Darin Adler]

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?

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

Committed r69514
	M	WebCore/ChangeLog
	M	WebCore/html/HTMLMediaElement.cpp
r69514 = 41102af89bdf6311aed9cf2d153ab8bc422caea5

[Joseph Pecoraro]

http://trac.webkit.org/changeset/69514

[WebKit Review Bot]

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

[Ademar Reis]

Revision r68682 cherry-picked into qtwebkit-2.2 with commit 9211999 <http://gitorious.org/webkit/qtwebkit/commit/9211999>

[Ademar Reis]

Revision r68682 cherry-picked into qtwebkit-2.2 with commit 9211999 <http://gitorious.org/webkit/qtwebkit/commit/9211999>

[Ademar Reis]

Revision r69514 cherry-picked into qtwebkit-2.2 with commit 3a5ef1b <http://gitorious.org/webkit/qtwebkit/commit/3a5ef1b>