Bug 1047022 - Remove manual addref for Session object. Let MediaRecorder hold reference to Session that makes a cycle reference, break it in DestrotedRunnable(). r=roc
authorBenjamin Chen <bechen@mozilla.com>
Mon, 18 Aug 2014 11:42:49 +0800
changeset 200264 53c7fcb7ead91a2bda0ec1fd26de32c8058e04dd
parent 200263 4db3058a5c9f54b92e4e675478f86f7b1c34c1f8
child 200265 8944f233d14b0863b90ec4e6800a0735bd7cafba
push id47860
push userryanvm@gmail.com
push dateTue, 19 Aug 2014 12:42:37 +0000
treeherdermozilla-inbound@bb91698edd20 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersroc
bugs1047022
milestone34.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 1047022 - Remove manual addref for Session object. Let MediaRecorder hold reference to Session that makes a cycle reference, break it in DestrotedRunnable(). r=roc
content/media/MediaRecorder.cpp
content/media/MediaRecorder.h
--- a/content/media/MediaRecorder.cpp
+++ b/content/media/MediaRecorder.cpp
@@ -65,20 +65,22 @@ NS_IMPL_RELEASE_INHERITED(MediaRecorder,
  * 2) Extract Stage (in Read Thread)
  *    Pull encoded A/V frames from MediaEncoder, dispatch to OnDataAvailable handler.
  *    Unless a client calls Session::Stop, Session object keeps stay in this stage.
  * 3) Destroy Stage (in main thread)
  *    Switch from Extract stage to Destroy stage by calling Session::Stop.
  *    Release session resource and remove associated streams from MSG.
  *
  * Lifetime of MediaRecorder and Session objects.
- * 1) MediaRecorder creates a Session in MediaRecorder::Start function.
- *    And the Session registers itself to ShutdownObserver and also holds a
- *    MediaRecorder. Therefore, the reference dependency in gecko is:
- *    ShutdownObserver -> Session -> MediaRecorder
+ * 1) MediaRecorder creates a Session in MediaRecorder::Start function and holds
+ *    a reference to Session. Then the Session registers itself to
+ *    ShutdownObserver and also holds a reference to MediaRecorder.
+ *    Therefore, the reference dependency in gecko is:
+ *    ShutdownObserver -> Session <-> MediaRecorder, note that there is a cycle
+ *    reference between Session and MediaRecorder.
  * 2) A Session is destroyed in DestroyRunnable after MediaRecorder::Stop being called
  *    _and_ all encoded media data been passed to OnDataAvailable handler.
  * 3) MediaRecorder::Stop is called by user or the document is going to
  *    inactive or invisible.
  */
 class MediaRecorder::Session: public nsIObserver
 {
   NS_DECL_THREADSAFE_ISUPPORTS
@@ -145,46 +147,38 @@ class MediaRecorder::Session: public nsI
     nsString mEventName;
   };
 
   // Record thread task and it run in Media Encoder thread.
   // Fetch encoded Audio/Video data from MediaEncoder.
   class ExtractRunnable : public nsRunnable
   {
   public:
-    ExtractRunnable(already_AddRefed<Session>&& aSession)
+    ExtractRunnable(Session* aSession)
       : mSession(aSession) {}
 
     ~ExtractRunnable()
-    {
-      if (mSession) {
-        NS_WARNING("~ExtractRunnable something wrong the mSession should null");
-        nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
-        NS_WARN_IF_FALSE(mainThread, "Couldn't get the main thread!");
-        NS_ProxyRelease(mainThread, mSession);
-      }
-    }
+    {}
 
     NS_IMETHODIMP Run()
     {
       MOZ_ASSERT(NS_GetCurrentThread() == mSession->mReadThread);
 
       LOG(PR_LOG_DEBUG, ("Session.ExtractRunnable shutdown = %d", mSession->mEncoder->IsShutdown()));
       if (!mSession->mEncoder->IsShutdown()) {
         mSession->Extract(false);
-        nsRefPtr<nsIRunnable> event = new ExtractRunnable(mSession.forget());
+        nsRefPtr<nsIRunnable> event = new ExtractRunnable(mSession);
         if (NS_FAILED(NS_DispatchToCurrentThread(event))) {
           NS_WARNING("Failed to dispatch ExtractRunnable to encoder thread");
         }
       } else {
         // Flush out remaining encoded data.
         mSession->Extract(true);
-        // Destroy this Session object in main thread.
         if (NS_FAILED(NS_DispatchToMainThread(
-                        new DestroyRunnable(mSession.forget())))) {
+                        new DestroyRunnable(mSession)))) {
           MOZ_ASSERT(false, "NS_DispatchToMainThread DestroyRunnable failed");
         }
       }
       return NS_OK;
     }
 
   private:
     nsRefPtr<Session> mSession;
