Bug 1577537 - Catch AppendMessage calls that could be too late. r=karlt a=lizzard
authorAndreas Pehrson <apehrson@mozilla.com>
Mon, 02 Sep 2019 14:57:47 +0000
changeset 554859 8773fdb19225d595eb2f27ced37d50777a743a48
parent 554858 afa5bb926fd849b7a9a5546f8678c32bc89d2aa8
child 554860 f7b7283f9bee41c02178abd2311bd7eb60b5e778
push id2165
push userffxbld-merge
push dateMon, 14 Oct 2019 16:30:58 +0000
treeherdermozilla-release@0eae18af659f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskarlt, lizzard
bugs1577537
milestone70.0
Bug 1577537 - Catch AppendMessage calls that could be too late. r=karlt a=lizzard If all streams and all ports have been destroyed, there's no guarantee that the graph is still alive. By forbidding AppendMessage calls after this point, we can catch bugs with the offending callsite still being in the stack. Differential Revision: https://phabricator.services.mozilla.com/D44223
dom/media/MediaStreamGraph.cpp
dom/media/MediaStreamGraphImpl.h
--- a/dom/media/MediaStreamGraph.cpp
+++ b/dom/media/MediaStreamGraph.cpp
@@ -1788,18 +1788,18 @@ void MediaStreamGraphImpl::SignalMainThr
       ("%p: MediaStreamGraph waiting for main thread cleanup", this));
   LifecycleStateRef() =
       MediaStreamGraphImpl::LIFECYCLE_WAITING_FOR_MAIN_THREAD_CLEANUP;
   EnsureStableStateEventPosted();
 }
 
 void MediaStreamGraphImpl::AppendMessage(UniquePtr<ControlMessage> aMessage) {
   MOZ_ASSERT(NS_IsMainThread(), "main thread only");
-  MOZ_ASSERT(!aMessage->GetStream() || !aMessage->GetStream()->IsDestroyed(),
-             "Stream already destroyed");
+  MOZ_ASSERT_IF(aMessage->GetStream(), !aMessage->GetStream()->IsDestroyed());
+  MOZ_DIAGNOSTIC_ASSERT(mMainThreadStreamCount > 0 || mMainThreadPortCount > 0);
 
   if (mDetectedNotRunning &&
       LifecycleStateRef() > LIFECYCLE_WAITING_FOR_MAIN_THREAD_CLEANUP) {
     // The graph control loop is not running and main thread cleanup has
     // happened. From now on we can't append messages to
     // mCurrentTaskMessageQueue, because that will never be processed again, so
     // just RunDuringShutdown this message. This should only happen during
     // forced shutdown, or after a non-realtime graph has finished processing.
@@ -1996,18 +1996,21 @@ void MediaStream::Destroy() {
     void Run() override {
       mStream->RemoveAllListenersImpl();
       auto graph = mStream->GraphImpl();
       mStream->DestroyImpl();
       graph->RemoveStreamGraphThread(mStream);
     }
     void RunDuringShutdown() override { Run(); }
   };
-  GraphImpl()->RemoveStream(this);
-  GraphImpl()->AppendMessage(MakeUnique<Message>(this));
+  // Keep a reference to the graph, since Message might RunDuringShutdown()
+  // synchronously and make GraphImpl() invalid.
+  RefPtr<MediaStreamGraphImpl> graph = GraphImpl();
+  graph->AppendMessage(MakeUnique<Message>(this));
+  graph->RemoveStream(this);
   // Message::RunDuringShutdown may have removed this stream from the graph,
   // but our kungFuDeathGrip above will have kept this stream alive if
   // necessary.
   mMainThreadDestroyed = true;
 }
 
 void MediaStream::AddAudioOutput(void* aKey) {
   class Message : public ControlMessage {
@@ -2979,17 +2982,21 @@ void MediaInputPort::Destroy() {
       mPort->Disconnect();
       --mPort->GraphImpl()->mPortCount;
       mPort->SetGraphImpl(nullptr);
       NS_RELEASE(mPort);
     }
     void RunDuringShutdown() override { Run(); }
     MediaInputPort* mPort;
   };
-  GraphImpl()->AppendMessage(MakeUnique<Message>(this));
+  // Keep a reference to the graph, since Message might RunDuringShutdown()
+  // synchronously and make GraphImpl() invalid.
+  RefPtr<MediaStreamGraphImpl> graph = GraphImpl();
+  graph->AppendMessage(MakeUnique<Message>(this));
+  --graph->mMainThreadPortCount;
 }
 
 MediaStreamGraphImpl* MediaInputPort::GraphImpl() { return mGraph; }
 
 MediaStreamGraph* MediaInputPort::Graph() { return mGraph; }
 
 void MediaInputPort::SetGraphImpl(MediaStreamGraphImpl* aGraph) {
   MOZ_ASSERT(!mGraph || !aGraph, "Should only be set once");
@@ -3097,16 +3104,17 @@ already_AddRefed<MediaInputPort> Process
                               aInputNumber, aOutputNumber);
   }
   if (aBlockedTracks) {
     for (TrackID trackID : *aBlockedTracks) {
       port->BlockSourceTrackIdImpl(trackID, BlockingMode::CREATION);
     }
   }
   port->SetGraphImpl(GraphImpl());
+  ++GraphImpl()->mMainThreadPortCount;
   GraphImpl()->AppendMessage(MakeUnique<Message>(port));
   return port.forget();
 }
 
 void ProcessedMediaStream::QueueSetAutofinish(bool aAutofinish) {
   class Message : public ControlMessage {
    public:
     Message(ProcessedMediaStream* aStream, bool aAutofinish)
--- a/dom/media/MediaStreamGraphImpl.h
+++ b/dom/media/MediaStreamGraphImpl.h
@@ -658,16 +658,25 @@ class MediaStreamGraphImpl : public Medi
    * management.
    *
    * When this becomes zero, the graph is marked as forbidden to add more
    * streams to. It will be shut down shortly after.
    */
   size_t mMainThreadStreamCount = 0;
 
   /**
+   * Main-thread view of the number of ports in this graph, to catch bugs.
+   *
+   * When this becomes zero, and mMainThreadStreamCount is 0, the graph is
+   * marked as forbidden to add more ControlMessages to. It will be shut down
+   * shortly after.
+   */
+  size_t mMainThreadPortCount = 0;
+
+  /**
    * Graphs own owning references to their driver, until shutdown. When a driver
    * switch occur, previous driver is either deleted, or it's ownership is
    * passed to a event that will take care of the asynchronous cleanup, as
    * audio stream can take some time to shut down.
    * Accessed on both the main thread and the graph thread; both read and write.
    * Must hold monitor to access it.
    */
   RefPtr<GraphDriver> mDriver;