483324 - "ASSERTION: nsMediaStream::Close called on non-main thread" with LoadOggHeaders on the stack

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jesse Ruderman]

I'm hitting this on mozilla-central a lot but haven't been able to make a testcase.

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

nsMediaStream::Close()
nsChannelReader::destroy()
oggplay_channel_reader_destroy(_OggPlayReader*)
oggplay_close
oggplay_open_with_reader
nsOggDecodeStateMachine::LoadOggHeaders(nsChannelReader*)
nsOggDecodeStateMachine::Run()
nsThread::ProcessNextEvent(int, int*)
NS_ProcessNextEvent_P(nsIThread*, int)
nsThread::ThreadFunc(void*)
_pt_root
_pthread_start
thread_start

Similar to the fixed bug 462878?

Comment 1

[Jesse Ruderman]

Often followed by additional thread-safety assertions, such as

###!!! ASSERTION: Only call on main thread: 'NS_IsMainThread()', file /Users/jruderman/central/content/media/video/src/nsMediaStream.cpp, line 564

###!!! ASSERTION: nsStandardURL not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/jruderman/central/netwerk/base/src/nsStandardURL.cpp, line 872

###!!! ASSERTION: nsVariant not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/jruderman/central/xpcom/ds/nsVariant.cpp, line 1694

Comment 2

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

These assertions are firing thanks to the new version of liboggplay which we updated last week or so. oggplay_open_with_reader() used to call free() on failure, now it's calling oggplay_close() on failure, which is calling into our code and so the assertions are being triggered.

Comment 4

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

In nsHTMLMediaElement::MediaLoadListener::OnStartRequest(), we always try to
initialized a channel. However when an access controls check has failed,  nsCrossSiteListenerProxy will
Cancel() the channel, but we still load a nsMediaDecoder, and create a decode state machine/thread. Then the channel closes, but we try to read it in LoadOggHeaders(), and that fails, which now calls oggplay_close(), hitting the assertion.

MediaLoadListener::OnStartRequest()
should not try to load the channel if it's already been canceled. Eventually
the call path in MediaLoadListener::OnStartRequest() calls down to
nsChannelToPipeListener::OnStartRequest() which checks the request status and
aborts the load (again). We should just bail out in 
MediaLoadListener::OnStartRequest() if the channel is canceled.

Comment 5

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

Ah, I think I discovered and fixed this in my patch in bug 475441 by allowing nsMediaStream::Close to happen on non-main threads. But we should do what you suggest as well.

Comment 6

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

Attachment: Patch v1

As per comment 4, don't try to load in MediaLoadListener::OnStartRequest() if the request is already canceled. The nsMediaStream::Close() assertion failures go away, and all video Mochitest still pass on Windows. I'm updating my Linux build now, and will report back if any of its Mochitests fail.

Comment 7

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

Comment on attachment 367658 [details] [diff] [review]
Patch v1

+  // Don't continue to load load if the request failed or has been canceled.

"load load"?

Comment 8

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

Attachment: Patch v2 - typo fixed.

As v1 with typo fixed. r+/sr+ roc.

Comment 9

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

http://hg.mozilla.org/mozilla-central/rev/bc40381836f6

Comment 10

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/73a856d68069