New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 737090 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug

Blocked on:
issue 737763



Sign in to add a comment

ComInitCheckHook breaks WebRTC windows bots

Project Member Reported by guidou@chromium.org, Jun 27 2017

Issue description

See
https://build.chromium.org/p/chromium.webrtc/builders/Win7%20Tester
https://build.chromium.org/p/chromium.webrtc/builders/Win10%20Tester
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win7%20Tester
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win8%20Tester
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win10%20Tester

Lots of tests fail with errors like this:
AudioInputDeviceManagerTest.OpenDeviceTwice (run #1):
[ RUN      ] AudioInputDeviceManagerTest.OpenDeviceTwice
[4692:4248:0626/145510.364:1882620:FATAL:com_init_check_hook.cc(144)] Check failed: false. Unrecognized hotpatch function format.
Backtrace:
	base::debug::StackTrace::StackTrace [0x02F73B87+55]
	base::debug::StackTrace::StackTrace [0x02F8894A+10]
	base::win::ComInitCheckHook::~ComInitCheckHook [0x02FA9249+777]
	base::win::ComInitCheckHook::ComInitCheckHook [0x02FA8F1A+42]
	base::TaskScheduler::Create [0x02F2E813+67]
	base::test::ScopedAsyncTaskScheduler::ScopedAsyncTaskScheduler [0x028FA9E4+145]
	??$MakeUnique@VScopedAsyncTaskScheduler@test@base@@$$V@base@@YA?AV?$unique_ptr@VScopedAsyncTaskScheduler@test@base@@U?$default_delete@VScopedAsyncTaskScheduler@test@base@@@std@@@std@@XZ [0x028BF435+22]
	content::TestBrowserThreadBundle::CreateThreads [0x028BF8D7+144]
	content::TestBrowserThreadBundle::Init [0x028BFCF2+455]
	content::TestBrowserThreadBundle::TestBrowserThreadBundle [0x028BF4F6+53]
	content::TestBrowserThreadBundle::TestBrowserThreadBundle [0x028BF507+10]
	testing::TestInfo::Run [0x028BDC78+101]
	testing::TestCase::Run [0x028BDBBC+133]
	testing::internal::UnitTestImpl::RunAllTests [0x028BDF41+433]
	testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool> [0x028B64B1+32]
	testing::UnitTest::Run [0x028BDD65+133]
	base::TestSuite::Run [0x028FB3C6+95]
	base::LaunchUnitTests [0x028F9655+784]
	base::LaunchUnitTests [0x028F939A+85]
	main [0x01AF6368+103]
	__scrt_common_main_seh [0x03FAB7EB+249] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253)
	BaseThreadInitThunk [0x76C4338A+18]
	RtlInitializeExceptionChain [0x77C49902+99]
	RtlInitializeExceptionChain [0x77C498D5+54]



 

Comment 1 by guidou@chromium.org, Jun 27 2017

Cc: robliao@chromium.org kjellander@chromium.org gab@chromium.org
Components: Infra>Client>WebRTC
robliao@: I reverted your CL https://chromium-review.googlesource.com/c/546157/ because it is the main suspect of breaking all Windows WebRTC-Chromium bots.

Please coordinate with kjellander@ before relanding it.

kjellander@: any idea why this change breaks all WebRTC Windows bots, but not the main Windows Chromium bots?

Comment 2 by guidou@chromium.org, Jun 27 2017

Cc: guidou@chromium.org
Re #1: Our bots in these waterfalls are bare-metal machines with real webcams and audio devices. Everything in the CQ and main waterfall is running on VMs (except GPU and Perf bots, but they don't run these tests).

Comment 4 by guidou@chromium.org, Jun 27 2017

The revert worked and bots are coming back to green:

https://build.chromium.org/p/chromium.webrtc/builders/Win10%20Tester/builds/17435

Comment 5 by guidou@chromium.org, Jun 27 2017

