462878 - "ASSERTION: nsMediaStream::Close called on non-main thread"

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jesse Ruderman]

###!!! ASSERTION: nsMediaStream::Close called on non-main thread: 'NS_IsMainThread()', file /Users/jruderman/central/content/media/video/src/nsMediaStream.cpp, line 757

nsMediaStream::Close() (nsUnicharUtils.cpp:)
nsChannelReader::destroy() (nsUnicharUtils.cpp:)
oggplay_channel_reader_destroy(_OggPlayReader*) (nsUnicharUtils.cpp:)
oggplay_close (nsUnicharUtils.cpp:)
nsOggDecodeStateMachine::~nsOggDecodeStateMachine() (nsUnicharUtils.cpp:)
nsRunnable::Release() (pldhash.c:)
nsCOMPtr<nsIRunnable>::~nsCOMPtr() (pldhash.c:)
nsCOMPtr<nsIRunnable>::~nsCOMPtr() (pldhash.c:)
nsThread::ProcessNextEvent(int, int*) (pldhash.c:)
NS_ProcessNextEvent_P(nsIThread*, int) (pldhash.c:)
nsThread::ThreadFunc(void*) (pldhash.c:)
_pt_root (/Users/jruderman/central/nsprpub/pr/src/md/unix/os_Darwin.s:47)
_pthread_start (/usr/lib/libSystem.B.dylib)
thread_start (/usr/lib/libSystem.B.dylib)

I only hit this once and I wasn't able to reproduce.  fix-macosx-stack.pl is apparently being less helpful than usual.

Comment 1

[Robert Sayre]

Attachment: video asserts during mochitest seek tests

These files are giving us a lot of trouble on the tinderbox--these asserts are one likely cause. Found these on Windows.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Caused by not destroying the nsMediaStream on the same thread as it's created on.

This can also cause leaks when a media element is destroyed before its had a chance to load its meta data, as the nsOggStateMachine assumes that the nsChannelReader is owned by its mPlayer field, but mPlayer is only created when the meta data is loaded. If the ogg decoder is destroyed before its loaded the meta data, the nsChannelReader will leak.

See also bug 463999.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

Do release of nsOggDecodeStateMachine's stuff on main thread, after nsOggDecodeStateMachine's thread has finished.

Comment 4

[cajbir (:cajbir)]

+    mReader = 0;

mReader = nsnull;

