Closed Bug 1189698 Opened 4 years ago Closed 4 years ago

Race condition shutting down the old player thread

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: neil, Unassigned)

Details

Attachments

(2 files)

Attached file Stack backtrace
I have an add-on that invokes the nsISound API at certain times, however as of late it has become very crashy. Looking at the stack frames, I notice that the thread that nsSound is trying to shut down is already nullptr in the nsSound instance, and the value of "this" in the next lower frame points to deleted memory, so what I assume happened is that the sound releaser got dispatched while the main thread was trying to shut down the player thread. This would have caused the call to shutdown to fail, but the sound code doesn't check that.
Attached patch Proposed patchSplinter Review
By moving the value of mPlayerThread into a local the function becomes reentrant.
Attachment #8641653 - Flags: review?(jmathies)
Comment on attachment 8641653 [details] [diff] [review]
Proposed patch

Review of attachment 8641653 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsSound.cpp
@@ +124,5 @@
>  
>  void nsSound::ShutdownOldPlayerThread()
>  {
> +  nsCOMPtr<nsIThread> playerThread(mPlayerThread.forget());
> +  if (playerThread)

nit - brace me
Attachment #8641653 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/mozilla-central/rev/a48677d48570
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.