Closed Bug 483324 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: cpearce)

References

Details

(Keywords: assertion, fixed1.9.1, regression)

Attachments

(1 file, 1 obsolete file)

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?
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
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.
Duplicate of this bug: 483563
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.
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.
Depends on: 475441
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → chris
Attachment #367658 - Flags: superreview?(roc)
Attachment #367658 - Flags: review?(roc)
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"?
Attachment #367658 - Flags: superreview?(roc)
Attachment #367658 - Flags: superreview+
Attachment #367658 - Flags: review?(roc)
Attachment #367658 - Flags: review+
As v1 with typo fixed. r+/sr+ roc.
Attachment #367658 - Attachment is obsolete: true
Attachment #367659 - Flags: superreview+
Attachment #367659 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
No longer depends on: 475441
Depends on: 475441
http://hg.mozilla.org/mozilla-central/rev/bc40381836f6
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
You need to log in before you can comment on or make changes to this bug.