Bug 46777

Summary: CRASH at WebCore::HTMLSourceElement::scheduleErrorEvent
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ademar, ap, darin, ddkilzer, eric.carlson, eric, joepeck, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 51249    
Attachments:
Description Flags
[PATCH] Clear Load State in userCancelledLoad
simon.fraser: review+
[PATCH] Another State and ASSERT / Early Return to stop Crashes eric.carlson: review+

Joseph Pecoraro
Reported 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) ...
Attachments
[PATCH] Clear Load State in userCancelledLoad (1.44 KB, patch)
2010-09-28 19:45 PDT, Joseph Pecoraro 		simon.fraser: review+
DetailsFormatted DiffDiff
[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+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Joseph Pecoraro
Comment 1 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.
Joseph Pecoraro
Comment 2 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.
Joseph Pecoraro
Comment 3 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
Alexey Proskuryakov
Comment 4 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: ?!
Joseph Pecoraro
Comment 5 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?
Joseph Pecoraro
Comment 6 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.
Alexey Proskuryakov
Comment 7 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.
Joseph Pecoraro
Comment 8 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.
Darin Adler
Comment 9 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?
Joseph Pecoraro
Comment 10 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.
Joseph Pecoraro
Comment 11 2010-10-11 12:03:43 PDT
Committed r69514 M WebCore/ChangeLog M WebCore/html/HTMLMediaElement.cpp r69514 = 41102af89bdf6311aed9cf2d153ab8bc422caea5
Joseph Pecoraro
Comment 12 2010-10-11 12:07:28 PDT
http://trac.webkit.org/changeset/69514
WebKit Review Bot
Comment 13 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
Ademar Reis
Comment 14 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>
Ademar Reis
Comment 15 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>
Ademar Reis
Comment 16 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>
Note You need to log in before you can comment on or make changes to this bug.