441197 - Gecko does not maintain an audio session on Vista+ (leading to no volume slider, inconsistent session title)

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Edward Copley]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14

If you are using firefox it would be very useful if there was a volume control in the vista volume mixer before the sounds started playing. There is one if a sound is being played but not before. I am not sure that this is really a bug, it might be a designed feature of the volume mixer, but it would be very handy if just having a firefox window open would cause the volume mixer to add a slider for firefox. 

Reproducible: Always

Steps to Reproduce:
1.Open firefox - no volume slider appears.
2.
3.
Actual Results:  
No volume slider appears when you open the firefox window. It appears after the sounds have started playing.

Expected Results:  
It would be useful if the slider appeared as the firefox window opened.

No theme in use. Nothing special going on. Happens on all computers I have used with all configurations and all versions of firefox.

Comment 1

[Jim Mathies [:jimm]]

I can confirm this. It's also interesting to note flash doesn't show up under our volume control. (IE exhibits the same behavior.) Something to look at.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Chrome does always provide a mixer.
Safari doesn't.
Strange.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So I've been looking into implementing this.  The main barrier is that Gecko doesn't have a persistent audio object of any sort that can maintain the necessary Windows COM interfaces.  It's also worth noting that this is why Firefox et. al. do not appear as "Mozilla Firefox" above the volume slider but rather as the title of the currently focused window.

Comment 4

[Rob Arnold [:robarnold]]

Could we steal nsAppShell or create a new object with the same lifetime for this purpose?

Comment 5

[Rob Arnold [:robarnold]]

Also might be worth considering how this interacts with multiple processes and our multi-process plans.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #5)
> Also might be worth considering how this interacts with multiple processes and
> our multi-process plans.

Indeed.  The Audio API on Vista+ supports cross-process audio sessions so we can handle this.  I think we may even be able to leverage the API into controlling the audio streams in mozilla-runtime processes, making something like per-tab volume control possible

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