Labels: -Pri-1 Pri-2
In the meantime, is there a way you could send me your combase.dll and/or ole32.dll from the bots? That will definitely aid in investigating the issue.

If not, seems like the only way to run these is to reland the change and watch what happens.
Owner: robliao@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ecd398a13103cb20f29dbb37af7f93e626da96e

commit 1ecd398a13103cb20f29dbb37af7f93e626da96e
Author: Robert Liao <robliao@chromium.org>
Date: Tue Jun 27 20:05:40 2017

Add Diagnostic Information For Unexpected Hotpatch Formats

Some bots have unexpected formats. Listing out those bytes will aid
diagnosing the issue.

BUG= 708303 , 737090 

Change-Id: Ie5f7bd20dc16329c44fbbc5e22b6a266111cb9cc
Reviewed-on: https://chromium-review.googlesource.com/550259
Reviewed-by: Scott Graham <scottmg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482721}
[modify] https://crrev.com/1ecd398a13103cb20f29dbb37af7f93e626da96e/base/win/com_init_check_hook.cc

FYI, I just relanded this change to get more info from the bots. Feel free to revert
https://chromium-review.googlesource.com/c/550859/
If I don't get to it first.

Comment 10 Deleted

The webrtc bots failed as expected and generated the more interesting error message:

Check failed: false. Unrecognized hotpatch function format: 90 90 90 90 90 e9 10

e9 is a jmp relative 32-bit. Do the WebRTC bots run special COM hooks?
Cc: pschmidt@chromium.org
Labels: OS-Windows
Thanks for reverting after gathering more data. The main difference is that they're bare-metal machines - all regular CQ trybots and main waterfall bots are VMS AFAIK.
The only exceptions are the perf and GPU bots which are also bare-metal.

I don't know what may be different in terms of the Windows image they run, maybe there are differences? Other than that we haven't done anything special with these machines except installing sound card drivers and configured the sound card for loopback.

+pschmidt from the Chrome infra labs team for more input regarding possible differences.
Can you give me an example of a bot where this is passing?
pschmidt: Pretty much everything on chromium.master. 
Here's an example of a successful run: https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/61129

Only webrtc thus far has failed.

I've yet to see a form of CoCreateInstance immediately start with a jmp, suggesting that the WebRTC bots and/or tests are running with hooks installed. 

Running the test under a debugger will be able to quickly verify that.
Cc: scottmg@chromium.org
scottmg suggests this might be a hotpatch done by Windows for an update and we need to reboot the machines.


Blockedon: 737763
Project Member

Comment 17 by bugdroid1@chromium.org, Jun 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/788101ef734f894f076e941c40c2693bd64dd548

commit 788101ef734f894f076e941c40c2693bd64dd548
Author: Robert Liao <robliao@chromium.org>
Date: Wed Jun 28 23:55:16 2017

Skip Hooking if External Hotpatching Already Took Place

The webrtc tests appear to have a patched CoCreateInstance already.
The cause is currently unknown. The workaround this, the COM hook will
skip that case.

BUG= 737090 

Change-Id: I2e3eef659e60951d9a7088b533c50cbc65631cb4
Reviewed-on: https://chromium-review.googlesource.com/552770
Commit-Queue: Robert Liao <robliao@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483204}
[modify] https://crrev.com/788101ef734f894f076e941c40c2693bd64dd548/base/win/com_init_check_hook.cc
[modify] https://crrev.com/788101ef734f894f076e941c40c2693bd64dd548/base/win/com_init_check_hook_unittest.cc
[modify] https://crrev.com/788101ef734f894f076e941c40c2693bd64dd548/base/win/patch_util.h

Labels: -Pri-2 Pri-1
Owner: pschm...@google.com
Status: Assigned (was: Started)
Reassigning to pschmidt for the bot investigation.

