Bug 1075980 - Be more careful about who holds a reference to the SourceBufferDecoder during initialization. r=karlt

content/media/mediasource/TrackBuffer.cpp

@@ -53,7 +53,7 @@ TrackBuffer::~TrackBuffer()
 
 class ReleaseDecoderTask : public nsRunnable {
 public:
-  explicit ReleaseDecoderTask(nsRefPtr<SourceBufferDecoder> aDecoder)
+  explicit ReleaseDecoderTask(SourceBufferDecoder* aDecoder)
   {
     mDecoders.AppendElement(aDecoder);
   }
@@ -237,40 +237,43 @@ TrackBuffer::NewDecoder()
 }
 
 bool
-TrackBuffer::QueueInitializeDecoder(nsRefPtr<SourceBufferDecoder> aDecoder)
+TrackBuffer::QueueInitializeDecoder(SourceBufferDecoder* aDecoder)
 {
   RefPtr<nsIRunnable> task =
-    NS_NewRunnableMethodWithArg<nsRefPtr<SourceBufferDecoder>>(this,
-                                                               &TrackBuffer::InitializeDecoder,
-                                                               aDecoder);
+    NS_NewRunnableMethodWithArg<SourceBufferDecoder*>(this,
+                                                      &TrackBuffer::InitializeDecoder,
+                                                      aDecoder);
   aDecoder->SetTaskQueue(mTaskQueue);
   if (NS_FAILED(mTaskQueue->Dispatch(task))) {
     MSE_DEBUG("MediaSourceReader(%p): Failed to enqueue decoder initialization task", this);
+    RemoveDecoder(aDecoder);
     return false;
   }
   return true;
 }
 
 void
-TrackBuffer::InitializeDecoder(nsRefPtr<SourceBufferDecoder> aDecoder)
+TrackBuffer::InitializeDecoder(SourceBufferDecoder* aDecoder)
 {
+  MOZ_ASSERT(mTaskQueue->IsCurrentThreadIn());
+
   // ReadMetadata may block the thread waiting on data, so it must not be
   // called with the monitor held.
   mParentDecoder->GetReentrantMonitor().AssertNotCurrentThreadIn();
 
   MediaDecoderReader* reader = aDecoder->GetReader();
   MSE_DEBUG("TrackBuffer(%p): Initializing subdecoder %p reader %p",
-            this, aDecoder.get(), reader);
+            this, aDecoder, reader);
 
   MediaInfo mi;
   nsAutoPtr<MetadataTags> tags; // TODO: Handle metadata.
   nsresult rv = reader->ReadMetadata(&mi, getter_Transfers(tags));
+  aDecoder->SetTaskQueue(nullptr);
   reader->SetIdle();
   if (NS_FAILED(rv) || (!mi.HasVideo() && !mi.HasAudio())) {
     // XXX: Need to signal error back to owning SourceBuffer.
     MSE_DEBUG("TrackBuffer(%p): Reader %p failed to initialize rv=%x audio=%d video=%d",
               this, reader, rv, mi.HasAudio(), mi.HasVideo());
-    aDecoder->SetTaskQueue(nullptr);
     RemoveDecoder(aDecoder);
     return;
   }
@@ -314,10 +317,9 @@ TrackBuffer::ValidateTrackFormats(const MediaInfo& aInfo)
 }
 
 bool
-TrackBuffer::RegisterDecoder(nsRefPtr<SourceBufferDecoder> aDecoder)
+TrackBuffer::RegisterDecoder(SourceBufferDecoder* aDecoder)
 {
   ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());
-  aDecoder->SetTaskQueue(nullptr);
   const MediaInfo& info = aDecoder->GetReader()->GetMediaInfo();
   // Initialize the track info since this is the first decoder.
   if (mInitializedDecoders.IsEmpty()) {
@@ -428,12 +430,19 @@ TrackBuffer::Dump(const char* aPath)
 #endif
 
 void
-TrackBuffer::RemoveDecoder(nsRefPtr<SourceBufferDecoder> aDecoder)
+TrackBuffer::RemoveDecoder(SourceBufferDecoder* aDecoder)
 {
-  ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());
-  MOZ_ASSERT(!mInitializedDecoders.Contains(aDecoder));
-  mDecoders.RemoveElement(aDecoder);
-  NS_DispatchToMainThread(new ReleaseDecoderTask(aDecoder));
+  RefPtr<nsIRunnable> task = new ReleaseDecoderTask(aDecoder);
+  {
+    ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());
+    MOZ_ASSERT(!mInitializedDecoders.Contains(aDecoder));
+    mDecoders.RemoveElement(aDecoder);
+    if (mCurrentDecoder == aDecoder) {
+      DiscardDecoder();
+    }
+  }
+  // At this point, task should be holding the only reference to aDecoder.
+  NS_DispatchToMainThread(task);
 }
 
 } // namespace mozilla

