1449388 - Crash in CLockedList::ForEachEntry in plugin process

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

[David Parks [:handyman]]

This crash was previously investigated in bug 1436972.

The crash is pretty old but it used to be a trickle until 2/6/18, when it became common.  A few things went in around that point that could be related but I think I've isolated the cause of the spike to turning on restricting SIDs in the plugin sandbox in bug 1426733.

This crash is in Windows code but it appears to be related to notifying the process of a change to the audio device -- this is usually something like plugging in headphones.  I recently noticed that Flash plugins no longer recognize things like such device changes -- mozregression shows the bug happened on 2/6.  This [1] is the push log, with bug 1426733 standing out as most related.

---

The Windows code looks like it might be looking at the registry.  Specifically, I think it might be looking at one/some of these [2].  If so, we can open the keys it needs (as long as they are safe).


[1] https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=66a85897ecb2d7b1a8b533a657b6767b117a262e&full=1
[2] http://www.windowrdb.com/w.php?w=hkcu-software-microsoft-internet-explorer-lowregistry

Comment 1

[David Parks [:handyman]]

Update: The device change problem doesn't go away by opening up the registry.  Strangely, I'm not seeing that Flash is using our audio device notification mechanism [1], which was added because the old API didn't work under sandbox level 2.  Either I'm interpreting the debugger behavior wrong or they found a different way to catch the audio changes... a way that is now broken again.

[1] https://searchfox.org/mozilla-central/source/dom/plugins/base/npapi.h#403

Comment 2

[David Parks [:handyman]]

I'm not sure if Flash ever used the NPPVpluginRequiresAudioDeviceChanges setting but its not using it now.  This is the reason the audio device doesn't change when connecting headphones and is highly likely to be a prerequisite for this crash.  This behavior happens with the plugin when the sandbox level _doesn't_ block device change notes:

>  1327681 3:28:11.698 PM  1 NPSWF64_29_0_0_113.dll  CoCreateInstance ( MMDeviceEnumerator class, NULL, CLSCTX_INPROC_SERVER, IMMDeviceEnumerator, 0x00000053861fe6e0 )  S_OK    0.0000481 Component Object Model (COM) / COM Fundamentals
>  1327687 3:28:11.701 PM  1 NPSWF64_29_0_0_113.dll  IMMDeviceEnumerator::GetDefaultAudioEndpoint ( eRender, eMultimedia, 0x000001dca23e9470 ) S_OK    0.0003329 Audio and Video / Core Audio / Windows Multimedia Device
>  1327882 3:28:11.704 PM  1 NPSWF64_29_0_0_113.dll  GetVersionExW ( 0x00000053861fe710 )  TRUE    0.0000011 System Services / Windows System Information / System Information
>  1327884 3:28:11.704 PM  1 NPSWF64_29_0_0_113.dll  IMMDeviceEnumerator::Release (  ) 4   0.0000000 Audio and Video / Core Audio / Windows Multimedia Device
>  1327885 3:28:11.704 PM  1 NPSWF64_29_0_0_113.dll  CreateEventW ( NULL, FALSE, FALSE, NULL ) 0x0000000000000b18    0.0000026 System Services / Synchronization / Event
>  1327888 3:28:11.704 PM  1 NPSWF64_29_0_0_113.dll  CreateEventW ( NULL, FALSE, FALSE, NULL ) 0x0000000000000b1c    0.0000007 System Services / Synchronization / Event
>  1327891 3:28:11.704 PM  1 NPSWF64_29_0_0_113.dll  CreateEventW ( NULL, FALSE, FALSE, NULL ) 0x0000000000000b24    0.0000004 System Services / Synchronization / Event
>  1327894 3:28:11.704 PM  1 NPSWF64_29_0_0_113.dll  IMMDevice::Activate ( IAudioClient, 1, NULL, 0x000001dca23e9478 ) S_OK    0.0002863 Audio and Video / Core Audio / Windows Multimedia Device
>  1328028 3:28:11.707 PM  1 NPSWF64_29_0_0_113.dll  CoCreateInstance ( MMDeviceEnumerator class, NULL, CLSCTX_INPROC_SERVER, IMMDeviceEnumerator, 0x000001dca23e9488 )  S_OK    0.0000113 Component Object Model (COM) / COM Fundamentals
>  1328036 3:28:11.707 PM  1 NPSWF64_29_0_0_113.dll  IAudioClient::IsFormatSupported ( AUDCLNT_SHAREMODE_SHARED, 0x000001dca23e9528, 0x000001dca23e9520 )  S_FALSE   0.0050890 Audio and Video / Core Audio / Windows Audio Session
>  [...]
>  1328161 3:28:11.714 PM  1 NPSWF64_29_0_0_113.dll  IAudioClient::Initialize ( AUDCLNT_SHAREMODE_SHARED, 262144, 230000, 0, 0x000001dca6a19b70, NULL )  S_OK    0.1376399 Audio and Video / Core Audio / Windows Audio Session
>  1328327 3:28:11.853 PM  1 NPSWF64_29_0_0_113.dll  IAudioClient::GetBufferSize ( 0x000001dca23e9544 )  S_OK    0.0000011 Audio and Video / Core Audio / Windows Audio Session
>  1328329 3:28:11.853 PM  1 NPSWF64_29_0_0_113.dll  IAudioClient::SetEventHandle ( 0x0000000000000b1c ) S_OK    0.0000653 Audio and Video / Core Audio / Windows Audio Session
>  1328334 3:28:11.853 PM  1 NPSWF64_29_0_0_113.dll  IAudioClient::GetService ( IAudioRenderClient, 0x000001dca23e9480 ) S_OK    0.0000106 Audio and Video / Core Audio / Windows Audio Session
>  1328339 3:28:11.854 PM  1 NPSWF64_29_0_0_113.dll  IAudioClient::GetService ( IAudioSessionControl, 0x000001dca23e9460 ) S_OK    0.0001174 Audio and Video / Core Audio / Windows Audio Session
>  1328395 3:28:11.856 PM  1 NPSWF64_29_0_0_113.dll  CreateEventW ( NULL, FALSE, FALSE, NULL ) 0x0000000000000a74    0.0000044 System Services / Synchronization / Event
>  1328398 3:28:11.856 PM  1 NPSWF64_29_0_0_113.dll  IAudioSessionControl::RegisterAudioSessionNotification ( 0x000001dca23e9440 ) S_OK    0.0000051 Audio and Video / Core Audio / Windows Audio Session
>  1328400 3:28:11.856 PM  1 NPSWF64_29_0_0_113.dll  IMMDeviceEnumerator::RegisterEndpointNotificationCallback ( 0x000001dca23e9438 )  S_OK    0.0000004 Audio and Video / Core Audio / Windows Multimedia Device
>  1328401 3:28:11.856 PM  1 NPSWF64_29_0_0_113.dll  IAudioRenderClient::GetBuffer ( 1104, 0x000001dca23e9550 )  S_OK    0.0000040 Audio and Video / Core Audio / Windows Audio Session
>  [...]
>  1328412 3:28:11.856 PM  1 NPSWF64_29_0_0_113.dll  IAudioRenderClient::ReleaseBuffer ( 1104, 0 ) S_OK    0.0000044 Audio and Video / Core Audio / Windows Audio Session
  [...]
>  1328419 3:28:11.856 PM  1 NPSWF64_29_0_0_113.dll  CreateThread ( NULL, 0, 0x00000000539e3b08, 0x000001dca23e9490, 0, 0x000001dca23e94f8 ) 0x0000000000000b44    0.0000365 System Services / Processes and Threads / Thread
>  1328425 3:28:11.856 PM  1 NPSWF64_29_0_0_113.dll  SetThreadPriority ( 0x0000000000000b44, THREAD_PRIORITY_ABOVE_NORMAL )  TRUE    0.0000015 System Services / Processes and Threads / Thread
>  1328427 3:28:11.856 PM  1 NPSWF64_29_0_0_113.dll  IAudioClient::Start (  )  S_OK    0.0023846 Audio and Video / Core Audio / Windows Audio Session

-----

The register call happens at sample #1328400.  That's the API that NPPVpluginRequiresAudioDeviceChanges remotes.  I note that _none_ of this happens with the sandbox on.  I'm currently wondering if the sandbox may be breaking something else that skips all of this (although we do still get sound playback).  But we will likely need Adobe to fix this on their end.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Crashes are mostly wildptrs

Comment 4

[Chris Peterson [:cpeterson]]

David, since Adobe hasn't released a Flash fix yet, should we back out bug 1426733 from Beta 60?

