Last Comment Bug 714078 - Code in AudioSession::OnSessionDisconnected needs to run asynchronously
: Code in AudioSession::OnSessionDisconnected needs to run asynchronously
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-29 08:42 PST by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2011-12-30 05:07 PST (History)
1 user (show)
khuey: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.46 KB, patch)
2011-12-29 08:45 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
ehsan: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-29 08:42:01 PST
http://msdn.microsoft.com/en-us/library/windows/desktop/dd368289%28v=VS.85%29.aspx

>In implementing the IAudioSessionEvents interface, the client should observe these rules to avoid deadlocks and undefined behavior:
>
>    The methods in the interface must be nonblocking. The client should never wait on a synchronization object during an event callback.
>    The client should never call the IAudioSessionControl::UnregisterAudioSessionNotification method during an event callback.
>    The client should never release the final reference on a WASAPI object during an event callback.

I hit a deadlock at shutdown today that looked like it might be a result of us doing this.

Patch coming up.
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-29 08:45:04 PST
Created attachment 584764 [details] [diff] [review]
Patch
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-29 08:46:57 PST
Technically NS_DispatchToMainThread takes a synchronization object ... but I think we can get away with that since it's a Gecko-only object.
Comment 3 :Ehsan Akhgari 2011-12-29 08:49:36 PST
Comment on attachment 584764 [details] [diff] [review]
Patch

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

r=me

::: widget/src/windows/AudioSession.cpp
@@ +84,5 @@
>    STDMETHODIMP OnDisplayNameChanged(LPCWSTR aDisplayName, LPCGUID aContext);
>    STDMETHODIMP OnGroupingParamChanged(LPCGUID aGroupingParam, LPCGUID aContext);
>    STDMETHODIMP OnIconPathChanged(LPCWSTR aIconPath, LPCGUID aContext);
>    STDMETHODIMP OnSessionDisconnected(AudioSessionDisconnectReason aReason);
> +  nsresult OnSessionDisconnectedInternal();

Make this private please.

@@ +451,5 @@
>    mAudioSessionControl->UnregisterAudioSessionNotification(this);
>    mAudioSessionControl = nsnull;
> +
> +  Start(); // If it fails there's not much we can do
> +  return NS_OK;

Allow me to express my disgust over (N)S_OK.  :-)
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-29 12:51:17 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef6d44e60875
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-30 05:07:01 PST
https://hg.mozilla.org/mozilla-central/rev/ef6d44e60875

Note You need to log in before you can comment on or make changes to this bug.