content/media/mediasource/TrackBuffer.h

@@ -90,17 +90,17 @@ private:
   bool AppendDataToCurrentResource(const uint8_t* aData, uint32_t aLength);
 
   // Queue execution of InitializeDecoder on mTaskQueue.
-  bool QueueInitializeDecoder(nsRefPtr<SourceBufferDecoder> aDecoder);
+  bool QueueInitializeDecoder(SourceBufferDecoder* aDecoder);
 
   // Runs decoder initialization including calling ReadMetadata.  Runs as an
   // event on the decode thread pool.
-  void InitializeDecoder(nsRefPtr<SourceBufferDecoder> aDecoder);
+  void InitializeDecoder(SourceBufferDecoder* aDecoder);
 
   // Adds a successfully initialized decoder to mDecoders and (if it's the
   // first decoder initialized), initializes mHasAudio/mHasVideo.  Called
   // from the decode thread pool.  Return true if the decoder was
   // successfully registered.
-  bool RegisterDecoder(nsRefPtr<SourceBufferDecoder> aDecoder);
+  bool RegisterDecoder(SourceBufferDecoder* aDecoder);
 
   // Returns true if aInfo is considered a supported or the same format as
   // the TrackBuffer was initialized as.
@@ -110,7 +110,7 @@ private:
   // to clean up the decoder.  If aDecoder was added to
   // mInitializedDecoders, it must have been removed before calling this
   // function.
-  void RemoveDecoder(nsRefPtr<SourceBufferDecoder> aDecoder);
+  void RemoveDecoder(SourceBufferDecoder* aDecoder);
 
   nsAutoPtr<ContainerParser> mParser;
 

content/media/mediasource/test/test_BufferedSeek.html

@@ -38,12 +38,12 @@ runWithMSE(function (ms, v) {
 
     v.addEventListener("seeking", function () {
       wasSeeking = true;
-      is(v.currentTime, target, "Video currentTime not at target");
+      is(v.currentTime, target, "Video currentTime at target");
     });
 
     v.addEventListener("seeked", function () {
       ok(wasSeeking, "Received expected seeking and seeked events");
-      is(v.currentTime, target, "Video currentTime not at target");
+      is(v.currentTime, target, "Video currentTime at target");
       SimpleTest.finish();
     });
   });

content/media/mediasource/test/test_FrameSelection.html

@@ -27,9 +27,9 @@ runWithMSE(function (ms, v) {
     var target;
 
     v.addEventListener("loadedmetadata", function () {
-      is(v.currentTime, 0, "currentTime has incorrect initial value");
-      is(v.videoWidth, 320, "videoWidth has incorrect initial value");
-      is(v.videoHeight, 240, "videoHeight has incorrect initial value");
+      is(v.currentTime, 0, "currentTime has correct initial value");
+      is(v.videoWidth, 320, "videoWidth has correct initial value");
+      is(v.videoHeight, 240, "videoHeight has correct initial value");
 
       fetchWithXHR("seek_lowres.webm", function (arrayBuffer) {
         // Append initialization segment.
@@ -50,10 +50,10 @@ runWithMSE(function (ms, v) {
     });
 
     v.addEventListener("seeked", function () {
-      is(v.currentTime, target.currentTime, "Video currentTime not at target");
+      is(v.currentTime, target.currentTime, "Video currentTime at target");
 
-      is(v.videoWidth, target.videoWidth, "videoWidth has incorrect final value");
-      is(v.videoHeight, target.videoHeight, "videoHeight has incorrect final value");
+      is(v.videoWidth, target.videoWidth, "videoWidth has correct final value");
+      is(v.videoHeight, target.videoHeight, "videoHeight has correct final value");
 
       target = targets.shift();
       if (target) {