Comment 5

[David Parks [:handyman]]

Hey Chris, bug 1450773 actually does -- instead of backing out the change, it blocks it out of all non-nightly builds.  That change has been uplifted and is in 60.0b11, which, so far, is not showing any of these crashes.

Comment 6

[Chris Peterson [:cpeterson]]

Thanks.

Setting tracking flag firefox60=disabled (by bug 1450773) so relman knows this bug no longer affects 60.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

62 is on Nightly now. Updating the flags accordingly.

Comment 8

[xzhang]

We are investigating on adding support to NPPVpluginRequiresAudioDeviceChanges in Flash Player.

We got some questions that need more clarify from Mozilla:

(1) Is NPPVpluginRequiresAudioDeviceChanges only available on Windows 10?
(2) We should use the return value from |NPN_SetValue(...NPPVpluginRequiresAudioDeviceChanges...)| to check whether it is available, right?
(3) If NPPVpluginRequiresAudioDeviceChanges is not supported, we can assume that the legacy WIN32 API should still work, that is, Flash Player should keep its original behavior.
(4) We noticed that IAudioSessionControl interface is not allowed in Firefox nightly build, we are wondering whether there is an alternative way for that? The current implementation in Flash Player still relies on that interface to detect if a session is disconnected.

Comment 9

[Bob Owen (:bobowen)]

Just in case you miss comment 8 in your bugmail.

Comment 10

[David Parks [:handyman]]