We would like to make this workaround crash so that they do not silently fail for bots that should have this running.
Can you explain to me what you want me to do?
We want to see what's intercepting the hook on these bots. The suspicion is either drivers or malware. 
Owner: robliao@chromium.org
I have access to the WebRTC bot to investigate.
I decided to fire up notepad.exe, a relatively unassuming application just to see if ole32!CoCreateInstance got hooked. Turns out... it did!

The machine doesn't have access to symbols, so they're a little wonky.

Before:
0:000> u ole32!CoCreateInstance
ole32!CoCreateInstance:
76749cbb 8bff            mov     edi,edi
76749cbd 55              push    ebp
76749cbe 8bec            mov     ebp,esp
76749cc0 83ec0c          sub     esp,0Ch
76749cc3 56              push    esi
76749cc4 8b7518          mov     esi,dword ptr [ebp+18h]
76749cc7 85f6            test    esi,esi
76749cc9 0f841e8a0400    je      ole32!CoCreateInstance+0x10 (767926ed)

And then we do a simple 'ba w1 76749cbb' after we start the main function.

The breakpoint hits! Someone else is indeed hooking COM. Seems like some sort audio driver or something.

After
0:000> u ole32!CoCreateInstance
ole32!CoCreateInstance:
76749cbb e9ff558bec      jmp     62fff2bf
76749cc0 83ec0c          sub     esp,0Ch
76749cc3 56              push    esi
76749cc4 8b7518          mov     esi,dword ptr [ebp+18h]
76749cc7 85f6            test    esi,esi
76749cc9 0f841e8a0400    je      ole32!CoCreateInstance+0x10 (767926ed)
76749ccf 8b4514          mov     eax,dword ptr [ebp+14h]
76749cd2 8365f800        and     dword ptr [ebp-8],0

0:000> kn200
 # ChildEBP RetAddr
WARNING: Stack unwind information not available. Following frames may be wrong.
00 0029f090 0029f0c4 HsSrv!removeCOMHook+0x96f6
01 0029f094 778708f6 0x29f0c4
02 0029f0c4 77871178 ntdll!RtlpValidateHeap+0x20
03 0029f10c 7782aa86 ntdll!RtlDebugAllocateHeap+0x31f
04 0029f1e8 1000c4d8 ntdll!RtlpAllocateHeap+0xc4
05 0029f214 1000c347 HsSrv!removeCOMHook+0x9e38
06 0029f2a8 1000bba5 HsSrv!removeCOMHook+0x9ca7
07 0029f2ac 1000c125 HsSrv!removeCOMHook+0x9505
08 0029f2d0 1000c2e0 HsSrv!removeCOMHook+0x9a85
09 0029f4f4 100107b0 HsSrv!removeCOMHook+0x9c40
0a 0029f51c 777ddc81 HsSrv!removeCOMHook+0xe110
0b 0029f610 777dc901 ntdll!LdrpRunInitializeRoutines+0x26f
0c 0029f784 7781d39f ntdll!LdrpLoadDll+0x472
0d 0029f7c0 771b2e0f ntdll!LdrLoadDll+0xc7
0e 0029f808 754baad3 KERNELBASE!LoadLibraryExW+0x233
0f 0029f948 777b011a USER32!__ClientLoadLibrary+0x66
10 0029f988 754ba8f8 ntdll!KiUserCallbackDispatcher+0x2e
11 0029fc34 754baa4c USER32!VerNtUserCreateWindowEx+0x1a9
12 0029fce8 754b8a6c USER32!_CreateWindowEx+0x210
13 0029fd24 76746407 USER32!CreateWindowExW+0x33
14 0029fd5c 767464c4 ole32!InitMainThreadWnd+0x3e
15 0029fd74 76740b39 ole32!wCoInitializeEx+0xef
16 0029fd94 00b4143e ole32!CoInitializeEx+0x29d
17 0029fdc8 00b4190e notepad!WinMain+0x35
18 0029fe58 7707338a notepad!_initterm_e+0x1a1
19 0029fe64 777d9902 kernel32!BaseThreadInitThunk+0x12
1a 0029fea4 777d98d5 ntdll!__RtlUserThreadStart+0x70
1b 0029febc 00000000 ntdll!_RtlUserThreadStart+0x1b

