Stop synchronously dispatching MediaDecoder::DecodeError from MDSM::DecodeError

MediaDecoder::DecodeError invokes MediaDecoder::Shutdown, which shuts down the state machine. Given the sync dispatch, this means that this is basically already what's happening.

dom/media/MediaDecoderStateMachine.cpp

@@ -1113,7 +1113,7 @@ void
 MediaDecoderStateMachine::CheckIfDecodeComplete()
 {
   AssertCurrentThreadInMonitor();
-  if (mState == DECODER_STATE_SHUTDOWN ||
+  if (IsShutdown() ||
       mState == DECODER_STATE_SEEKING ||
       mState == DECODER_STATE_COMPLETED) {
     // Don't change our state if we've already been shutdown, or we're seeking,
@@ -1506,7 +1506,7 @@ void MediaDecoderStateMachine::SetDormant(bool aDormant)
   MOZ_ASSERT(OnStateMachineThread(), "Should be on state machine thread.");
   ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
 
-  if (mState == DECODER_STATE_SHUTDOWN) {
+  if (IsShutdown()) {
     return;
   }
 
@@ -1749,7 +1749,7 @@ MediaDecoderStateMachine::Seek(SeekTarget aTarget)
 
   mDecodingFrozenAtStateDecoding = false;
 
-  if (mState == DECODER_STATE_SHUTDOWN) {
+  if (IsShutdown()) {
     return MediaDecoder::SeekPromise::CreateAndReject(/* aIgnored = */ true, __func__);
   }
 
@@ -1905,7 +1905,7 @@ MediaDecoderStateMachine::DispatchDecodeTasksIfNeeded()
     RefPtr<nsIRunnable> event = NS_NewRunnableMethod(
         this, &MediaDecoderStateMachine::SetReaderIdle);
     nsresult rv = DecodeTaskQueue()->Dispatch(event.forget());
-    if (NS_FAILED(rv) && mState != DECODER_STATE_SHUTDOWN) {
+    if (NS_FAILED(rv) && !IsShutdown()) {
       DECODER_WARN("Failed to dispatch event to set decoder idle state");
     }
   }
@@ -2203,31 +2203,26 @@ MediaDecoderStateMachine::DecodeError()
 {
   MOZ_ASSERT(OnStateMachineThread());
   ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
-  if (mState == DECODER_STATE_SHUTDOWN) {
+  if (IsShutdown()) {
     // Already shutdown.
     return;
   }
 
-  // Change state to shutdown before sending error report to MediaDecoder
-  // and the HTMLMediaElement, so that our pipeline can start exiting
-  // cleanly during the sync dispatch below.
+  // Change state to error, which will cause the state machine to wait until
+  // the MediaDecoder shuts it down.
+  SetState(DECODER_STATE_ERROR);
   ScheduleStateMachine();
-  SetState(DECODER_STATE_SHUTDOWN);
-  DECODER_WARN("Decode error, changed state to SHUTDOWN due to error");
+  DECODER_WARN("Decode error, changed state to ERROR");
+
+  // Is anybody actually waiting on this monitor, or is it just
+  // a leftover from when we used to do sync dispatch for the below?
   mDecoder->GetReentrantMonitor().NotifyAll();
 
-  // Dispatch the event to call DecodeError synchronously. This ensures
-  // we're in shutdown state by the time we exit the decode thread.
-  // If we just moved to shutdown state here on the decode thread, we may
-  // cause the state machine to shutdown/free memory without closing its
-  // media stream properly, and we'll get callbacks from the media stream
-  // causing a crash.
- {
-    nsCOMPtr<nsIRunnable> event =
-      NS_NewRunnableMethod(mDecoder, &MediaDecoder::DecodeError);
-    ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
-    NS_DispatchToMainThread(event, NS_DISPATCH_SYNC);
-  }
+  // MediaDecoder::DecodeError notifies the owner, and then shuts down the state
+  // machine.
+  nsCOMPtr<nsIRunnable> event =
+    NS_NewRunnableMethod(mDecoder, &MediaDecoder::DecodeError);
+  NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
 }
 
 void
@@ -2416,7 +2411,7 @@ MediaDecoderStateMachine::FinishDecodeFirstFrame()
   MOZ_ASSERT(OnStateMachineThread());
   DECODER_LOG("FinishDecodeFirstFrame");
 
-  if (mState == DECODER_STATE_SHUTDOWN) {
+  if (IsShutdown()) {
     return NS_ERROR_FAILURE;
   }
 
@@ -2517,7 +2512,7 @@ MediaDecoderStateMachine::SeekCompleted()
 
   if (mState != DECODER_STATE_SEEKING) {
     MOZ_DIAGNOSTIC_ASSERT(mState == DECODER_STATE_DORMANT ||
-                          mState == DECODER_STATE_SHUTDOWN);
+                          IsShutdown());
     // It would be nice to assert mCurrent.Exists() here, but it's possible that
     // we've transitioned to DECODER_STATE_SHUTDOWN but not yet gone through
     // RunStateMachine in that state, which is where this promise gets rejected.
@@ -2690,6 +2685,11 @@ nsresult MediaDecoderStateMachine::RunStateMachine()
   NS_ENSURE_TRUE(resource, NS_ERROR_NULL_POINTER);
 
   switch (mState) {
+    case DECODER_STATE_ERROR: {
+      // Just wait for MediaDecoder::DecodeError to shut us down.
+      return NS_OK;
+    }
+
     case DECODER_STATE_SHUTDOWN: {
       mQueuedSeek.RejectIfExists(__func__);
       mPendingSeek.RejectIfExists(__func__);
@@ -3524,7 +3524,8 @@ MediaDecoderStateMachine::SetMinimizePrerollUntilPlaybackStarts()
 bool MediaDecoderStateMachine::IsShutdown()
 {
   AssertCurrentThreadInMonitor();
-  return GetState() == DECODER_STATE_SHUTDOWN;
+  return mState == DECODER_STATE_ERROR ||
+         mState == DECODER_STATE_SHUTDOWN;
 }
 
 void MediaDecoderStateMachine::QueueMetadata(int64_t aPublishTime,