+    case DECODER_STATE_SHUTDOWN: {

Please put { on following line as per the other case statements in that switch.

+      nsCOMPtr<nsIRunnable> event =
+        NS_NEW_RUNNABLE_METHOD(nsOggDecodeStateMachine, this, CleanUp);
+      NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
       return NS_OK;

Is it possible for nsOggDecodeStateMachine to be deleted before this CleanUp is run? nsOggDecoder::Stop results in Shutdown on the nsOggDecodeStateMachine, which posts the event, but later in Stop it clears the reference to nsOggDecodeStateMachine which results in it being deleted, before that cleanup event is run.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks for your comments.

(In reply to comment #4)
> Is it possible for nsOggDecodeStateMachine to be deleted before this CleanUp is
> run?

No, the event holds a reference to the state machine, and the main thread's event queue holds a reference to the event.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsOggDecodeStateMachine says:
  // The decoder object that created this state machine. The decoder
  // always outlives us since it controls our lifetime.
  nsOggDecoder* mDecoder;

So I think the Cleanup method should live in nsOggDecoder, and the event should be fired at nsOggDecoder. Also we should get rid of mReader in nsOggDecodeStateMachine, just get it from the nsOggDecoder. Then when we null it out, it's really gone.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

Moves responsibility of nsChannelReader up into nsOggDecoder, freeing it in nsOggDecoder::Stop() along with everything else, on the main thread.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 347464 [details] [diff] [review]
Patch v2

       return NS_OK;
-
+  

Don't add trailing whitespace.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3

As v2 with review comment addressed. r+ doublec, sr+ roc

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 347464 [details] [diff] [review]
Patch v2

May this patch have approval please? This patch fixes leaks under certain conditions and blocks bug 451958.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 347637 [details] [diff] [review]
Patch v3

a=beltzner for beta

Comment 12

[[e-mail]]

Pushed changeset 72088d2498c3 to mozilla-central.

Comment 13

[Dave Townsend [:mossop]]

I've backed this out as it was causing crashes during the mochitest runs on OSX and Linux:

*** 27603 INFO Running /tests/content/media/video/test/test_seek8.html...
pure virtual method called
terminate called without an active exception
ERROR FAIL Exited with code 6 during test run

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v4 - v3 with re-entrant nsOggDecoder::Stop() prevented

Same as v3 but it includes the mIsStopping check in nsOggDecoder::Stop() which patch for bug 451958 was going to add. This is needed here to prevent a crash in test_seek8.html so pulling it down into this patch.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v5

Similar to v4, but checks on nsOggDecoder::mPlayState. Chris Double pointed out that the previous approach would need mIsStopping to be reset in nsOggDecoder::Load(), and suggested checking mPlayState instead, which doesn't need to be reset.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  {
+    nsAutoMonitor mon(mMonitor);
+    if (mPlayState == PLAY_STATE_ENDED ||
+        mPlayState == PLAY_STATE_SHUTDOWN)
+      return;
+  }
+
   ChangeState(PLAY_STATE_ENDED);

This looks incorrect. It looks like we should put the ChangeState inside the monitor; otherwise, there is a risk of finding mPlayState is not ENDED/SHUTDOWN, but then having it change before we get to change the state ourselves.

Actually I don't think any thread but the main thread can change the state; ChangeState and Stop seem to only be called on the main thread. If that's true, then lets assert that and remove the use of the monitor here, since it's just confusing.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also change the documentation in nsOggDecoder.h.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v6

V5 with review comments addressed.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks good but ChangeState still needs to take the monitor since we need to keep other threads from reading the state while we change the state.

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v7

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: v8

With assertion.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 348501 [details] [diff] [review]
v8

Carrying forward beta2 approval

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed bbbcc5a1eb5e

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bustage fix

Yikes! We're leaking threads with that approach, here's the fix: revert to using a flag.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks for that. Please attach a comment here explaining why the fixup was necessary.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backed out due to test failures.

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

The nsOggDecoderStateMachine was getting stuck in DECODER_STATE_COMPLETED. In the nsOggDecoderStateMachine::Run() case for DECODER_STATE_COMPLETED, there two loops that Wait() while state != DECODER_STATE_SHUTDOWN, and it was getting stuck in there indefinitely. This is because nsOggDecoder::Stop() is bailing out before it could stop the state machine's thread. This was because the guard in Stop() was incorrect.

The problem is that we where bailing out in nsOggDecoder::Stop() if (mPlayState==PLAY_STATE_SHUTDOWN || mPlayState==PLAY_STATE_ENDED), but in nsOggDecoder::Shutdown(), which is one of three entry points for Stop(), we set mPlayState to PLAY_STATE_SHUTDOWN. So we'd bail out by default in this common case, without shutting down the decode thread.

We can't just remove the check to PLAY_STATE_SHUTDOWN, and guard on PLAY_STATE_ENDED only, because although we do call ChangeState(PLAY_STATE_ENDED) in nsOggDecoder::Stop(), that refuses to transition state away from SHUTDOWN to ENDED, and we transition to SHUTDOWN in nsOggDecoder::Shutdown() before calling nsOggDecoder::Stop(). However not all calls to Stop() transition to SHUTDOWN first. So basically mPlayState is not a reliable way to tell if we've run Stop() before, since we do not always change state to SHUTDOWN before entering Stop(), e.g. in nsOggDecoder::NetworkError().

Easiest way to fix is to add the mIsStopping flag, as in the fixup patch. I've tried a few other things, like moving the SHUTDOWN transition to inside Stop(), and replacing Stop() calls with nsOggDecoder::Shutdown() calls, but its still having problems. I suspect that it's problems in this shutdown sequence which is causing intermittent failures, so I'd like to land this ASAP with the mIsStopping flag so that it makes the code freeze tonight, and then work on tidying it up and add assertions and so on, so that mIsStopping is not needed, and to hopefully reduce the chance of intermittent failures.

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v9 - v8 + bustage fix + crash fix

v8 + bustage fix + add null pointer check around mPipeInput->Close() in stream strategies. This would sometimes cause crashes on Mac, in cases where load was interrupted or cancelled, and the channel was valid, but mPipeInput had not been opened due to interruption.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should try relanding this.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(and the access-controls patch)

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Relanded 2d4ddc92e47d

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think I'll actually delay checking in bug 451958 for now. I want to see if this patch can stick by itself.