0:000> lmvm HsSrv
start    end        module name
10000000 10037000   HsSrv    C (export symbols)       C:\Windows\SysWOW64\HsSrv.dll
    Loaded symbol image file: C:\Windows\SysWOW64\HsSrv.dll
    Image path: C:\Windows\SysWOW64\HsSrv.dll
    Image name: HsSrv.dll
    Timestamp:        Thu Sep 16 22:52:57 2010 (4C930239)
    CheckSum:         00000000
    ImageSize:        00037000
    File version:     1.0.10.917
    Product version:  1.0.0.1
    File flags:       0 (Mask 17)
    File OS:          4 Unknown Win32
    File type:        2.0 Dll
    File date:        00000000.00000000
    Translations:     0409.04b0
    CompanyName:      C-Media Electronics Inc.
    ProductName:      HsSrv Dynamic Link Library
    InternalName:     CMGxSrv
    OriginalFilename: HsSrv.dll
    ProductVersion:   1, 0, 0, 1
    FileVersion:      1, 0, 10, 917
    FileDescription:  HsSrv Dynamic Link Library
    LegalCopyright:   Copyright (C) 2008 C-Media Electronics Inc.


What's the latest here? Is this still a valid issue?
This is still valid. There is code on the WebRTC bots that hooks CoCreateInstance. It would be ideal if there was an alternate way of performing the same action without the hook.
robliao@ Thanks fir the update! Are you the right owner for this? Also, given the long period of no action, is this really a Pri-1?
Owner: ----
Status: Available (was: Assigned)
anatolid: I think given that I've finished the investigation, I'm not the right owner for this. This should go to the WebRTC Bot Owners.

The P1 is due to the fact we had to disable COM correctness checks in the code to work around this.
Cc: grunell@chromium.org henrika@chromium.org
I don't know enough about Windows or COM to comment on why this happened to our bots. The only thing that makes these bots different from regular Chromium CQ bots is that they're bare-metal machines (not VMs) and that they run a tests that relies on real audio and video devices.

Since the crashing test in the original description is AudioInputDeviceManagerTest.OpenDeviceTwice I'm adding grunell@ and henrika@ to take a look.

Cc: tommi@chromium.org
Not much to add at this stage. Think you need a real COM expert to figure out why the proposed change breaks on WebRTC bots.

I recall from https://bugs.chromium.org/p/chromium/issues/detail?id=416594 that there used to be Chrome FYI bots running tests on real Windows hardware as well. Can't access http://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Audio now but perhaps the URL can be updated and comparisons could be made.

Adding tommi@ for COM parts. 

Comment 29 by tommi@chromium.org, Oct 13 2017

Owner: robliao@chromium.org
This doesn't really seem to be an issue with COM itself but rather strong assumptions around the actual binary code inside of ole32.dll for the purposes of patching.
The code looks relatively new, so I'm guessing that we're still shaking out some issues and that on the bot where we're seeing these failures, we have a version of ole32.dll that has different binary code than we've previously tested with ComInitCheckHook.

Assigning to author of the check for now based on:
https://chromium-review.googlesource.com/c/chromium/src/+/550259
Owner: kjellander@chromium.org
The root cause is in #22. The bare metal machines load a driver which patches COM in all processes. If we can find a different driver that doesn't exhibit this (arguably bad) behavior, then we can start to close this bug.

Comment 31 by tommi@chromium.org, Oct 13 2017

Ah, sorry I missed that.

In that case, I'd argue for changing the NOTREACHED to be a DLOG(ERROR).
My reasoning is that we do hit this DCHECK in production and the code does actually expect and handle this case. DCHECKs are more to catch things like programmer errors or to put where we make assumptions that we do not have to check for.

