Bob Owen (:bobowen) Reporter
	Description • 10 years ago

Mochitest dom/media/tests/mochitest/test_dataChannel_basicAudio.html fails when running using the Windows process sandbox.

The main problem appears to be attempted access to the registry.
I'll attach two stack traces from procmon of places in the code where this is happening.

	Bob Owen (:bobowen) Reporter
	Comment 2 • 10 years ago

Hi gcp,
I've been told you would be good person to have a look at this issue.

I forgot to mention the registry keys are things like:
HKCU\Software\Microsoft\Multimedia\Audio Compression Manager\

So, I'm guessing a couple of the pertinent questions include:
* Can we change the code so that the registry access isn't needed?
* Can we change the code to do this via the broker process?

Thanks.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 3 • 10 years ago

18	kernel32.dll	RegQueryInfoKeyW + 0x3e1
19	kernel32.dll	RegCreateKeyExW + 0x11d
20	kernel32.dll	RegCreateKeyExW + 0x2d
21	msacm32.dll	acmFormatSuggest + 0xad7
22	msacm32.dll	acmFormatSuggest + 0x14c8
23	msacm32.dll	acmDriverEnum + 0x67
...
27	winmm.dll	waveOutOpen + 0x140

The registry queries are coming from the normal functions to open the Windows audio system. I'm a bit surprised that just opening a WaveOut causes the ACM to get initialized, which are format converters and decoders (and obviously have config data stored in the registry). The documentation says: http://msdn.microsoft.com/en-us/library/windows/desktop/dd743866%28v=vs.85%29.aspx

WAVE_FORMAT_DIRECT	If this flag is specified, the ACM driver does not perform conversions on the audio data.

I think we want this - I can't think of a case where we want an extra format decoder in-between as we should be sending basic PCM.

webrtc::AudioMixerManager::OpenSpeaker + 0x114, c:\dev\mozilla-central\media\webrtc\trunk\webrtc\modules\audio_device\win\audio_mixer_manager_win.cc(610)
winmm_stream_init + 0x400, c:\dev\mozilla-central\media\libcubeb\src\cubeb_winmm.c(416)

padenot, do you think this makes sense for cubeb too? I'm not sure this is the only thing that will end up using the registry but if it is, this is a simple fix.

	Paul Adenot (:padenot)
	Comment 4 • 10 years ago

It's certainly worth trying, yes.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 5 • 10 years ago

Still seems to be failing. Trying to analyze here, but my machine tries to use WASAPI for cubeb and fails with an E_HANDLE in Initialize(). I guess I'll ProcMon that and see what it's doing.

	Paul Adenot (:padenot)
	Comment 6 • 10 years ago

gcp, here is a patch that force use of winmm, even on windows vista, seven or eight.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 7 • 10 years ago

I'm seeing things fail in:
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/audio_device/win/audio_mixer_manager_win.cc#835
(even with WAVE_FORMAT_DIRECT).

I'm a bit curious how you get stack traces of exactly which calls are failing due to the sandboxing? Procmon gives me a ton of stuff, but most of the stacks don't actually seem to be ending up in plugin-container.exe, even though it does look like it's Windows subsystems probing the registry to find the default audio input device etc.

I'm not sure how the windows sandboxing stuff works when we do a normal windows API call and the code backing that (which might be a third party driver) goes off and tries to do things that our sandbox doesn't allow.

	Bob Owen (:bobowen) Reporter
	Comment 8 • 10 years ago

(In reply to Gian-Carlo Pascutto [:gcp] from comment #7)

> I'm a bit curious how you get stack traces of exactly which calls are
> failing due to the sandboxing? Procmon gives me a ton of stuff, but most of
> the stacks don't actually seem to be ending up in plugin-container.exe, even
> though it does look like it's Windows subsystems probing the registry to
> find the default audio input device etc.

Hmm, not sure, do you have ac_add_options --disable-optimize?

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 9 • 10 years ago

Yes, I do.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧
	Comment 10 • 10 years ago

Move process sandboxing bugs to the new Bugzilla component.

(Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)

	Bob Owen (:bobowen) Reporter
	Comment 11 • 9 years ago

Once the patches for another bug I'm working on land (still need reviewing), we will be able to start the content process with an initial integrity level of low.

I don't know why, but when I've tried this, it seems that the audio suddenly starts working again and the tests all pass (at least all the ones that pass with e10s).

I know that other things like UIPI depend on the integrity level early on (see [1]), so maybe something similar is happening for audio and the subsequent mismatch between the integrity levels is causing the current problem.

I think there are other reasons (e.g. for other platforms) that may mean we want to do bug 1104619 anyway and we may well need it on Windows, when we eventually want to drop to untrusted integrity, but that won't be our first release.

So, assuming I can land those patches, it looks like this may no longer block us for getting to low integrity.

[1] (near the bottom) https://msdn.microsoft.com/en-us/library/bb625963.aspx

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 12 • 9 years ago

Bob, is this still relevant?

	Bob Owen (:bobowen) Reporter
	Comment 13 • 9 years ago

Should be fixed by that bug I mentioned, I'll leave the needinfo to remind me to test to make sure.

	Bob Owen (:bobowen) Reporter
	Comment 14 • 9 years ago

Yes, this is passing on all Windows platforms now, even with stronger sandbox settings that the current default.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 8 years ago

Bob, we're skipping the dom/media mochitests when strict sandboxing is enabled (with a reference to this bug). Is that something we should still be doing?

	Bob Owen (:bobowen) Reporter
	Comment 17 • 8 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Bob, we're skipping the dom/media mochitests when strict sandboxing is
> enabled (with a reference to this bug). Is that something we should still be
> doing?

No sorry, I should have removed that.

What was the "strict(er)" settings at the time is now the default anyway I think.
Getting most tests running with a stronger sandbox was blocked on getting these tests running under e10s at all.

Looks like I might be able to revisit that now at some point.