1095251 - Determine what keeps MediaDecoder alive in GStreamer callbacks

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Karl Tomlinson (:karlt)]

MDReaders have a bare pointer to their MediaDecoders, which are used in
GStreamer callbacks.

If this pointer is cleared in MediaDecoderReader::BreakCycles(), immediately
before MediaDecoderStateMachine releases its reference to the MediaDecoder in
BreakCycles(), then mochitests crash in GStreamerReader::ReadAndPushData()
GStreamerReader::Eos() trying to use mDecoder.

 MediaDecoderReader::BreakCycles()
  {
    if (mSampleDecodedCallback) {
      mSampleDecodedCallback->BreakCycles();
      mSampleDecodedCallback = nullptr;
    }
    mTaskQueue = nullptr;
 +  mDecoder = nullptr;
  }

BreakCycles is very late in MDSM shutdown.  I would have expected all decode
threads to have completed at this point, or at least ceased assuming that the
MediaDecoder still exists.

Comment 1

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Got a stack?

Comment 2

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2967307&repo=try
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2967321&repo=try

Comment 3

[Karl Tomlinson (:karlt)]

A typical stack trace, with source files having minor changes from 58e4264903ba is

#1  0x00007ff61782f5d8 in mozilla::GStreamerReader::SeekData (
    this=0x7ff578a5c800, aSrc=0x7ff5d4273c50, aOffset=83700)
    at /mnt/ssd1/karl/moz/dev/dom/media/gstreamer/GStreamerReader.cpp:977
#2  0x00007ff61782f58b in mozilla::GStreamerReader::SeekDataCb (
    aSrc=0x7ff5d4273c50, aOffset=83700, aUserData=0x7ff578a5c800)
    at /mnt/ssd1/karl/moz/dev/dom/media/gstreamer/GStreamerReader.cpp:969
#3  0x00007ff5949062a8 in gst_app_src_create ()
   from /usr/lib64/libgstapp-0.10.so.0
#4  0x00007ff5946d451e in gst_base_src_get_range ()
   from /usr/lib64/libgstbase-0.10.so.0
#5  0x00007ff5946d56d1 in gst_base_src_pad_get_range ()
   from /usr/lib64/libgstbase-0.10.so.0
#6  0x00007ff594b6a4c7 in gst_pad_get_range_unchecked ()
   from /usr/lib64/libgstreamer-0.10.so.0
#7  0x00007ff594b6f3e9 in gst_pad_pull_range ()
   from /usr/lib64/libgstreamer-0.10.so.0
#8  0x00007ff594b6a4c7 in gst_pad_get_range_unchecked ()
   from /usr/lib64/libgstreamer-0.10.so.0
#9  0x00007ff594b6f3e9 in gst_pad_pull_range ()
   from /usr/lib64/libgstreamer-0.10.so.0
#10 0x00007ff59377041f in gst_type_find_element_getrange ()
   from /usr/lib64/gstreamer-0.10/libgstcoreelements.so
#11 0x00007ff594b6a4c7 in gst_pad_get_range_unchecked ()
   from /usr/lib64/libgstreamer-0.10.so.0
#12 0x00007ff594b6f3e9 in gst_pad_pull_range ()
   from /usr/lib64/libgstreamer-0.10.so.0
#13 0x00007ff5897ac11a in gst_qtdemux_pull_atom ()
   from /usr/lib64/gstreamer-0.10/libgstisomp4.so
#14 0x00007ff5897c1526 in gst_qtdemux_loop ()
   from /usr/lib64/gstreamer-0.10/libgstisomp4.so
#15 0x00007ff594b94906 in gst_task_func ()
   from /usr/lib64/libgstreamer-0.10.so.0
#16 0x00007ff60fc0348e in g_thread_pool_thread_proxy ()
   from /usr/lib64/libglib-2.0.so.0
#17 0x00007ff60fc02add in g_thread_proxy () from /usr/lib64/libglib-2.0.so.0
#18 0x00007ff61dc6227a in start_thread () from /lib64/libpthread.so.0
#19 0x00007ff61cf7167d in clone ()
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

Comment 4

[Karl Tomlinson (:karlt)]

The reason the decoder is still alive is that nsDecoderDisposeEvent releases
its reference to mStateMachine (on the main thread) before its reference to
mDecoder, mStateMachine holds the last reference to the reader, and
~GStreamerReader joins GStreamer threads that run callbacks.

I'd like to clean up mPlayBin earlier than ~GStreamerReader in case something
still holds a reference to the reader, which has a bare pointer to the
decoder, which is separately owned.