Comment 32 by tommi@chromium.org, Oct 13 2017

(oh and I totally agree that this behavior isn't nice. However, it's not that uncommon in the Windows world)
The ComInitCheckHook is not run on production builds. It's only run in debug and 32-bit builds. The impact here is if something did change on Microsoft's side, we wouldn't be able to tell the difference between that and a hook. Since we control our environment, it would be nice to be clean of all hooks.

Comment 34 by tommi@chromium.org, Oct 13 2017

Would there be a way to not have COM_INIT_CHECK_HOOK_ENABLED defined in the webrtc builds?
Is there a marker we could use to identify WebRTC bots?
Changing the driver would work too.
Owner: phoglund@chromium.org
There's nothing special with our bots, the only thing that differs from most other machines in Chrome infra is that they're bare-metal (like the perf and GPU machines) instead of VMs.

tommi: would changing the driver be OK or do we potentially hide a user-problem doing so? I'm not the right person to own this going forward, as this is my last week at Google.

Comment 37 by tommi@chromium.org, Oct 16 2017

Re #35: Changing the driver is not an option.  It's a 3rd party component.  Perhaps that's where the misunderstanding lies.  The purpose of the bot is to test Chrome video features with this 3rd party software.  So, the bot is "clean" in the sense that it only has software installed that is needed in order to run the tests.

However, what ComInitCheckHook is doing, is not necessary, so I'd either like us to disable it, or simply change the NOTREACHED() to logging an error instead.
While the bot is clean, it's not doing clean things. We will routinely reject crashes from the wild if we found it was patching COM. Avoiding the use of HsSrv would be ideal.

ComInitCheckHook is ensuring the correctness of COM within the Chromium codebase (and has found several places where COM was being used in an uninitialized context).

I'm happy to discuss this in person if that would make things easier.

Comment 39 by tommi@chromium.org, Oct 16 2017

Sure - I'm in Kirkland today, so I think we're in the same timezone  :) I expect to have time after lunch today. Can you book something at a time that works for you?
Re: #39. Done!
Cc: phoglund@chromium.org
Owner: tommi@chromium.org
Summary from the meeting:

tommi:
  * Investigate if there is an alternative to HsSrv so we don't hook all of COM
  * If no alternative exists, investigate using special build flag to turn off the
    ComInitCheckHook

robliao:
  * Once the above is determined, make appropriate ComInitCheckHook changes.

Assigning to tommi@ for now.

Comment 42 by gab@chromium.org, Oct 16 2017

Plan SGTM.

Le lun. 16 oct. 2017 16 h 43, robliao via monorail <
monorail+v2.1842724928@chromium.org> a écrit :

Comment 43 by tommi@chromium.org, Oct 16 2017

Thanks Robert.

Patrik can you look into this?:
  * See if we can use some other software on the bot that accomplishes the same thing as the current one does, but does not do this type of hooking.


