Closed Bug 466945 Opened 14 years ago Closed 14 years ago

nsStreamStrategy::~nsStreamStrategy triggers ptsynch.c lock assertions


(Core :: Audio/Video, defect, P2)






(Reporter: jruderman, Unassigned)



(4 keywords, Whiteboard: [sg:critical?])


(2 files)

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).
Flags: blocking1.9.1?
Whiteboard: [sg:critical?]
Flags: blocking1.9.1? → blocking1.9.1+
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.
On the other hand, maybe not. liboggplay appears to be doing something else. Discussing with the liboggplay maintaners...
This seems to be resulting in lots of different assertions and crashes, which gets in my way :(
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.
Attached patch possible patchSplinter Review
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".
Assignee: nobody → Olli.Pettay
Attachment #352583 - Flags: review?(chris.double)
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.
(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.
Indeed bug 468721 has very similar patch. I guess it fixes this one too, so my
patch isn't needed.
Depends on: 468721
Attachment #352583 - Flags: review?(chris.double)
Assignee: Olli.Pettay → nobody
Closed: 14 years ago
Resolution: --- → FIXED
Bug 468721 is fixed1.9.1, so marking this one too.
Keywords: fixed1.9.1
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
Flags: in-testsuite?
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Flags: wanted1.9.0.x-
Flags: wanted1.8.1.x-
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.