#0  pthread_cond_wait ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:185
#1  0x00007ff60fc20a98 in g_cond_wait () from /usr/lib64/libglib-2.0.so.0
#2  0x00007ff594b956f5 in gst_task_join ()
   from /usr/lib64/libgstreamer-0.10.so.0
#3  0x00007ff594b70afa in gst_pad_stop_task ()
   from /usr/lib64/libgstreamer-0.10.so.0
#4  0x00007ff593760597 in gst_queue_src_activate_push ()
   from /usr/lib64/gstreamer-0.10/libgstcoreelements.so
#5  0x00007ff594b6dc9b in gst_pad_activate_push ()
   from /usr/lib64/libgstreamer-0.10.so.0
#6  0x00007ff594b6e890 in gst_pad_set_active ()
   from /usr/lib64/libgstreamer-0.10.so.0
#7  0x00007ff594b4f8f1 in activate_pads ()
   from /usr/lib64/libgstreamer-0.10.so.0
#8  0x00007ff594b612cd in gst_iterator_fold ()
   from /usr/lib64/libgstreamer-0.10.so.0
#9  0x00007ff594b4f853 in iterator_activate_fold_with_resync ()
   from /usr/lib64/libgstreamer-0.10.so.0
#10 0x00007ff594b51e86 in gst_element_pads_activate ()
   from /usr/lib64/libgstreamer-0.10.so.0
#11 0x00007ff594b52215 in gst_element_change_state_func ()
   from /usr/lib64/libgstreamer-0.10.so.0
#12 0x00007ff594b53efc in gst_element_change_state ()
   from /usr/lib64/libgstreamer-0.10.so.0
#13 0x00007ff594b54832 in gst_element_set_state_func ()
   from /usr/lib64/libgstreamer-0.10.so.0
#14 0x00007ff594b413d3 in gst_bin_change_state_func ()
   from /usr/lib64/libgstreamer-0.10.so.0
#15 0x00007ff594b53efc in gst_element_change_state ()
   from /usr/lib64/libgstreamer-0.10.so.0
#16 0x00007ff594b54832 in gst_element_set_state_func ()
   from /usr/lib64/libgstreamer-0.10.so.0
#17 0x00007ff594b413d3 in gst_bin_change_state_func ()
   from /usr/lib64/libgstreamer-0.10.so.0
#18 0x00007ff593dd5e7c in gst_play_sink_change_state ()
   from /usr/lib64/gstreamer-0.10/libgstplaybin.so
#19 0x00007ff594b53efc in gst_element_change_state ()
   from /usr/lib64/libgstreamer-0.10.so.0
#20 0x00007ff594b54832 in gst_element_set_state_func ()
   from /usr/lib64/libgstreamer-0.10.so.0
#21 0x00007ff594b413d3 in gst_bin_change_state_func ()
   from /usr/lib64/libgstreamer-0.10.so.0
#22 0x00007ff594b722f3 in gst_pipeline_change_state ()
   from /usr/lib64/libgstreamer-0.10.so.0
#23 0x00007ff593dcf48d in gst_play_bin_change_state ()
   from /usr/lib64/gstreamer-0.10/libgstplaybin.so
#24 0x00007ff594b53efc in gst_element_change_state ()
   from /usr/lib64/libgstreamer-0.10.so.0
#25 0x00007ff594b53f7f in gst_element_change_state ()
   from /usr/lib64/libgstreamer-0.10.so.0
#26 0x00007ff594b54832 in gst_element_set_state_func ()
   from /usr/lib64/libgstreamer-0.10.so.0
#27 0x00007ff61782c732 in mozilla::GStreamerReader::~GStreamerReader (
    this=0x7ff578a5c800, __in_chrg=<optimized out>)
    at /mnt/ssd1/karl/moz/dev/dom/media/gstreamer/GStreamerReader.cpp:125
#28 0x00007ff61782c822 in mozilla::GStreamerReader::~GStreamerReader (
    this=0x7ff578a5c800, __in_chrg=<optimized out>)
    at /mnt/ssd1/karl/moz/dev/dom/media/gstreamer/GStreamerReader.cpp:139
#29 0x00007ff6176d30ff in mozilla::MediaDecoderReader::Release (
    this=0x7ff578a5c800) at ../../dist/include/MediaDecoderReader.h:61
#30 0x00007ff6176f95e1 in nsRefPtr<mozilla::MediaDecoderReader>::assign_assuming_AddRef (this=0x7ff578cf2de8, aNewPtr=0x0)
    at ../../dist/include/nsRefPtr.h:47