If that's a dead end, then here's what we'll do:

  * Add support for a special build flag that turns off "COM_INIT_CHECK_HOOK_ENABLED" on only the WebRTC Windows Bots? (presumably WebRTC's "Win Builder").
https://cs.chromium.org/chromium/src/base/win/com_init_check_hook.h?rcl=14c48caf1d643328b20af02368d56a9814b2da95&l=23

I, or Guido can prepare such a change and send the review to Robert. Updating the bot itself to set the flag, could be done separately.

Comment 44 by tommi@chromium.org, Oct 16 2017

Owner: phoglund@chromium.org
Assigning to Patrik to investigate the first option.
Owner: pschmidt@chromium.org
Uh, and "some other software" would be replacing HsSrc, which is this according to Google: "HsSrv Dynamic Link Library and developed by C-Media Electronics Inc."

That would be something that came with the driver to the USB sound cards we use on the bare-metal bots, right?

Maybe we can just uninstall those drivers and fall back to default Windows USB sound card drivers. pschmidt, I believe you've installed drivers for us. Any idea if our tests work without those drivers? Can you try it on one bot, say build98-b1 (https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win7%20Tester)? Assign the bug back to me after you've tried. Alternatively, I can try it, but in that case provide me instructions on how to re-install the driver if necessary.

IMO the best option is to uninstall the drivers and hope our tests work anyway. That'll be less work the next time we set up bare metal bots, as well. If that doesn't work, we have to pass that flag.
Labels: -Pri-1 Pri-2
Also this is hardly a P1, no?
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win7%20Tester has a Asus Xonar DX pcie audio device using a C-Media based driver.  It's version 7.12.8.1794.  The latest is Version 7.0.8.1821.

Uninstalling drivers for this will not work.   Want me to replace this device with a generic usb sound device?

There is also a usb audio device associated with the attached webcam which is using the std. ms usb audio driver.

Comment 48 by tommi@chromium.org, Oct 18 2017

If replacing the audio device with a generic one fixes the problem, then that would be ideal.  I'm assuming that the webcam only has a mic (not a speaker) and that the tests require both audio capture and rendering, so replacing with a separate audio device sounds like a plan to me.
Owner: phoglund@chromium.org
usb audio device has been installed.  Will be effective with the next build which will be https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win7%20Tester/builds/10549
Ohh, the audio quality test is disabled on Win at the moment, so we can't see any effects this had. Let me re-enable it.
Project Member

Comment 51 by bugdroid1@chromium.org, Oct 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/32b5eb6e2dee07a0f1da2335dd9a0533c46779b1

commit 32b5eb6e2dee07a0f1da2335dd9a0533c46779b1
Author: Patrik Höglund <phoglund@chromium.org>
Date: Thu Oct 19 09:04:43 2017

Revert "Disables WebRtcAudioQualityBrowserTest on Windows"

This reverts commit 885d259d95260d44c1dcbe602f4afbeaa1c74c70.

Reason for revert: Need to test new sound card config on win bots.

Original change's description:
> Disables WebRtcAudioQualityBrowserTest on Windows
> 
> TBR=phoglund
> 
> Bug: webrtc:677256
> Change-Id: I84a0fdf08a25204231ee1869918ecbffc8bdca9d
> Reviewed-on: https://chromium-review.googlesource.com/677458
> Reviewed-by: Henrik Andreasson <henrika@chromium.org>
> Commit-Queue: Henrik Andreasson <henrika@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#503516}

TBR=phoglund@chromium.org,henrika@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  677256 , 737090 
Change-Id: Ie41beeec00b3e3fbc7702ba6c7fd8a754c567e18
Reviewed-on: https://chromium-review.googlesource.com/727919
Reviewed-by: Patrik Höglund <phoglund@chromium.org>
Reviewed-by: Henrik Andreasson <henrika@chromium.org>
Commit-Queue: Patrik Höglund <phoglund@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510041}
[modify] https://crrev.com/32b5eb6e2dee07a0f1da2335dd9a0533c46779b1/chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc

pschmidt: did you replace the sound cards of both build97-b1 and build98-b1? In that case, it didn't work. If you only replaced one, the test has probably been broken and I need to fix it before we can tell whether we can replace those audio devices.
Only replaced it on build98-b1.  Let me know if you want the one in build97-b1 replaced.
Project Member

Comment 54 by bugdroid1@chromium.org, Oct 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/794407a995dfb2b199cad445890d68de84c7edeb

commit 794407a995dfb2b199cad445890d68de84c7edeb
Author: Patrik Höglund <phoglund@chromium.org>
Date: Fri Oct 20 07:59:45 2017

"Enable WebRtcAudioQualityBrowserTest on Windows"

This reverts commit 32b5eb6e2dee07a0f1da2335dd9a0533c46779b1.

Reason for revert: The test is broken, either through cruft accumulating or that the new sound cards don't work on the bot.