(In reply to xzhang from comment #8)
> (1) Is NPPVpluginRequiresAudioDeviceChanges only available on Windows 10?
No, it should be available in all Windows builds.

> (2) We should use the return value from
> |NPN_SetValue(...NPPVpluginRequiresAudioDeviceChanges...)| to check whether
> it is available, right?
That is correct, although this API should only fail under extraordinary circumstances.  In other words, unless there are bugs, the call should succeed any time an IMMNotificationClient could be registered in a normal (unsandboxed) process.  If you are seeing this happen then something could be wrong with the implementation.

> (3) If NPPVpluginRequiresAudioDeviceChanges is not supported, we can assume
> that the legacy WIN32 API should still work, that is, Flash Player should
> keep its original behavior.
Actually, no.  In this case, you would need to ignore the feature.  But, again, this should really never happen.

> (4) We noticed that IAudioSessionControl interface is not allowed in Firefox
> nightly build, we are wondering whether there is an alternative way for
> that? The current implementation in Flash Player still relies on that
> interface to detect if a session is disconnected.
Are you sure about this?  We use these objects in the content processes (and I think we may still be setting them up in the plugin process) without sandbox issues.  If you can't get that to work then let me know.  We could put together a simple NPAPI point to work around it.

Comment 11

[xzhang]

To clarify (4), the failure happens when Flash Player tries to get IAudioSessionControl interface through |IAudioClient::GetService()|, it returns error E_ACCESSDENIED on Firefox nightly build. Firefox release build works fine. I guess this is related to sandboxing?

Comment 12

[David Parks [:handyman]]

Thanks xzhang.  I have an idea of what's happening now.  (And you are right -- this is about the sandbox.)  We could approach this a few different ways but all but the first one involve some effort.

Option #1: You could try to get an IAudioSessionControl from the IAudioSessionManager.  The IAudioSessionControl won't be tied to your IAudioClient but you likely don't have reason to care (the idea being that IAudioClient::GetService fails because it has to _create_ the AudioSessionControl object but the AudioSessionManager doesn't).  I actually think this should work because I think its what cubeb does in our content processes.  So, it looks like this road is permitted but, if its not, the rest of the solutions are more involved.  You can try this by pretty much just copying our code here [1].  (To be clear, I am saying that I _think_ you can GetDefaultAudioEndpoint from the IMMDeviceEnumerator for this -- the NPPVpluginRequiresAudioDeviceChanges was needed to give access to the notification system, not the device object itself.  From there, you get the IAudioSessionManager, then the IAudioSessionControl.  I might be wrong about that being permitted under the sandbox tho.)

Option #2: We could provide a special NPN_GetValue NPObject to give access to our IAudioSessionControl, created before the process' sandbox lockdown (ie when we still have permission).  You would mostly still use your current implementation.  But NPObjects are tedious.

Option #3: We could add OnSessionDisconnect notifications to NPPVpluginRequiresAudioDeviceChanges's callback.  So you get messages from device changes _and_ session disconnects.  This may be better than option #2.

Option #4: You could create IAudioClients and tie them to our session if you had our GUID (using the AudioSessionGUID parameter to IAudioClient::Initialize).  Since you aren't then creating the IAudioSessionControl, IAudioClient::GetService might not complain about permissions.  You would just need access to our grouping parameter GUID [2], which would be much easier to share than the IAudioSessionControl (since its just a string).  This is the biggest long-shot.

So give option #1 a try and let me know what you see.  If it fails, we can look for the right alternative.

-----

[1] https://searchfox.org/mozilla-central/rev/da499aac682d0bbda5829327b60a865cbc491611/widget/windows/AudioSession.cpp#254
[2] https://searchfox.org/mozilla-central/rev/da499aac682d0bbda5829327b60a865cbc491611/widget/windows/AudioSession.cpp#383

Comment 13

[xzhang]

Thanks David!

I tried Option #1. What's happening is that |IAudioSessionManager::GetAudioSessionControl()| returns E_ACCESSDENIED with nightly build. Here is the code snippet for the experiment:

[...]
HRESULT hr = mEndPointDevice->Activate(__uuidof(IAudioSessionManager),
                                       CLSCTX_ALL,
                                       NULL,
                                       reinterpret_cast<void **>(&mAudioSessionManager));
if (FAILED(hr))
    return false;

hr = mAudioSessionManager->GetAudioSessionControl(&GUID_NULL, 0, &mAudioSessionControl);
if (FAILED(hr))
    return false;
[...]

|mEndPointDevice| and |mAudioSessionManager| are obtained successfully, but it fails to get |mAudioSessionControl|


I agree that Option #3 is better than #2. In our current Flash Player implementation, it cares about 2 disconnec reasons:
DisconnectReasonDeviceRemoval
DisconnectReasonFormatChanged

so that it can handle switching stream properly.

Regarding Option #4, do you mean we can use the Mozilla predefined AudioSessionGUID in IAudioClient::Initialize? I can give it a try, if you send me the GUID.

Comment 14

[David Parks [:handyman]]

Attachment: WIP: npapi.h

Hey xzhang,

Too bad the IAudioSessionManager didn't work.  Lets try option #3 first.  I'm not sure if it will properly handle the DisconnectReasonFormatChanged event but I see it send DisconnectReasonDeviceRemoval when I disable all audio devices from the "Playback devices" Windows settings panel.

So I put together an attempt at this that uses the NPPVpluginRequiresAudioDeviceChanges mechanism that you are already working on.  When you request that, it starts sending NPNVaudioDeviceChangeDetails(4001) with NPAudioDeviceChangeDetails structs as payload.  With my candidate patch, it will also start sending NPNVaudioSessionDisconnected(4002) with NPAudioDisconnectReason as payload -- NPAudioDisconnectReason is an enum that just cases the Win32 AudioSessionDisconnectReason (see npapi.h).

You can play with the builds from here [1].  (Click the green `B` for the x64 build you want -- debug vs opt -- and select either the target.zip or the installer exe from the job details.)  Unfortunately, since I don't have a plugin that registers for the NPPVpluginRequiresAudioDeviceChanges call, I could only test that it prepares to send the event when the audio devices have been disabled.  Let me know if it works out.

Also note that the zip doesn't seem to include npapi.h, which I changed to add the new data types.  Pulling it out of the try server's diff would be annoying so I'm posting it here for you to build with.

-----

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ab28ae443020d9c589575b795ca1be5dfa1bf82

Comment 15

[David Parks [:handyman]]

Oops:
s/cases/casts/

Comment 16

[xzhang]

Thanks David,

I have tested the new NPNVaudioSessionDisconnected with Flash Player, and I am able to receive the notification when I disable the current device or unplug the device. However, I noticed some issues that may need your help:

(1) When Flash Player receives NPNVaudioSessionDisconnected notification, the payload value looks weird, it does not seem to be the valid reason. How should I convert |value| to reason code?

(NPAudioDisconnectReason)(reinterpret_cast<size_t>(value)) or (NPAudioDisconnectReason)(*value)

(2) If disable the same audio device the second time, Flash Player does not receive NPNVaudioSessionDisconnected notification.
My test steps are:
(a) Set a USB headset as default device
(b) Start Flash audio playback through the default audio device
(c) Disable the default audio device through control panel, audio playback will switch to another device
(d) Re-enable previous USB audio device, audio playback will switch back
(e) Disable the default audio device again, and Flash Player does not receive NPNVaudioSessionDisconnected notification.

Comment 17

[David Parks [:handyman]]

(1) This is my fault.  The void* parameter should actually be a NPAudioDisconnectReason& but its actually a NPAudioDisconnectReason*.  I'll get you a build with that fixed.  (The old one should have worked as *((NPAudioDisconnectReason*)value) though.)

(2) In your test, in step (c) you should be getting the old NPNVaudioDeviceChangeDetails message (not the new NPNVaudioSessionDisconnected message) as you go from one audio device to the other.  The same message should then be sent in step (e) -- again, it would not send the NPNVaudioSessionDisconnected message but it would send the NPNVaudioDeviceChangeDetails message.  Is that not whats happening?

Comment 18

[David Parks [:handyman]]

On second thought...

For problem (1), I've looked over the other setvalue fields and it looks like they are also passed as pointers to basic data.  So, instead of making a new build, can you just use the void* value as *(static_cast<NPAudioDisconnectReason*>value)?  That definitely looks like it "should" be valid.

Comment 19

[xzhang]

Thanks David, that conversion works fine for me.

Comment 20

[xzhang]

Regarding problem (2), what's happening in step (c) is that I receive NPNVaudioDeviceChangeDetails message first, then I receive the new NPNVaudioSessionDisconnected message. In step (e), I can receive NPNVaudioDeviceChangeDetails message so the audio device will switch back to the previous default device, but I don't receive the new NPNVaudioSessionDisconnected message anymore.

The reason I am focusing on this issue is that, Flash Player has a feature that allows content to select audio device through actionscript API. If the current selected device is not the system default device, and it is removed, we need to know this event so that we can handle the switch.

Comment 21

[David Parks [:handyman]]

Attachment: npapi.h

Hey xzhang.  Sorry about the delay -- I didn't have a good sense of what this meant before or how to fix it.

I think I understand what you are describing here now -- its an edge case involving Flash using a device other than the default.  The most direct way of supporting this seems like it would be a lot of work for something so exceptional, but I have an idea that might work.  It should be easy to try anyway.

The hard way of doing this would require us to mimic the behavior you were doing so that we can get an IAudioSessionControl in a sandbox-permitting process.  In addition to the work of creating/maintaining the AudioClient/AudioSession objects, we would have to get a note from the plugin, telling us of your audio device choice.  

A hack that might work around this would be for us to use the IMMDeviceEnumerator that we already use for the NPNVaudioSessionDisconnected message... only now we _also_ send you  a new message, NPNVaudioDeviceRemoved, when the IMMDeviceEnumerator sends the IMMNotificationClient::OnDeviceRemoved message [1].  You could then test the device ID in those messages against your chosen device to see if your device was lost.

I'm not certain that this message will get sent in your case but hopefully it will.  I've hacked up a build that does this -- it also includes the NPNVaudioSessionDisconnected work from above.

I've launched a build [2] (which will hopefully work) and I'm attaching the new npapi.h with the new NPNVaudioDeviceRemoved value.  The payload will be a wchar_t* deviceID, which is what the Windows device APIs return.  Of course, you will need to do a string-compare to test for a device match.  Let me know if this works!

PS: I haven't heard anything about when/if the work that this bug was originally about, the NPNVaudioDeviceChangeDetails stuff, will be/was released in Flash Player.  Do you have any insight into this?  We would like to activate the hardened sandbox properties as soon as that happens.  (This was all unrelated to the issues we are still discussing.)

---

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd371422(v=vs.85).aspx
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfd7ecb3621fe9fb0b4d4a5980cbe7ea424fd202

Comment 22

[David Parks [:handyman]]

Attachment: npapi.h

Since that didn't work -- new build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8df2e790504603378ebeb732eb9e2095d8f5aac

Comment 23

[xzhang]

I believe the hack should work. But could you pass NPNVaudioDeviceStateChanged according to IMMNotificationClient::OnDeviceStateChanged, instead of NPNVaudioDeviceRemoved? Flash Player actually cares about the change message.

Regarding the original bug, I think our discussion is relevant. The bug is about enabling the hardened sandbox, and there are 3 parts in Flash Player that are using the device change notification: microphone, audio output and audio device selection. I cannot ignore any of them, otherwise some functionality will be broken.

I think the NPNVaudioDeviceStateChanged message should help us solve the remaining problem, and we should be able to switch to this new mechanism smoothly.

Comment 24

[David Parks [:handyman]]

Attachment: npapi.h

That all makes sense.

Here's a new build with npapi.h -- I've removed the NPNVaudioDeviceRemoved value (from IMMNotificationClient::OnDeviceRemoved) and added NPNVaudioDeviceStateChanged (from IMMNotificationClient::OnDeviceStateChanged).  The payload is a `NPAudioDeviceStateChanged*`, NPAudioDeviceStateChanged is now defined in the new npapi.h -- its newState value is the dwNewState parameter to IMMNotificationClient::OnDeviceStateChanged.

Let me know if this works.  I left the NPNVaudioSessionDisconnected message in case you still need it but I dont think you do -- if you don't, let me know and I'll take it out.

Build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a9f157184de00aa021c6cd092841e6f5c8dd9f9

Comment 25

[xzhang]

The new NPAudioDeviceStateChanged approach works for me. I am trying to finish the implementation in Flash Player.

One quick question to confirm. This is only related to 64-bit Firefox, and the hardened sandbox is not for 32-bit Firefox, right?

Comment 26

[David Parks [:handyman]]

Awesome.  And you are correct -- while the NPPVpluginRequiresAudioDeviceChanges request should result in the proper messages in 32-bit, our sandbox doesn't apply there so there should be no need to register NPPVpluginRequiresAudioDeviceChanges there at all.

Sounds like you don't need the NPNVaudioSessionDisconnected so I'll take that out.  I'll get this into nightly in the next few business days so you can work with official builds.  Thanks for being so patient with this.

Comment 27

[xzhang]

NPNVaudioSessionDisconnected is tricky, since Flash Player does not know which device is this event associated with.

I think my plan is like this:
(1) We just ignore NPNVaudioSessionDisconnected for now, so you can take it out.
(2) If there is any strong requirement shows that SessionDisconnected needs to be supported in the future, we can consider a proper fix. I would imagine a new NPObject would be needed.

Comment 28

[David Parks [:handyman]]

OK, so, even if you need something like NPNVaudioSessionDisconnected, the current version wouldn't be sufficient.  Sounds like a good enough reason to dump it for now.

Comment 29

[xzhang]

The only impact is that if user changes the format of output device from control panel, Flash Player will not be able to handle that, since Flash Player won't receive IAudioSessionEvents::DisconnectReasonFormatChanged anymore. 

Do we plan to improve this in the future?

Comment 30

[xzhang]

The modification in Flash Player is under code review.

I think the safe steps to switch to this new functionality should be like this:
(1) Mozilla enables NPNVaudioDeviceStateChanged in Firefox stable channel
(2) Flash Player turns on its full support of NPPVpluginRequiresAudioDeviceChanges
(3) When everything works well, Mozilla turns on the hardened sandbox in 64-bit firefox.

Comment 31

[David Parks [:handyman]]

(In reply to xzhang from comment #29)
> The only impact is that if user changes the format of output device from
> control panel, Flash Player will not be able to handle that, since Flash
> Player won't receive IAudioSessionEvents::DisconnectReasonFormatChanged
> anymore. 

I see.  Since the NPNVaudioSessionDisconnected code only reported if the format was changed for the default device (since that's the only one Firefox has an AudioSession for), you still have the case where someone changes the non-default audio device that you are using from the control panel.  For this, we probably need to register an AudioSession for you to get the events.  Whatever we do, we'll need another round of APIs to get it done.  We can hold off on it for now.

Your plan sounds good to me.  I'm finishing up my patch for review now.

Comment 32

[David Parks [:handyman]]

A minor FYI --
Removing the NPNVaudioSessionDisconnected message is changing the NPNVaudioDeviceStateChanged enum value so you'll need to rebuild with the new npapi.h when using the official builds.

Comment 33

[David Parks [:handyman]]

Attachment: Send NPNVaudioDeviceStateChanged to plugins when a Windows audio device changes state

The patch basically just forwards the OnDeviceStateChanged message from the Windows IMMNotificationClient object in the chrome process to the Flash plugin.  It largely mirrors the way we forward the OnDefaultDeviceChanged message.

Comment 34

[David Parks [:handyman]]

Attachment: Send NPNVaudioDeviceStateChanged to plugins when a Windows audio device changes state

Ugh.  Fix some #ifdefs for non-Windows builds.
See patch notes in comment 33.

Build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=990de6e8bae27c0cc68e4e9611244d5bcffdf104

Comment 35

[David Parks [:handyman]]

Comment on attachment 8988015 [details] [diff] [review]
Send NPNVaudioDeviceStateChanged to plugins when a Windows audio device changes state

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It would be very hard to even determine that this crash existed from these patches -- the real "fix" is in Adobe's code.  This is part of that fix but, without _both_ our and Adobe's changes, nothing happens.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No -- same reason.

Which older supported branches are affected by this flaw?
The crash comes from audio registration in the plugin so any version of Firefox using affected plugin versions can see the crash.

If not all supported branches, which bug introduced the flaw?
N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This would probably be a bad idea as it would require heavy testing with Flash to make sure that changes to it or the browser were compatible with this change.  This patch builds off of IMMNotificationClient work that went into Firefox in Bug 1251202 (firefox 52).  

How likely is this patch to cause regressions; how much testing does it need?
It is unlikely to cause anything to happen until Adobe's changes land.  At that point, since it largely mirrors other stable code, this is a mild risk.

Comment 36

[Chris Peterson [:cpeterson]]

(In reply to David Parks (dparks) [:handyman] from comment #32)
> Removing the NPNVaudioSessionDisconnected message is changing the
> NPNVaudioDeviceStateChanged enum value so you'll need to rebuild with the
> new npapi.h when using the official builds.

David, will changing the enum values cause binary compatibility issues for older Flash versions in the wild built against the old npapi.h? Can we just skip the old NPNVaudioDeviceStateChanged enum value for backwards compatibility?

Comment 37

[David Parks [:handyman]]

No, this was just a thing internal to this bug -- I had added NPNVaudioSessionDisconnected in an early version of the patch, later added NPNVaudioDeviceStateChanged, and finally removed NPNVaudioSessionDisconnected.  But none of that was public.  The only builds of Flash that would care at all would be builds their engineers were using for development.

Comment 38

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8988015 [details] [diff] [review]
Send NPNVaudioDeviceStateChanged to plugins when a Windows audio device changes state

sec-approval+ for trunk.
Do we want this on beta or should it ride the trains from trunk?

Comment 39

[David Parks [:handyman]]

Comment on attachment 8988015 [details] [diff] [review]
Send NPNVaudioDeviceStateChanged to plugins when a Windows audio device changes state

Lets try for uplift.

Approval Request Comment
[Feature/Bug causing the regression]:
N/A.  An issue with Flash causes this crash.  They are internally changing their code.  This patch to Firefox is needed for their fix.

[User impact if declined]:
This patch will be needed for Flash audio to work when Adobe releases its changes.  Beyond that, there is no impact to this patch.  We don't have a set date for Adobe's release.

[Is this code covered by automated tests?]:
No.  It adds NPAPI functionality, very little of which is actually tested.

[Has the fix been verified in Nightly?]:
In so far as that is possible, yes.  Adobe engineers have confirmed it works for them.

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
Mildly, when Adobe's changes land.

[Why is the change risky/not risky?]:
Because Flash does not currently register for the message.  When Flash does begin to register for it, the code will kick in.  It is similar to the handling of a related message so the code should be safe when that happens.  

[String changes made/needed]:
None

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9a332fe140be7a4935e79535e0fd61aa2b9625

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9e9a332fe140

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8988015 [details] [diff] [review]
Send NPNVaudioDeviceStateChanged to plugins when a Windows audio device changes state

Fix for issue with upcoming Flash release. Let's uplift for 62 beta 7.

Comment 43

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5f79cf2a5110

Comment 44

[Marcia Knous [:marcia]]

Not sure if it is related, but there is a set of crashes starting with 20180628220051 that have a similar signature, https://bit.ly/2m417A8. Several of the crashes occurred after the fix landed in Comment 41. All of them have EXCEPTION_ACCESS_VIOLATION_EXEC as the crash reason. I can file a new bug if necessary.

Comment 45

[xzhang]

Hi David, is the patch available in current nightly build?

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Yes, as well as 62.0b7.