#31 0x00007ff6176ee9dc in nsRefPtr<mozilla::MediaDecoderReader>::assign_with_AddRef (this=0x7ff578cf2de8, aRawPtr=0x0) at ../../dist/include/nsRefPtr.h:31
#32 0x00007ff6176e27ab in nsRefPtr<mozilla::MediaDecoderReader>::operator= (
    this=0x7ff578cf2de8, aRhs=0x0) at ../../dist/include/nsRefPtr.h:127
#33 0x00007ff6176a1d25 in mozilla::MediaDecoderStateMachine::~MediaDecoderStateMachine (this=0x7ff578cf2c00, __in_chrg=<optimized out>)
    at /mnt/ssd1/karl/moz/dev/dom/media/MediaDecoderStateMachine.cpp:287
#34 0x00007ff6176a1e1a in mozilla::MediaDecoderStateMachine::~MediaDecoderStateMachine (this=0x7ff578cf2c00, __in_chrg=<optimized out>)
    at /mnt/ssd1/karl/moz/dev/dom/media/MediaDecoderStateMachine.cpp:292
#35 0x00007ff61768518b in mozilla::MediaDecoderStateMachine::Release (
    this=0x7ff578cf2c00)
    at /mnt/ssd1/karl/moz/dev/dom/media/MediaDecoderStateMachine.h:127
#36 0x00007ff6176f8013 in nsRefPtr<mozilla::MediaDecoderStateMachine>::assign_assuming_AddRef (this=0x7ff5cedae180, aNewPtr=0x0)
    at ../../dist/include/nsRefPtr.h:47
#37 0x00007ff6176ecd76 in nsRefPtr<mozilla::MediaDecoderStateMachine>::assign_with_AddRef (this=0x7ff5cedae180, aRawPtr=0x0)
    at ../../dist/include/nsRefPtr.h:31
#38 0x00007ff6176e0811 in nsRefPtr<mozilla::MediaDecoderStateMachine>::operator= (this=0x7ff5cedae180, aRhs=0x0) at ../../dist/include/nsRefPtr.h:127
#39 0x00007ff6176d4d0e in mozilla::nsDecoderDisposeEvent::Run (
    this=0x7ff5cedae160)
    at /mnt/ssd1/karl/moz/dev/dom/media/MediaDecoderStateMachine.cpp:2687

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: shutdown GStreamer playbin during Shutdown() r?

instead of during reader destruction.

Closing the guts of the reader during Shutdown() is consistent with what is
done in WebMReader and MP4Reader.

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: clear mDecoder on reader during Shutdown() r?

My main concern here is the dangling pointer.  If we know that the state
machine holds the last reference to the reader, then there is no need to
reference count the reader.  If the reader is reference counted then we should
assume that something else may hold a reference to the reader and so we should
clear dangling pointers.

There's probably more than one place where the decoder pointer could be set to
null, MDReader::BreakCycles() being the other primary candidate.  The choice
of Shutdown() here is consistent with what was done with TrackBuffer's
subreaders.  Subreaders can be Shutdown() while their task queue (the parent
reader's) continues to process tasks, so clearing the decoder on the task
queue (instead of the main thread) provides memory consistency should there be
a need for a subsequent task to null-check the pointer before using it.

I'm not convinced that mShutdown is always true in ~MediaDecoderReader,
particularly with some of the set-up failure paths in
webaudio/MediaBufferDecoder.cpp.  But if mShutdown is true, then mDecoder
should be null.  I guess we could still ensure that Shutdown() is called when
reader set-up fails and permit such a call on a non-decode thread when there
is no task queue, but I'm treating that as an existing issue and leaving as is
for now.

Comment 7

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4059b7fd39d4
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cd2323821d8

Comment 8

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef73d0015dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b86c91f7b96

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ef73d0015dc
https://hg.mozilla.org/mozilla-central/rev/9ef73d0015dc

Comment 10

[Karl Tomlinson (:karlt)]

This "fixed" a potential security issue in the last iteration of the fix for
bug 1069602.  However, the security issue is now replaced with a null dereference, tracked in bug 1137076.

https://bugzilla.mozilla.org/attachment.cgi?oldid=8497922&action=interdiff&newid=8498694&headers=1

When ProcessCachedData() runs, there is no lock held.  After the IsShutdown()
test succeeds, mIsShutdown may be set to true on the main thread before the
bare pointer mDecoder is dereferenced.  If mIsShutdown is true, the object
associated with mDecoder may have been deleted.  At least the pointer is now
null when dereferenced.