Original change's description:
> Revert "Disables WebRtcAudioQualityBrowserTest on Windows"
> 
> This reverts commit 885d259d95260d44c1dcbe602f4afbeaa1c74c70.
> 
> Reason for revert: Need to test new sound card config on win bots.
> 
> Original change's description:
> > Disables WebRtcAudioQualityBrowserTest on Windows
> > 
> > TBR=phoglund
> > 
> > Bug: webrtc:677256
> > Change-Id: I84a0fdf08a25204231ee1869918ecbffc8bdca9d
> > Reviewed-on: https://chromium-review.googlesource.com/677458
> > Reviewed-by: Henrik Andreasson <henrika@chromium.org>
> > Commit-Queue: Henrik Andreasson <henrika@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#503516}
> 
> TBR=phoglund@chromium.org,henrika@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug:  677256 , 737090 
> Change-Id: Ie41beeec00b3e3fbc7702ba6c7fd8a754c567e18
> Reviewed-on: https://chromium-review.googlesource.com/727919
> Reviewed-by: Patrik Höglund <phoglund@chromium.org>
> Reviewed-by: Henrik Andreasson <henrika@chromium.org>
> Commit-Queue: Patrik Höglund <phoglund@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#510041}

TBR=phoglund@chromium.org,henrika@chromium.org

Change-Id: I58c29571bf4fd298d733edaed5da741a4902d8e0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  677256 ,  737090 
Reviewed-on: https://chromium-review.googlesource.com/728259
Reviewed-by: Henrik Andreasson <henrika@chromium.org>
Commit-Queue: Patrik Höglund <phoglund@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510373}
[modify] https://crrev.com/794407a995dfb2b199cad445890d68de84c7edeb/chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc

Ok, then the test is broken, and I need to fix that before we can conclude whether the new sound card works or not.
Owner: tommi@chromium.org
Aha, now I can see what's going on. I must have logged in to the wrong machine yesterday. The test does work, at least most of the time, on build97-b1. It does not work on build98-b1.

The new sound card will not work. It does not offer the capability to loop back audio through the stereo mix device.

I'm considering getting rid of this magical bot config sometime soon and replace it with https://codereview.chromium.org/2773083002/, but we're not scheduled to work on that this quarter. So, for now, we'll have to go with an exception to COM_INIT_CHECK_HOOK_ENABLED in our bots. Tommi, can you prepare the change as per #43 and I'll get someone to help you with the bot config.
pschmidt, please switch back to the old sound card for build98-b1 and let me know when it's done, and I'll try to re-enable the test again.
Owner: pschmidt@chromium.org
Status: Assigned (was: Available)
Will do this tomorrow when I get back in to the lab.
Installed the asus pcie audio card into build98-b1 and enabled stereo mix.

Effective starting with https://uberchromegw.corp.google.com/i/chromium.webrtc.fyi/builders/Win7%20Tester/builds/10710
Cc: -tommi@chromium.org
Owner: tommi@chromium.org
Assigning back.
Tommi: I might as well set this define for all WebRTC builds. I could set it only on the bots, but I think developers will be confused if they ever trigger this check and it doesn't show up on the bots. 
Or wait, you're actually changing the Chromium code, so I can't set it in the WebRTC code. Ok, I'll try to make all win builders set that define then...
Ok, everything should be ready once tommi gets the CL through review (remember to address my comments).
Project Member

Comment 65 by bugdroid1@chromium.org, Nov 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0855a8dbdf1941d35b0bea0551ed3653e79d96b2

commit 0855a8dbdf1941d35b0bea0551ed3653e79d96b2
Author: Tommi <tommi@chromium.org>
Date: Mon Nov 20 12:46:35 2017

Add support for a flag to explicitly disable ComInitCheckHook.

This is to be used only on bots where there's a conflict with necessary
(for purposes of testing) 3rd party drivers/components that employ
patching as part of their function. Currently WebRTC's "Win Builder"
will need to set this flag.