@@ -219,17 +213,17 @@ class MediaRecorder::Session: public nsI
   private:
     nsRefPtr<Session> mSession;
   };
   // Main thread task.
   // To delete RecordingSession object.
   class DestroyRunnable : public nsRunnable
   {
   public:
-    DestroyRunnable(already_AddRefed<Session>&& aSession)
+    DestroyRunnable(Session* aSession)
       : mSession(aSession) {}
 
     NS_IMETHODIMP Run()
     {
       LOG(PR_LOG_DEBUG, ("Session.DestroyRunnable session refcnt = (%d) stopIssued %d s=(%p)",
                          (int)mSession->mRefCnt, mSession->mStopIssued, mSession.get()));
       MOZ_ASSERT(NS_IsMainThread() && mSession.get());
       nsRefPtr<MediaRecorder> recorder = mSession->mRecorder;
@@ -241,29 +235,27 @@ class MediaRecorder::Session: public nsI
       // We need to switch MediaRecorder to "Stop" state first to make sure
       // MediaRecorder is not associated with this Session anymore, then, it's
       // safe to delete this Session.
       // Also avoid to run if this session already call stop before
       if (!mSession->mStopIssued) {
         ErrorResult result;
         mSession->mStopIssued = true;
         recorder->Stop(result);
-        if (NS_FAILED(NS_DispatchToMainThread(
-                        new DestroyRunnable(mSession.forget())))) {
+        if (NS_FAILED(NS_DispatchToMainThread(new DestroyRunnable(mSession)))) {
           MOZ_ASSERT(false, "NS_DispatchToMainThread failed");
         }
         return NS_OK;
       }
 
       // Dispatch stop event and clear MIME type.
       mSession->mMimeType = NS_LITERAL_STRING("");
       recorder->SetMimeType(mSession->mMimeType);
       recorder->DispatchSimpleEvent(NS_LITERAL_STRING("stop"));
-      recorder->RemoveSession(mSession);
-      mSession->mRecorder = nullptr;
+      mSession->BreakCycle();
       return NS_OK;
     }
 
   private:
     // Call mSession::Release automatically while DestroyRunnable be destroy.
     nsRefPtr<Session> mSession;
   };
 
@@ -449,37 +441,34 @@ private:
         return;
       }
     }
 
     // In case source media stream does not notify track end, recieve
     // shutdown notification and stop Read Thread.
     nsContentUtils::RegisterShutdownObserver(this);
 
