Bug 164928 – [GStreamer] Crash in WebCore::HTMLMediaElement::removeAudioTrack

• Component: Media
• Status: RESOLVED WORKSFORME

[Michael Catanzaro]

Web process crash in WebCore::HTMLMediaElement::removeAudioTrack:

Truncated backtrace:
Thread no. 1 (10 frames)
 #0 WTF::RefPtr<WebCore::AudioTrackList>::operator-> at /usr/src/debug/webkitgtk-2.12.3/Source/WTF/wtf/RefPtr.h:69
 #1 WebCore::HTMLMediaElement::removeAudioTrack at /usr/src/debug/webkitgtk-2.12.3/Source/WebCore/html/HTMLMediaElement.cpp:3605
 #2 WebCore::MediaPlayer::removeAudioTrack at /usr/src/debug/webkitgtk-2.12.3/Source/WebCore/platform/graphics/MediaPlayer.cpp:1253
 #3 WebCore::MediaPlayerPrivateGStreamer::notifyPlayerOfAudio at /usr/src/debug/webkitgtk-2.12.3/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:698
 #4 WebCore::MediaPlayerPrivateGStreamer::<lambda()>::operator() at /usr/src/debug/webkitgtk-2.12.3/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:665
 #5 WebCore::MainThreadNotifier<WebCore::MediaPlayerPrivateGStreamerBase::MainThreadNotification>::notify<WebCore::MediaPlayerPrivateGStreamer::audioChangedCallback(WebCore::MediaPlayerPrivateGStreamer*)::<lambda()> > at /usr/src/debug/webkitgtk-2.12.3/Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:42
 #6 WebCore::MediaPlayerPrivateGStreamer::audioChangedCallback at /usr/src/debug/webkitgtk-2.12.3/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:665
 #7 _g_closure_invoke_va at gclosure.c:867
 #10 g_cclosure_marshal_VOID__OBJECTv at gmarshal.c:2102
 #11 _g_closure_invoke_va at gclosure.c:867

We have seven reports of this crash. Full backtrace on the downstream bug.

[Philippe Normand]

Steps to reproduce?

[Philippe Normand]

Looks like a use-after-free, the track is removed from the list and then reused, not sure how that is supposed to work :)

https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp#L727

[Xabier Rodríguez Calvar]

A use case for testing would be interesting

[Michael Catanzaro]

Normally users don't know what causes a crash; this case is no exception.

Note: we're still at only seven reports, so nobody has hit this in Fedora in the past two months.

[Philippe Normand]

Looking at the stack-trace in the downstream bug, it refers to code that was removed in bug 137552 ... So I'll close this issue because the crash should no longer happen. Please re-open otherwise.

[Michael Catanzaro]

(In reply to Philippe Normand from comment #5)
> Looking at the stack-trace in the downstream bug, it refers to code that was
> removed in bug 137552 ... So I'll close this issue because the crash should
> no longer happen. Please re-open otherwise.

The timeline is not right. This crash was reported in late 2016. That bug was closed in 2014. Sometimes frames get omitted from the stack trace; likely the crash is really inside mediaPlayerDidRemoveAudioTrack. The MediaPlayerClient is surely HTMLMediaElement.

I assume the crash must have been happening here:

void HTMLMediaElement::removeAudioTrack(AudioTrack& track)
{
    m_audioTracks->remove(track); // <---
    track.clearClient();
}

But I agree the current code does not match up. I'm not sure when it changed; trac doesn't allow blaming HTMLMediaElement because it is too big, and GitHub just times out.

[Philippe Normand]

Could the fix not be part of the stable release that was crashing?