BUG= 737090 

Change-Id: If483b06aa9f0f8a797e41f429cc363979a5c01f3
Reviewed-on: https://chromium-review.googlesource.com/753591
Commit-Queue: Tommi <tommi@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517805}
[modify] https://crrev.com/0855a8dbdf1941d35b0bea0551ed3653e79d96b2/base/win/com_init_check_hook.h

Project Member

Comment 66 by bugdroid1@chromium.org, Nov 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/960d47f7e78b6fc92d1bd493b6a4d13cd4e75d67

commit 960d47f7e78b6fc92d1bd493b6a4d13cd4e75d67
Author: Patrik Höglund <phoglund@chromium.org>
Date: Tue Nov 28 14:27:24 2017

Add GN var for com init check hook.

This adds the wiring needed by https://crrev.com/c/753728 to actually
disable com init check hooks for the WebRTC bots.

Bug:  chromium:737090 
Change-Id: I7ea209345475e2fd34de969e277ba523483506a9
Reviewed-on: https://chromium-review.googlesource.com/788411
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Patrik Höglund <phoglund@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519676}
[modify] https://crrev.com/960d47f7e78b6fc92d1bd493b6a4d13cd4e75d67/base/BUILD.gn

Owner: phoglund@chromium.org
Landing the last CL now...
Project Member

Comment 68 by bugdroid1@chromium.org, Nov 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9b3957ebadae93ff71e848b42f48f633c569db42

commit 9b3957ebadae93ff71e848b42f48f633c569db42
Author: Patrik Höglund <phoglund@chromium.org>
Date: Tue Nov 28 17:47:53 2017

Make WebRTC Chromium Win builders disable com init check hooks.

The WebRTC chromium bots need sound cards, and the drivers for those
sound cards inject themselves into the COM stack, which triggers a
CHECK chromium does to detect precisely this. Therefore, disable
this check on these particular builders.

We will aim to get rid of the sound cards and simplify the tests
in Q1, so hopefully this is just temporary.

Depends on crrev.com/c/753591.

Bug:  737090 
Change-Id: I6cc5603e345b0d8b8866b97abfd3db8d4e94e7eb
Reviewed-on: https://chromium-review.googlesource.com/753728
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Patrik Höglund <phoglund@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519732}
[modify] https://crrev.com/9b3957ebadae93ff71e848b42f48f633c569db42/tools/mb/mb_config.pyl

Owner: robliao@chromium.org
I've verified the WebRTC win bots now pass the GN arg: see generate_build_files step here: https://build.chromium.org/p/chromium.webrtc/builders/Win%20Builder/builds/64459

Back to robliao; I guess you want to turn the checks on for real now?
Project Member

Comment 70 by bugdroid1@chromium.org, Nov 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0c60c75f187c789f4908371391acb67db86b0ec

commit d0c60c75f187c789f4908371391acb67db86b0ec
Author: Robert Liao <robliao@chromium.org>
Date: Thu Nov 30 18:45:20 2017

Make Externally Patched COM Calls Fail the ComInitCheckHook

Now that WebRTC has disabled these hooks we can fail builds that
previously patched COM. This ensures that the hook is running where
we expect it to run instead of failing silently.

BUG= 737090 

Change-Id: I28b6f8203ecff3c3f54396848a70c78a17c9055e
Reviewed-on: https://chromium-review.googlesource.com/801250
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520618}
[modify] https://crrev.com/d0c60c75f187c789f4908371391acb67db86b0ec/base/win/com_init_check_hook.cc
[modify] https://crrev.com/d0c60c75f187c789f4908371391acb67db86b0ec/base/win/com_init_check_hook_unittest.cc

When the WebRTC bots remain green after this change, we can mark this as fixed.
Status: Fixed (was: Assigned)
The waterfall suggests we're good to go here! Thanks everyone!

Sign in to add a comment