Bug 1075980 - Be more careful about who holds a reference to the SourceBufferDecoder during initialization. r=karlt
authorMatthew Gregan <kinetik@flim.org>
Thu, 02 Oct 2014 18:04:06 +1300
changeset 232000 d5eb688ed4a1bfb175e02caa2092a46cff9662e3
parent 231999 88258156ffef434a46ecacf2ea09f47f8862e958
child 232001 0de83f6ba398472a92d25124a3d56175377e458c
push id4187
push userbhearsum@mozilla.com
push dateFri, 28 Nov 2014 15:29:12 +0000
treeherdermozilla-beta@f23cc6a30c11 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskarlt
bugs1075980
milestone35.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1075980 - Be more careful about who holds a reference to the SourceBufferDecoder during initialization. r=karlt
content/media/mediasource/TrackBuffer.cpp
content/media/mediasource/TrackBuffer.h
content/media/mediasource/test/test_BufferedSeek.html
content/media/mediasource/test/test_FrameSelection.html
--- a/content/media/mediasource/TrackBuffer.cpp
+++ b/content/media/mediasource/TrackBuffer.cpp
@@ -48,17 +48,17 @@ TrackBuffer::TrackBuffer(MediaSourceDeco
 
 TrackBuffer::~TrackBuffer()
 {
   MOZ_COUNT_DTOR(TrackBuffer);
 }
 
 class ReleaseDecoderTask : public nsRunnable {
 public:
-  explicit ReleaseDecoderTask(nsRefPtr<SourceBufferDecoder> aDecoder)
+  explicit ReleaseDecoderTask(SourceBufferDecoder* aDecoder)
   {
     mDecoders.AppendElement(aDecoder);
   }
 
   explicit ReleaseDecoderTask(nsTArray<nsRefPtr<SourceBufferDecoder>>& aDecoders)
   {
     mDecoders.SwapElements(aDecoders);
   }
@@ -232,50 +232,53 @@ TrackBuffer::NewDecoder()
 
   mLastStartTimestamp = 0;
   mLastEndTimestamp = 0;
 
   return QueueInitializeDecoder(decoder);
 }
 
 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;
   }
 
   if (mi.HasVideo()) {
     MSE_DEBUG("TrackBuffer(%p): Reader %p video resolution=%dx%d",
               this, reader, mi.mVideo.mDisplay.width, mi.mVideo.mDisplay.height);
   }
@@ -309,20 +312,19 @@ TrackBuffer::ValidateTrackFormats(const 
     MSE_DEBUG("TrackBuffer(%p)::ValidateTrackFormats audio format mismatch", this);
     return false;
   }
 
   return true;
 }
 
 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()) {
     mInfo = info;
     mParentDecoder->OnTrackBufferConfigured(this, mInfo);
   }
   if (!ValidateTrackFormats(info)) {
     MSE_DEBUG("TrackBuffer(%p)::RegisterDecoder with mismatched audio/video tracks", this);
@@ -423,17 +425,24 @@ TrackBuffer::Dump(const char* aPath)
     PR_MkDir(buf, 0700);
 
     mDecoders[i]->GetResource()->Dump(buf);
   }
 }
 #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
--- a/content/media/mediasource/TrackBuffer.h
+++ b/content/media/mediasource/TrackBuffer.h
@@ -85,37 +85,37 @@ private:
   // initialized until it is added to mDecoders.
   bool NewDecoder();
 
   // Helper for AppendData, ensures NotifyDataArrived is called whenever
   // data is appended to the current decoder's SourceBufferResource.
   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.
   bool ValidateTrackFormats(const MediaInfo& aInfo);
 
   // Remove aDecoder from mDecoders and dispatch an event to the main thread
   // 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;
 
   // A task queue using the shared media thread pool.  Used exclusively to
   // initialize (i.e. call ReadMetadata on) decoders as they are created via
   // NewDecoder.
   RefPtr<MediaTaskQueue> mTaskQueue;
 
--- a/content/media/mediasource/test/test_BufferedSeek.html
+++ b/content/media/mediasource/test/test_BufferedSeek.html
@@ -33,22 +33,22 @@ runWithMSE(function (ms, v) {
         v.currentTime = target;
       }
     });
 
     var wasSeeking = false;
 
     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();
     });
   });
 });
 
 </script>
 </pre>
 </body>
--- a/content/media/mediasource/test/test_FrameSelection.html
+++ b/content/media/mediasource/test/test_FrameSelection.html
@@ -22,19 +22,19 @@ runWithMSE(function (ms, v) {
     });
 
     var targets = [{ currentTime: 3, videoWidth: 160, videoHeight: 120 },
                    { currentTime: 2, videoWidth: 160, videoHeight: 120 },
                    { currentTime: 0, videoWidth: 320, videoHeight: 240 }];
     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.
         sb.appendBuffer(new Uint8Array(arrayBuffer, 0, 438));
         var first = true;
         sb.addEventListener("updateend", function () {
           if (first) {
             // Append media segment covering range [2, 4].
@@ -45,20 +45,20 @@ runWithMSE(function (ms, v) {
             target = targets.shift();
             v.currentTime = target.currentTime;
           }
         });
       });
     });
 
     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) {
         v.currentTime = target.currentTime;
       } else {
         SimpleTest.finish();
       }
     });