ComInitCheckHook breaks WebRTC windows bots |
||||||||||||||||||||||||||
Issue descriptionSee 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]
,
Jun 27 2017
,
Jun 27 2017
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).
,
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
,
Jun 27 2017
,
Jun 27 2017
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.
,
Jun 27 2017
,
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
,
Jun 28 2017
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.
,
Jun 28 2017
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?
,
Jun 28 2017
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.
,
Jun 28 2017
Can you give me an example of a bot where this is passing?
,
Jun 28 2017
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.
,
Jun 28 2017
scottmg suggests this might be a hotpatch done by Windows for an update and we need to reboot the machines.
,
Jun 28 2017
,
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
,
Jun 29 2017
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.
,
Jun 29 2017
Can you explain to me what you want me to do?
,
Jun 29 2017
We want to see what's intercepting the hook on these bots. The suspicion is either drivers or malware.
,
Jul 19 2017
I have access to the WebRTC bot to investigate.
,
Jul 19 2017
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.
,
Aug 23 2017
What's the latest here? Is this still a valid issue?
,
Aug 23 2017
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.
,
Oct 12 2017
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?
,
Oct 12 2017
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.
,
Oct 13 2017
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.
,
Oct 13 2017
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.
,
Oct 13 2017
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
,
Oct 13 2017
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.
,
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.
,
Oct 13 2017
(oh and I totally agree that this behavior isn't nice. However, it's not that uncommon in the Windows world)
,
Oct 13 2017
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.
,
Oct 13 2017
Would there be a way to not have COM_INIT_CHECK_HOOK_ENABLED defined in the webrtc builds?
,
Oct 13 2017
Is there a marker we could use to identify WebRTC bots? Changing the driver would work too.
,
Oct 16 2017
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.
,
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.
,
Oct 16 2017
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.
,
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?
,
Oct 16 2017
Re: #39. Done!
,
Oct 16 2017
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.
,
Oct 16 2017
Plan SGTM. Le lun. 16 oct. 2017 16 h 43, robliao via monorail < monorail+v2.1842724928@chromium.org> a écrit :
,
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.
,
Oct 16 2017
Assigning to Patrik to investigate the first option.
,
Oct 18 2017
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.
,
Oct 18 2017
Also this is hardly a P1, no?
,
Oct 18 2017
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.
,
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.
,
Oct 18 2017
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
,
Oct 19 2017
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.
,
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
,
Oct 19 2017
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.
,
Oct 19 2017
Only replaced it on build98-b1. Let me know if you want the one in build97-b1 replaced.
,
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
,
Oct 20 2017
Ok, then the test is broken, and I need to fix that before we can conclude whether the new sound card works or not.
,
Oct 20 2017
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.
,
Oct 27 2017
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.
,
Oct 30 2017
Will do this tomorrow when I get back in to the lab.
,
Oct 31 2017
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
,
Nov 2 2017
Assigning back.
,
Nov 6 2017
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.
,
Nov 6 2017
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...
,
Nov 6 2017
,
Nov 9 2017
Ok, everything should be ready once tommi gets the CL through review (remember to address my comments).
,
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
,
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
,
Nov 28 2017
Landing the last CL now...
,
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
,
Nov 29 2017
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?
,
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
,
Nov 30 2017
When the WebRTC bots remain green after this change, we can mark this as fixed.
,
Nov 30 2017
The waterfall suggests we're good to go here! Thanks everyone! |
||||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||||
Comment 1 by guidou@chromium.org
, Jun 27 2017Components: Infra>Client>WebRTC