Closed Bug 679375 Opened 8 years ago Closed 8 years ago
Sound can cause GC to be reentered
nsSound can get released as part of garbage collection. nsSound::~nsSound tries to synchronously shut down its player thread. This works by spinning an event loop. But that means that it's possible to, say, fire a GC timer event. Reentering GC is disallowed and causes the application to crash.
> But that means that it's possible to, say, fire a GC timer event. How can I fire a GC timer event?
Well, by "GC timer event" I think he means "any event that could trigger a GC". We have lots of those. I haven't really looked into it but one simple solution is probably to just join the thread asynchronously (e.g. dispatch a runnable to the main thread that shuts down the sound thread).
Well, in my case, it was an actual timer whose purpose is to call GC.
> Well, in my case, it was an actual timer whose purpose is to call GC. I guess I'm interested in how to manually call GC. For example I guess I could release an extra ref on the object early but I think you mean something more explicit. Maybe if you could point me to an MDC doc or source file?
As long as you're on the main thread you can call nsJSContext::GarbageCollect() and nsJSContext::CycleCollect().
great, thank you!
Assignee: nobody → netzen
Attachment #556531 - Flags: review?(neil)
Comment on attachment 556531 [details] [diff] [review] Fixed GC reentered crash by dispatching to main thread for thread shutdown Sorry for the drive-by but I'd suggest continuing to null out mPlayerThread for sanity.
> Sorry for the drive-by but I'd suggest continuing to null out mPlayerThread for sanity. No problem, done.
You could probably improve that further by transferring the reference from the nsSound object directly to the new object, although offhand I'm not sure what the ideal way of doing that is (swap and forget are two options for instance).
It's not clear to me why we're joining the sound thread from the destructor. Shouldn't we join the sound thread from the sound releaser?
(In reply to comment #13) > It's not clear to me why we're joining the sound thread from the destructor. > Shouldn't we join the sound thread from the sound releaser? (and why so many sound threads anyway?) (In reply to comment #12) > (swap and forget are two options for instance). I don't like swap, because it doesn't allow you to construct the new nsCOMPtr. But I'm not sure whether it's better to pass the nsSound object and forget its member or forget its member and pass the already_AddRefed<nsIThread> object to the constructor.
> and why so many sound threads anyway? That's a great question. It's not my code but a lot of that threading code doesn't seem right to me in Win32's nsSound. How it currently works: 0. nsSound::PlaySystemSound or nsSound::PlayEventSound is called 1. It calls shutdown() right away on the existing nsSound::mPlayerThread thread if one exists (shutdown() = Join and not accept new dispatches) 2. Create a new nsSoundPlayer (which is a runnable event), it calls AddRef on the nsSound object to ensure the nsSound destructor won't be hit 3. Create a new thread and store it in nsSound::mPlayerThread 4. Dispatch the runnable event (nsSoundPlayer) to the thread we just created (nsSound::mPlayerThread). 5. Inside the nsSoundPlayer thread we dispatch back to the main thread which calls release on the nsSound. ::PlaySoundW should already be async if you pass SND_ASYNC ( http://msdn.microsoft.com/en-us/library/dd743680(v=vs.85).aspx ) which we do. So it would seem that all this threading is pointless. But I looked in the hg logs and seen Bug 498079. Inside Bug 498079 we did all of this threading because of a reported performance problem. I think the reasoning ended up being that some sound cards have bugs and cause non async handling contrary to MSDN documentation. I'm not sure if that is valid or not though, but I'll assume it is. Shouldn't @mozilla.org/sound;1 be an XPCOM service and not instantiate a new object each time we may have sound by the way? Even if we fixup the nsSound to have a single thread, instead of one per call, it would make no difference currently in our usage of nsISound. We always create a new object for each sound we play, so there would still be 1 thread per sound. It doens't make sense to me to have a whole thread startup and shutdown every time a sound MAY be played. And on Windows most of these sounds are defaulted to off, yet we will still create a new thread anyway in case they are turned on. Currently it will create a new thread just for opening a menu even know the sound is turned off by default for menu opens. Open task manager and set the update frequency to high and show thread counts. Then expand and collapse the top left menu, you will see it startup and shutdown a new thread for each time you click the firefox menu. I would like to propose a shared single Win32 Widget thread. Currently it could be used by each instance of nsSound, JumpList icon caching, and I think also LSPAnnotate. This would eliminate the startup / teardown of a thread for trivial things and also consolodate 2 threads into one and make it so we have a placeholder for making more things async in the future Win32 widget code. Thoughts?
> I would like to propose... Or perhaps propose that we make @mozilla.org/sound;1 a service and update the Win32 nsSound to have only one thread which gets created in the constructor and gets shutdown in the destructor. Or better yet gets created when we first need it so that it doesn't cause any slowdown to startup.
Just found this, so perhaps there is nothing to do here? Bug 520417
I think the playing sound thread should be independent from other work. If it were in trouble, the other jobs would be stopped too. I think nsSound should be a singleton class (service), see also bug 520417.
Ya after thinking about it more I agree with your Comment 18 and my Comment 16 more. The last update to Bug 520417 was 2010-03-01, is it still being worked on?
No. If you would take it, please :-) Actually, the bug's range is too wide. So, if you wanted to make nsSound a singleton service, you should do it in this bug.
Ok thanks for the info Masayuki. Neil, I suggest Comment 16, but please let me know your thoughts.
Comment on attachment 556593 [details] [diff] [review] Fixed GC reentered crash by dispatching to main thread for thread shutdown v2 I guess for the purposes of this bug we should go with this patch as a stopgap.
Attachment #556593 - Flags: review?(neil) → review+
As per Neil's idea.
Comment on attachment 556864 [details] [diff] [review] Alternate patch which uses the SoundReleaser to shutdown the mPlayerThread thread >+ // Each event on mPlayerThread holds a reference to nsISound so we will >+ // only reach here if the thread is already done. >+ mPlayerThread.forget(); Not sure what you're trying to do here... >- mPlayerThread = nsnull; >+ mPlayerThread.forget(); Why this change?
Maybe I am misunderstanding the use of forget()? I thought it nulls out the pointer? > forget() > 764 // return the value of mRawPtr and null out mRawPtr. >+ // Each event on mPlayerThread holds a reference to nsISound so we will >+ // only reach here if the thread is already done. >+ mPlayerThread.forget(); > Not sure what you're trying to do here... The whole comment and line of code can be removed. I normally wouldn't NULL out anything in a destructor but as per Comment 3 I thought it was wanted. >- mPlayerThread = nsnull; >+ mPlayerThread.forget(); > Why this change? I thought you preferred it that way because of Comment 12. Let me know how to proceed, thanks for the review.
(In reply to Brian R. Bondy from comment #25) > Maybe I am misunderstanding the use of forget()? I thought it nulls out the pointer? It does, but it also returns the previous value in a form that is intended for assignment into another nsCOMPtr, or into an outparam. (It leaks if you don't.)
ah thanks for explaining, makes sense that it has to keep a reference since it is returning it. I'll update both cases in Comment 24 to nsnull.
Implemented review comments (.forget() to = nsnull)
Comment on attachment 557065 [details] [diff] [review] Alternate patch which uses the SoundReleaser to shutdown the mPlayerThread thread v2' >+ // Each event on mPlayerThread holds a reference to to the nsSound, so >+ // the nsSound destructor will not be called if there are events left on >+ // the thread. So we do not need to call shutdown() on the mPlayerThread. >+ mPlayerThread = nsnull; It just occurs to me that we can probably do better than this - we should be able to assert that mPlayerThread is already null, since the player thread dispatches an event that nulls it out. (You don't actually need to explicitly null it out anyway, because its destructor will do that.)
Comment on attachment 558023 [details] [diff] [review] Alternate patch which uses the SoundReleaser to shutdown the mPlayerThread thread v3 No assertions seen locally :-)
Attachment #558023 - Flags: review?(neil) → review+
/me ducksed, cancelled, and re-pushed to try with Win32 only to avoid Neil's thwapping: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=0c3517cfa0c0
Pushed to mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/9f853ef4b664
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.