-    already_AddRefed<Session> session(this); // addref in MediaRecorder::Start
-    nsRefPtr<nsIRunnable> event = new ExtractRunnable(Move(session));
+    nsRefPtr<nsIRunnable> event = new ExtractRunnable(this);
     if (NS_FAILED(mReadThread->Dispatch(event, NS_DISPATCH_NORMAL))) {
       NS_WARNING("Failed to dispatch ExtractRunnable at beginning");
     }
   }
   // application should get blob and onstop event
   void DoSessionEndTask(nsresult rv)
   {
     MOZ_ASSERT(NS_IsMainThread());
     if (NS_FAILED(rv)) {
       mRecorder->NotifyError(rv);
     }
 
     CleanupStreams();
     if (NS_FAILED(NS_DispatchToMainThread(new PushBlobRunnable(this)))) {
       MOZ_ASSERT(false, "NS_DispatchToMainThread PushBlobRunnable failed");
     }
-    // Destroy this session object in main thread.
-    already_AddRefed<Session> session(this); // addref in MediaRecorder::Start
-    if (NS_FAILED(NS_DispatchToMainThread(new DestroyRunnable(Move(session))))) {
+    if (NS_FAILED(NS_DispatchToMainThread(new DestroyRunnable(this)))) {
       MOZ_ASSERT(false, "NS_DispatchToMainThread DestroyRunnable failed");
     }
   }
   void CleanupStreams()
   {
     if (mInputPort.get()) {
       mInputPort->Destroy();
       mInputPort = nullptr;
@@ -497,26 +486,33 @@ private:
     LOG(PR_LOG_DEBUG, ("Session.Observe XPCOM_SHUTDOWN %p", this));
     if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {
       // Force stop Session to terminate Read Thread.
       mEncoder->Cancel();
       if (mReadThread) {
         mReadThread->Shutdown();
         mReadThread = nullptr;
       }
-      if (mRecorder) {
-        mRecorder->RemoveSession(this);
-        mRecorder = nullptr;
-      }
+      BreakCycle();
       Stop();
     }
 
     return NS_OK;
   }
 
+  // Break the cycle reference between Session and MediaRecorder.
+  void BreakCycle()
+  {
+    MOZ_ASSERT(NS_IsMainThread());
+    if (mRecorder) {
+      mRecorder->RemoveSession(this);
+      mRecorder = nullptr;
+    }
+  }
+
 private:
   // Hold reference to MediaRecoder that ensure MediaRecorder is alive
   // if there is an active session. Access ONLY on main thread.
   nsRefPtr<MediaRecorder> mRecorder;
 
   // Receive track data from source and dispatch to Encoder.
   // Pause/ Resume controller.
   nsRefPtr<ProcessedMediaStream> mTrackUnionStream;
@@ -635,26 +631,18 @@ MediaRecorder::Start(const Optional<int3
       return;
     }
 
     timeSlice = aTimeSlice.Value();
   }
 
   mState = RecordingState::Recording;
   // Start a session.
-  // Add session's reference here and pass to ExtractRunnable since the
-  // MediaRecorder doesn't hold any reference to Session. Also
-  // the DoSessionEndTask need this reference for DestroyRunnable.
-  // Note that the reference count is not balance here due to the
-  // DestroyRunnable will destroyed the last reference.
-  nsRefPtr<Session> session = new Session(this, timeSlice);
-  Session* rawPtr;
-  session.forget(&rawPtr);
   mSessions.AppendElement();
-  mSessions.LastElement() = rawPtr;
+  mSessions.LastElement() = new Session(this, timeSlice);
   mSessions.LastElement()->Start();
 }
 
 void
 MediaRecorder::Stop(ErrorResult& aResult)
 {
   LOG(PR_LOG_DEBUG, ("MediaRecorder.Stop %p", this));
   if (mState == RecordingState::Inactive) {
--- a/content/media/MediaRecorder.h
+++ b/content/media/MediaRecorder.h
@@ -106,19 +106,19 @@ protected:
 
   MediaRecorder(const MediaRecorder& x) MOZ_DELETE; // prevent bad usage
   // Remove session pointer.
   void RemoveSession(Session* aSession);
   // MediaStream passed from js context
   nsRefPtr<DOMMediaStream> mStream;
   // The current state of the MediaRecorder object.
   RecordingState mState;
-  // Hold the sessions pointer and clean it when the DestroyRunnable for a
+  // Hold the sessions reference and clean it when the DestroyRunnable for a
   // session is running.
-  nsTArray<Session*> mSessions;
+  nsTArray<nsRefPtr<Session> > mSessions;
   // It specifies the container format as well as the audio and video capture formats.
   nsString mMimeType;
 
 private:
   // Register MediaRecorder into Document to listen the activity changes.
   void RegisterActivityObserver();
   void UnRegisterActivityObserver();
 };