Jesse Ruderman Reporter
	Description • 16 years ago

Loading the testcase usually triggers one of:

Assertion failure: PR_FALSE == lock->locked, at  nsprpub/pr/src/pthreads/ptsynch.c:190

Assertion failure: 0 == rv, at nsprpub/pr/src/pthreads/ptsynch.c:360

I hit this frequently in my testing.  My opt build crashes less frequently than my debug build asserts, but the crashes looks scary (e.g. some event dispatch code ends up dereferencing the invalid address c0000067).

	cajbir (:cajbir)
	Comment 1 • 16 years ago

This error is a result of a recent bug fix that changed the lifetime of nsChannelReader to be owned by an nsAutoPtr. See bug 462878 comment 7. The lifetime of the reader is controlled by liboggplay. 

liboggplay is calling free on the channel reader when the ogg headers fail to load (due it not being a valid ogg file). We then later call free ourselves due to the autopointer. 

We either need to request liboggplay maintainers to allow us to maintain ownership of the object ourselves, or revert that change from that bug and fix the issue in that bug another way.

	cajbir (:cajbir)
	Comment 2 • 16 years ago

On the other hand, maybe not. liboggplay appears to be doing something else. Discussing with the liboggplay maintaners...

	Jesse Ruderman Reporter
	Comment 3 • 16 years ago

This seems to be resulting in lots of different assertions and crashes, which gets in my way :(

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 16 years ago

Ok, this is fun. Seems like shutting down the previous mDecodeThread may
cause a new load to start. The code really can't handle that, and
crashes when non-null mReader gets a new value.
I may have a patch soon, but not taking this bug quite yet.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 16 years ago

Keep things alive long enough but still allow nsOggDecodeStateMachine to proceed 
to dispatch NS_NEW_RUNNABLE_METHOD(nsOggDecoder, mDecoder, PlaybackEnded), which 
may end up to dispatch a DOM event "ended".

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 16 years ago

Comment on attachment 352583 [details] [diff] [review]
possible patch

>+  decodeStateMachine = nsnull;
>+  reader = nsnull;
Note, I kept this on purpose so that the order of destruction doesn't change.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 7 • 16 years ago

(In reply to comment #4)
> Ok, this is fun. Seems like shutting down the previous mDecodeThread may
> cause a new load to start.

Yeah, the main thread will run other events while it's waiting for the decode thread to terminate in Shutdown(), including JS events, which can do anything, like starting a new load...

Bug 468721's patch does the same thing as this one, except it uses an event to shut down the decode thread at a later time. I think that is important, because when we call Stop() from PlaybackEnded(), we don't want to run any JS before we post the ended event (by calling mElement->PlaybackEnded()) further down in nsOggDecoder::PlaybackEnded(). Without this, some JS timeout which starts a new load could fire in Stop(), and we'd then send an ended event to the media element right after that new load was started.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 16 years ago

Indeed bug 468721 has very similar patch. I guess it fixes this one too, so my
patch isn't needed.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 9 • 16 years ago

Bug 468721 is fixed1.9.1, so marking this one too.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 10 • 16 years ago

Verified fixed on trunk and 1.9.1 using builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090204 Minefield/3.2a1pre ID:20090204020339

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre ID:20090204020327

	Jesse Ruderman Reporter
	Comment 11 • 15 years ago

Crashtest: http://hg.mozilla.org/mozilla-central/rev/c38d6ec28c45