On trunk mozilla-runtime and firefox show up separately with OOPP.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The audio session GUID is roughly equivalent to the AppUserModelID the shell uses.  The difference is that we need that GUID to be unique per running session (i.e. one firefox.exe and it's associated mozilla-runtime.exes should share a GUID)  I think we'll need to generate a GUID at runtime and pass it across the proxy boundary.  This also may be wanted before OOPP ships as users are going to be very confused when they have to change the volume on "mozilla-runtime.exe"

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

After thinking about this a bit more, it makes more sense to use the GUID as a "Grouping Parameter"[0] rather than the actual session GUID.  It's a subtle difference, but it means we don't have to worry about what Flash/et. al. do on the native side of things.

[0] http://msdn.microsoft.com/en-us/library/dd370848%28VS.85%29.aspx

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I have a patch for the non-OOPP parts of this, which has raised a number of questions.

Per the grouping parameter link in Comment 9, "In the case of a volume control for a grouping, Sndvol arbitrarily selects one of the sessions in the grouping as the source for the display name and icon that it displays with the volume control. Thus, to ensure that Sndvol always displays the same display name and icon for a grouping, all application instances that assign sessions to that grouping should ensure that their respective sessions have the same display name and icon."   

This means that I need to set the Audio Session's title and icon on both sides.  On the parent side this isn't too difficult (I'm currently using nsIXulAppInfo::name for the title) but it gets much harder on the child side.  Do I have access to things like nsIXulAppInfo on the child side?  If not, do we want to create subprotocols for platform specific stuff because we're going to need to pass at least three items across the boundary just for this simple audio issue.  It seems like cluttering up PPluginModule with that is at best inelegant.

Also, I assume the default behavior should be to fail silently (especially on the child side, not being able to give a title to the audio shouldn't quash the entire plugin load IMO).

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Also testing other browsers, IE 8 gets this right (not surprising since the Grouping Parameter was created for IE 8 and the browser use case) and Chrome gets it wrong (Not all the processes are grouped.)

Comment 12

[Jim Mathies [:jimm]]

(In reply to comment #10)
> Do I have access to things like nsIXulAppInfo on the child side?  If not, do we

No.

> want to create subprotocols for platform specific stuff because we're going to
> need to pass at least three items across the boundary just for this simple
> audio issue.  It seems like cluttering up PPluginModule with that is at best
> inelegant.

Yes. See the patch in bug 563381. This is really easy to add. Do you still have this in your queue Kyle? Might be nice to get this landed. (I'm going to a file bug now on the icon issue.)

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

> (In reply to comment #10)
> Yes. See the patch in bug 563381. This is really easy to add. Do you still have
> this in your queue Kyle? Might be nice to get this landed. (I'm going to a file
> bug now on the icon issue.)

Yes.  I'll dig it out and see if I can't get it done relatively soon.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Screenshot w/patch applied

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

I'm cheating on some of the XPCOM stuff as the patch explains.  We can do it correctly if you like but I think this is good enough for the moment.  Patch itself works great.  It's worth noting that MSDN says that SndVol can pick any session to take the name and icon from but AFAICT it always takes the first one (meaning the parent process) which is nice because it means that we don't have to handle the icon stuff.

This applies over the patch in Bug 543061.  I threw this at try a few minutes ago and will post the build link when it is complete.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch 1.1

Now with IDL

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

There's also probably no need for this to be scriptable.  That came along from the copy/paste.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://build.mozilla.org/tryserver-builds/[e-mail]-try-3abfbbe7ad87/

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch 1.2

And this version doesn't blow up every other platform.

Comment 21

[James May [:fowl2]]

Does this work with out of process plugins? I've noticed since OOPP I can control flash's volume independently, which is good, I think.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This groups the plugins together with the parent process so the volume of everything is changed together.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

There's some weirdness where if you launch the app once and load a youtube page or some other plugin it works, but if you close the app and relaunch it they show up separately again.  I think it's some sort of caching thing in SndVol.

Comment 24

[Jim Mathies [:jimm]]

Kyle, can I be a pain and ask that you split the plugin and widget code into two patches? One will have to go through ipc reviewers, the other through widget people.

(also, fyi, ipc reviewers tedn to be sticklers for 80 character limits on lines. Been there, done that!)

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Widget patch

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Ipc patch

Jim, can you suggest a reviewer for this?

There is some weirdness with closing and reopening the browser if you keep SndVol open throughout, but I'm not sure whether that's in my patch or in SndVol.

Comment 28

[Jim Mathies [:jimm]]

Comment on attachment 443968 [details] [diff] [review]
Widget patch

> PRUnichar *buffer;
> mIconPath.GetMutableData(&buffer, MAX_PATH);
> 
> if (::GetModuleFileName(NULL, buffer, MAX_PATH)) {
>   mIconPath.AppendLiteral(",0");
> }

After calling GetMutableData, the string length is set MAX_PATH. You need to terminate this and set the length manually based on the response, or just use a temporary [MAXPATH] buffer. Your append here basically fails. (actually it appends to the end at MAXPATH.) Although maybe we don't need this since windows grabs the first index by default..

> if (hr != S_OK)

Generally lets try to stick with COM's FAILED macro on these. 

Formatting nit - 

STDMETHODIMP
AudioSession::QueryInt

vs.

> STDMETHODIMP AudioSession::QueryInt

to keep lengths down.

>  	JumpListItem.cpp \
>+  AudioSession.cpp \
> 	$(NULL)

Try and clean up the formatting here, looks like the convention is to use a tab.

+  nsCOMPtr<nsIWinAudioSession> audioSession = 
+    do_GetService("@mozilla.org/windows-audio-session;1", &rv);
+  NS_ENSURE_SUCCESS(rv, rv);
+  NS_ASSERTION(audioSession, "Could not create AudioSession object");
+  rv = audioSession->Start(); // This can fail if no audio devices exist
+                              // Or if we're running on a pre-Vista OS
+  if (NS_FAILED(rv))
+    audioSession = NULL;
+#endif

Maybe just manually warn here? I don't think we want to use NS_ENSURE_SUCCESS.

What kind of startup hit are we talking here? When you land this check perf stats to see if this effects start up times, or alternatively, is there any way we could delay calling Start() a bit? Either way we should always being thinking about ways to avoid added startup code.

Also, note roc will need to sr on interface / design.

Comment 29

[Jim Mathies [:jimm]]

(In reply to comment #26)
> Created an attachment (id=443975) [details]
> Ipc patch
> 
> Jim, can you suggest a reviewer for this?
>

:bsmedberg would probably be best for this.

Comment 30

[Jim Mathies [:jimm]]

Hmm, also, I don't believe the path/icon code will work for xulrunner apps, which use the firefox.exe as a base. 

https://developer.mozilla.org/en/Getting_started_with_XULRunner

Might want to test with the sample and make sure your audio code works properly.

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #28)
> 
> +  nsCOMPtr<nsIWinAudioSession> audioSession = 
> +    do_GetService("@mozilla.org/windows-audio-session;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ASSERTION(audioSession, "Could not create AudioSession object");
> +  rv = audioSession->Start(); // This can fail if no audio devices exist
> +                              // Or if we're running on a pre-Vista OS
> +  if (NS_FAILED(rv))
> +    audioSession = NULL;
> +#endif
> 
> Maybe just manually warn here? I don't think we want to use NS_ENSURE_SUCCESS.

Not sure what the reasoning is here.  nsIWinAudioSession exists even when we're not on Vista and can't not exist now that malloc is infallible.  I actually think the code might be neater if AudioSession::GetSingleton fails on pre-Vista platforms so that consumers of the service only need to care about whether the service exists or not.

> What kind of startup hit are we talking here? When you land this check perf
> stats to see if this effects start up times, or alternatively, is there any way
> we could delay calling Start() a bit? Either way we should always being
> thinking about ways to avoid added startup code.
> 
> Also, note roc will need to sr on interface / design.

Unless we don't initialize COM during startup normally I would imagine it's fairly low.  This definitely needs to be measured though.  I'm not sure how we could delay it though, we need to establish the session before we play any audio and before we load any plugins (because they may play audio).

Comment 32

[Jim Mathies [:jimm]]

I think you'll need NS_IMPL_THREADSAFE_ISUPPORTS1, I'm seeing this on startup - 

###!!! ASSERTION: AudioSession not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file f:/Mozilla/firefox/mc/widget/src/windows/AudioSession.cpp, line 85
###!!! ASSERTION: AudioSession not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file f:/Mozilla/firefox/mc/widget/src/windows/AudioSession.cpp, line 85
###!!! ASSERTION: AudioSession not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file f:/Mozilla/firefox/mc/widget/src/windows/AudioSession.cpp, line 85
###!!! ASSERTION: AudioSession not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file f:/Mozilla/firefox/mc/widget/src/windows/AudioSession.cpp, line 85

Comment 33

[Jim Mathies [:jimm]]

(In reply to comment #31)
 I actually
> think the code might be neater if AudioSession::GetSingleton fails on pre-Vista
> platforms so that consumers of the service only need to care about whether the
> service exists or not.

^^ Thought that was the way it worked. Sounds like a good idea.

Comment 34

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We should fix this for final, it's a fairly simple polish issue.  Hopefully I'll find some time to fix this up and get it in for another round of reviews soon,

Comment 35

[Joe Drew (not getting mail)]

We should fix this, but it's hard for me to say this blocks. Feel free to renom if you feel strongly.

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't have time to finish this, but it should be fairly straightforward for somebody to pick up these patches and shepherd them into the tree.

Comment 37

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I want to at least get the non-IPC part of this into 2.0.

Comment 38

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: widget/ patch

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: ipc patch

Comment 40

[Benjamin Smedberg]

Comment on attachment 524186 [details] [diff] [review]
ipc patch

I'm going to let jimm review both halves of this.

Comment 41

[Jim Mathies [:jimm]]

Comment on attachment 524185 [details] [diff] [review]
widget/ patch

Review of attachment 524185 [details] [diff] [review]:

all in all nice looking code.

::: widget/src/windows/AudioSession.cpp
@@ +15,5 @@
+ *
+ * The Original Code is mozilla.org code.
+ *
+ * The Initial Developer of the Original Code is
+ *   Kyle Huey <[e-mail]>

nit - This should be Mozilla Foundation w/ your name in the contributors list.

@@ +16,5 @@
+ * The Original Code is mozilla.org code.
+ *
+ * The Initial Developer of the Original Code is
+ *   Kyle Huey <[e-mail]>
+ * Portions created by the Initial Developer are Copyright (C) 2010

nit - 2011

@@ +45,5 @@
+#include "nsIStringBundle.h"
+#include "nsIUUIDGenerator.h"
+#include "nsIXULAppInfo.h"
+
+//#include "AudioSession.h"

nit - cleanup

@@ +162,5 @@
+// IAudioSessionEvents interface that will keep us alive until the appshell
+// calls Stop.
+nsresult
+AudioSession::Start()
+{

Lets make sure and run this through Talos looking for startup regressions.

@@ +228,5 @@
+  hr = manager->GetAudioSessionControl(NULL,
+                                       FALSE,
+                                       getter_AddRefs(mAudioSessionControl));
+  if (FAILED(hr))
+    return NS_ERROR_FAILURE;

On these failures, should we also consider calling Stop? It looks like we leave things in an unknown state.

@@ +303,5 @@
+
+STDMETHODIMP
+AudioSession::OnSessionDisconnected(AudioSessionDisconnectReason aReason)
+{
+  mAudioSessionControl->UnregisterAudioSessionNotification(this);

Lets check mAudioSessionControl just to be safe.

Comment 42

[Jim Mathies [:jimm]]

Comment on attachment 524186 [details] [diff] [review]
ipc patch

Review of attachment 524186 [details] [diff] [review]:

r+ with comments addressed.

::: dom/plugins/PluginModuleChild.cpp
@@ +669,5 @@
     }
 
+#if defined XP_WIN && MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
+    mozilla::widget::StopAudioSession();
+#endif

I think QuickExit is only used when we crash. We also want to shutdown on normal shut downs in AnswerNP_Shutdown. (A little debugging would confirm this.)

::: dom/plugins/PluginModuleParent.cpp
@@ +781,5 @@
+    nsID id;
+    nsString sessionName;
+    nsString iconPath;
+    rv = mozilla::widget::GetAudioSessionData(id, sessionName, iconPath);
+    if (NS_SUCCEEDED(rv)) {

Lets wrap these together and ditch rv.

@@ +783,5 @@
+    nsString iconPath;
+    rv = mozilla::widget::GetAudioSessionData(id, sessionName, iconPath);
+    if (NS_SUCCEEDED(rv)) {
+        if (!SendSetAudioSessionData(id, sessionName, iconPath))
+            return NS_ERROR_FAILURE;

I don't think we want to fail NP_Initialize here. Just call it and if it fails, oh well. If something is seriously wrong, CallNP_Initialize will catch it below.

::: widget/src/windows/AudioSession.cpp
@@ +328,5 @@
+{
+  lhs.m0 = rhs.m0;
+  lhs.m1 = rhs.m1;
+  lhs.m2 = rhs.m2;
+  for (int i = 0; i < 8; i++ ) {

nit - white space

@@ +339,5 @@
+                             nsString& aSessionName,
+                             nsString& aIconPath)
+{
+  NS_ABORT_IF_FALSE(mState != UNINITIALIZED && mState != STOPPED,
+                    "State invariants violated");

Do we really want to abort here if the audio session failed to startup properly?

Comment 43

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #41)
> Comment on Review of attachment 524185 [details] [diff] [review]:
> widget/ patch
> 
> attachment 524185 [details] [diff] [review]
> 
> all in all nice looking code.

Thanks.

> ::: widget/src/windows/AudioSession.cpp
> @@ +15,5 @@
> + *
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + *   Kyle Huey <[e-mail]>
> 
> nit - This should be Mozilla Foundation w/ your name in the contributors list.

I'm not a Mozilla employee.

> @@ +16,5 @@
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + *   Kyle Huey <[e-mail]>
> + * Portions created by the Initial Developer are Copyright (C) 2010
> 
> nit - 2011
> 
> @@ +45,5 @@
> +#include "nsIStringBundle.h"
> +#include "nsIUUIDGenerator.h"
> +#include "nsIXULAppInfo.h"
> +
> +//#include "AudioSession.h"
> 
> nit - cleanup
> 
> @@ +162,5 @@
> +// IAudioSessionEvents interface that will keep us alive until the appshell
> +// calls Stop.
> +nsresult
> +AudioSession::Start()
> +{
> 
> Lets make sure and run this through Talos looking for startup regressions.
> 
> @@ +228,5 @@
> +  hr = manager->GetAudioSessionControl(NULL,
> +                                       FALSE,
> +                                       getter_AddRefs(mAudioSessionControl));
> +  if (FAILED(hr))
> +    return NS_ERROR_FAILURE;
> 
> On these failures, should we also consider calling Stop? It looks like we leave
> things in an unknown state.

Yes, probably.  At least the part that clears out mAudioSessionControl.

> @@ +303,5 @@
> +
> +STDMETHODIMP
> +AudioSession::OnSessionDisconnected(AudioSessionDisconnectReason aReason)
> +{
> +  mAudioSessionControl->UnregisterAudioSessionNotification(this);
> 
> Lets check mAudioSessionControl just to be safe.

OK.

(In reply to comment #42)
> Comment on Review of attachment 524186 [details] [diff] [review]:
> ipc patch
> 
> attachment 524186 [details] [diff] [review]
> 
> r+ with comments addressed.
> 
> ::: dom/plugins/PluginModuleChild.cpp
> @@ +669,5 @@
>      }
> 
> +#if defined XP_WIN && MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
> +    mozilla::widget::StopAudioSession();
> +#endif
> 
> I think QuickExit is only used when we crash. We also want to shutdown on
> normal shut downs in AnswerNP_Shutdown. (A little debugging would confirm
> this.)

Interesting.  I'll verify and change.  That might explain the odd session lingering behavior I saw a couple times but couldn't reproduce.

> ::: dom/plugins/PluginModuleParent.cpp
> @@ +781,5 @@
> +    nsID id;
> +    nsString sessionName;
> +    nsString iconPath;
> +    rv = mozilla::widget::GetAudioSessionData(id, sessionName, iconPath);
> +    if (NS_SUCCEEDED(rv)) {
> 
> Lets wrap these together and ditch rv.

OK.

> @@ +783,5 @@
> +    nsString iconPath;
> +    rv = mozilla::widget::GetAudioSessionData(id, sessionName, iconPath);
> +    if (NS_SUCCEEDED(rv)) {
> +        if (!SendSetAudioSessionData(id, sessionName, iconPath))
> +            return NS_ERROR_FAILURE;
> 
> I don't think we want to fail NP_Initialize here. Just call it and if it fails,
> oh well. If something is seriously wrong, CallNP_Initialize will catch it
> below.

OK.

> ::: widget/src/windows/AudioSession.cpp
> @@ +328,5 @@
> +{
> +  lhs.m0 = rhs.m0;
> +  lhs.m1 = rhs.m1;
> +  lhs.m2 = rhs.m2;
> +  for (int i = 0; i < 8; i++ ) {
> 
> nit - white space
> 
> @@ +339,5 @@
> +                             nsString& aSessionName,
> +                             nsString& aIconPath)
> +{
> +  NS_ABORT_IF_FALSE(mState != UNINITIALIZED && mState != STOPPED,
> +                    "State invariants violated");
> 
> Do we really want to abort here if the audio session failed to startup
> properly?

No, this is bogus.  We probably want another state "FAILED" or something.  Aborting on shutdown on Windows XP is probably not the way to go here.

I'll fix this up this weekend.

Comment 44

[Jim Mathies [:jimm]]

Looks like it would be trivial to add a fix for bug 598792 after this lands.

Comment 45

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So I think I'm going to hold off on this until after we merge to aurora next week.  This seems like the kind of thing that should get some shaking out on the nightly branch first.

Comment 46

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch - Serialize nsID

Unbitrotted.  This is from the other bug.

Comment 47

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: widget/ patch

Unbitrotted and comments addressed.

Comment 48

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: ipc/ patch

Unbitrotted and comments addressed.

Comment 49

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So there's still one problem here.  The plugin process fails to print leak stats when run with this patch with certain plugins.  In particular, it breaks with the test plugin and with silverlight, but works with flash.

Comment 50

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

If I add an fflush call to the trace ref count stuff that fixes it ... but that's presumably wall paper over a real problem.

Comment 51

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Don't rely on the system to close and flush our file descriptors

bent and I looked at this for some time this morning, and we couldn't find any misusage of the API in our patch.  We also noticed that we don't even fflush or fclose these file descriptors, so lets do that.

Comment 52

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/projects/build-system/rev/ffc023482b24
http://hg.mozilla.org/projects/build-system/rev/d004850a8358
http://hg.mozilla.org/projects/build-system/rev/3296e9564de6

Jumping the gun a bit here; if dbaron has comments I'll fix them in a followup.

Comment 53

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/ffc023482b24
http://hg.mozilla.org/mozilla-central/rev/d004850a8358
http://hg.mozilla.org/mozilla-central/rev/3296e9564de6

Comment 54

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 541118 [details] [diff] [review]
widget/ patch

>diff --git a/widget/src/windows/nsAppShell.cpp b/widget/src/windows/nsAppShell.cpp
>--- a/widget/src/windows/nsAppShell.cpp
>+++ b/widget/src/windows/nsAppShell.cpp
>@@ -244,10 +245,20 @@ nsAppShell::Run(void)
> {
>   LoadedModuleInfo modules[NUM_LOADEDMODULEINFO];
>   memset(modules, 0, sizeof(modules));
>-  sLoadedModules = modules;
>+  sLoadedModules = modules;	

Trailing space :(

Comment 55

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 541119 [details] [diff] [review]
ipc/ patch

>diff --git a/dom/plugins/ipc/PluginModuleParent.cpp b/dom/plugins/ipc/PluginModuleParent.cpp
>--- a/dom/plugins/ipc/PluginModuleParent.cpp
>+++ b/dom/plugins/ipc/PluginModuleParent.cpp
>@@ -757,6 +761,8 @@ PluginModuleParent::NP_Initialize(NPNets
> {
>     PLUGIN_LOG_DEBUG_METHOD;
> 
>+    nsresult rv;
>+

Unused?

>     mNPNIface = bFuncs;
> 
>     if (mShutdown) {
>@@ -764,6 +770,18 @@ PluginModuleParent::NP_Initialize(NPNets
>         return NS_ERROR_FAILURE;
>     }
> 
>+#if defined XP_WIN && MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
>+    // Send the info needed to join the chrome process's audio session to the
>+    // plugin process
>+    nsID id;
>+    nsString sessionName;
>+    nsString iconPath;
>+
>+    if (NS_SUCCEEDED(mozilla::widget::GetAudioSessionData(id, sessionName,
>+                                                          iconPath)))
>+        SendSetAudioSessionData(id, sessionName, iconPath);
>+#endif
>+
>     if (!CallNP_Initialize(&mPluginThread, error))
>         return NS_ERROR_FAILURE;
>