Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1779333003:
Add test for multithreaded ActiveVerifier behavior. (Closed)
|
Created:
4 years, 9 months ago by Will Harris Modified:
4 years, 9 months ago CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@handlecreate Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd test for multithreaded ActiveVerifier behavior.
Previous to crrev.com/678736a there was a race in the setting of the
g_active_verifier global that could have caused multiple instantiations.
This adds a test to verify the new behavior and detect any future
regressions. The test failing with the old code can be seen here:
http://pastebin.com/raw/iXz8j21C
BUG=592753
TEST=base_unittests --gtest_filter=ScopedHandleTest.MultiProcess
Committed: https://crrev.com/8f20e83de53c12033403e198b903f81112625d0b
Cr-Commit-Position: refs/heads/master@{#380835}
Patch Set 1 #Patch Set 2 : update test #Patch Set 3 : nits #
Total comments: 20
Patch Set 4 : code review changes #Patch Set 5 : multiprocess #
Total comments: 12
Patch Set 6 : code review comments. fix a thread start race. #
Total comments: 12
Patch Set 7 : avoid the typedef #Patch Set 8 : change to loadable_module #
Messages
Total messages: 28 (10 generated)
Description was changed from ========== Add crazy test BUG=592753 ========== to ========== Add test for multithreaded ActiveVerifier behavior. BUG=592753 ==========
wfh@chromium.org changed reviewers: + scottmg@chromium.org
PT an initial Look. Note: this test doesn't work because ICU initializes before the test runs in base_unittests so cannot test the race. I'd have to think of a way of fixing this either 1. add a flag to disable initializing icu in tests, and make the test manual 2. build a second process similar to the dll one and start that and do all the testing in there 3. add a catch in main of base_unittests to test for a flag to disable ICU init, and then (ab)use the existing multiprocess test system. I think 3 might work and be the cleanest. It's what we do in sbox_integration_tests, sort of.
https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle.cc File base/win/scoped_handle.cc (right): https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle.... base/win/scoped_handle.cc:296: return base::GetModuleFromAddress(pc); Can you just use &__ImageBase instead? .. Oh, I see, it's to make sure you got the right one. OK, just a comment in the header then describing why we want to do this. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle.h File base/win/scoped_handle.h (right): https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle.... base/win/scoped_handle.h:182: HMODULE BASE_EXPORT GetHandleVerifierModuleForTesting(); Could you put these BASE_EXPORTs before the return type like most files? https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle.... base/win/scoped_handle.h:182: HMODULE BASE_EXPORT GetHandleVerifierModuleForTesting(); Some sort of comment here. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... File base/win/scoped_handle_test_dll.cc (right): https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:29: Result InternalRunThreadTest() { This return value isn't useful. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:32: HANDLE start_event = ::CreateEvent(nullptr, true, false, nullptr); A comment here noting that `true` means we're going to release all threads simultaneouslyish because it doesn't reset. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:83: TIL DllMain() is optional. Huh. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... File base/win/scoped_handle_test_dll.h (right): https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. No (c). https://www.chromium.org/developers/coding-style#TOC-File-headers https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.h:12: enum class Result { base::win::testing::Result seems a bit too generic of a name. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.h:14: ERROR_COULD_NOT_CREATE_EVENT, You also don't seem to be using the great expanse of possible return values in the test-checking code, so maybe just return a bool instead. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... File base/win/scoped_handle_unittest.cc (right): https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_unittest.cc:8: #include <utility> (is this actually used?)
On 2016/03/11 18:52:28, Will Harris wrote: > PT an initial Look. > > Note: this test doesn't work because ICU initializes before the test runs in > base_unittests so cannot test the race. I'd have to think of a way of fixing > this either > > 1. add a flag to disable initializing icu in tests, and make the test manual > 2. build a second process similar to the dll one and start that and do all the > testing in there > 3. add a catch in main of base_unittests to test for a flag to disable ICU init, > and then (ab)use the existing multiprocess test system. > > I think 3 might work and be the cleanest. It's what we do in > sbox_integration_tests, sort of. I'd probably do #2, but whatever you think is best is fine.
changes made haven't yet added the multiprocess part. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle.cc File base/win/scoped_handle.cc (right): https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle.... base/win/scoped_handle.cc:296: return base::GetModuleFromAddress(pc); On 2016/03/11 19:02:18, scottmg wrote: > Can you just use &__ImageBase instead? > > .. Oh, I see, it's to make sure you got the right one. OK, just a comment in the > header then describing why we want to do this. I've actually changed this to use __ImageBase inside the member function. Much better. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle.h File base/win/scoped_handle.h (right): https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle.... base/win/scoped_handle.h:182: HMODULE BASE_EXPORT GetHandleVerifierModuleForTesting(); On 2016/03/11 19:02:18, scottmg wrote: > Some sort of comment here. Done. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle.... base/win/scoped_handle.h:182: HMODULE BASE_EXPORT GetHandleVerifierModuleForTesting(); On 2016/03/11 19:02:18, scottmg wrote: > Could you put these BASE_EXPORTs before the return type like most files? Done. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... File base/win/scoped_handle_test_dll.cc (right): https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:29: Result InternalRunThreadTest() { On 2016/03/11 19:02:18, scottmg wrote: > This return value isn't useful. now it is, I check more. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:32: HANDLE start_event = ::CreateEvent(nullptr, true, false, nullptr); On 2016/03/11 19:02:18, scottmg wrote: > A comment here noting that `true` means we're going to release all threads > simultaneouslyish because it doesn't reset. Done. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:83: On 2016/03/11 19:02:18, scottmg wrote: > TIL DllMain() is optional. Huh. Ack. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... File base/win/scoped_handle_test_dll.h (right): https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/03/11 19:02:18, scottmg wrote: > No (c). > > https://www.chromium.org/developers/coding-style#TOC-File-headers bah! I chose the wrong file to copy and paste. And I felt so proud of myself that I (for once) remembered to change the date to 2016. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.h:12: enum class Result { On 2016/03/11 19:02:18, scottmg wrote: > base::win::testing::Result seems a bit too generic of a name. base::win::testing::ScopedHandleTestResult okay? I can tell you don't like my enum. maybe once the test is fully passing I can change it to a bool? https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.h:14: ERROR_COULD_NOT_CREATE_EVENT, On 2016/03/11 19:02:18, scottmg wrote: > You also don't seem to be using the great expanse of possible return values in > the test-checking code, so maybe just return a bool instead. it's nice to know which error happened when it happens, but sure for testing I suppose it doens't matter. https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... File base/win/scoped_handle_unittest.cc (right): https://codereview.chromium.org/1779333003/diff/40001/base/win/scoped_handle_... base/win/scoped_handle_unittest.cc:8: #include <utility> On 2016/03/11 19:02:19, scottmg wrote: > (is this actually used?) no. removed.
Added multiprocess test. PTAL.
https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle.cc File base/win/scoped_handle.cc (right): https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle.... base/win/scoped_handle.cc:290: // Must pass by reference in order to avoid tail-call optimization. This seems a bit hokey. Would __declspec(noinline) do what you want? https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... File base/win/scoped_handle_test_dll.cc (right): https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:28: ScopedHandle handle_holder(handle); Does ActiveVerifier CHECK or DCHECK? Do we need to make it fatal-er always? https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:53: if (threads_.size() != kNumThreads) { I wouldn't bother with this, just return false instead of break since you're trying to get something to assert anyway. (And if CreateThread fails, the world is over anyway.). Does LOG break the test? Otherwise, consider logging on failures for CreateEvent, CreateThread, and WFSO.
https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle.cc File base/win/scoped_handle.cc (right): https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle.... base/win/scoped_handle.cc:290: // Must pass by reference in order to avoid tail-call optimization. On 2016/03/11 22:13:09, scottmg wrote: > This seems a bit hokey. Would __declspec(noinline) do what you want? I needed this when using tracked_objects::GetProgramCounter as it uses _ReturnAddress. In that case because it was doing a JMP to tracked_objects::GetProgramCounter the tail call optimization got the wrong return address, even when using NOINLINE. But, it's possible __ImageBase doesn't have this issue... although I doubt it since it's using the same intrinsic calling as _ReturnAddress. I can certainly try again. EDIT: oh interesting seem using volatile disables tail call optimization - see https://code.google.com/p/chromium/codesearch#chromium/src/base/profiler/stac... maybe that will work too. I'll try several things here. https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... File base/win/scoped_handle_test_dll.cc (right): https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:28: ScopedHandle handle_holder(handle); On 2016/03/11 22:13:09, scottmg wrote: > Does ActiveVerifier CHECK or DCHECK? Do we need to make it fatal-er always? it does a CHECK, so it is certainly quite fatal. the test failing looks like this: https://paste.googleplex.com/4738316201099264?raw https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:53: if (threads_.size() != kNumThreads) { On 2016/03/11 22:13:09, scottmg wrote: > I wouldn't bother with this, just return false instead of break since you're > trying to get something to assert anyway. (And if CreateThread fails, the world > is over anyway.). > > Does LOG break the test? Otherwise, consider logging on failures for > CreateEvent, CreateThread, and WFSO. bah why u not like my cleanup code?
https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... File base/win/scoped_handle_test_dll.cc (right): https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:28: ScopedHandle handle_holder(handle); On 2016/03/11 22:23:44, Will Harris wrote: > On 2016/03/11 22:13:09, scottmg wrote: > > Does ActiveVerifier CHECK or DCHECK? Do we need to make it fatal-er always? > > it does a CHECK, so it is certainly quite fatal. > > the test failing looks like this: > https://paste.googleplex.com/4738316201099264?raw OK, I was more worried about this test becoming pointless if someone decided that should only be DCHECK later. https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:53: if (threads_.size() != kNumThreads) { On 2016/03/11 22:23:44, Will Harris wrote: > On 2016/03/11 22:13:09, scottmg wrote: > > I wouldn't bother with this, just return false instead of break since you're > > trying to get something to assert anyway. (And if CreateThread fails, the > world > > is over anyway.). > > > > Does LOG break the test? Otherwise, consider logging on failures for > > CreateEvent, CreateThread, and WFSO. > > bah why u not like my cleanup code? I don't really care either way. You can keep it if you like, it just seems pointless to me.
Patchset #6 (id:100001) has been deleted
code review comments. also fixed a thread start race that had made the test flaky. https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle.cc File base/win/scoped_handle.cc (right): https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle.... base/win/scoped_handle.cc:290: // Must pass by reference in order to avoid tail-call optimization. On 2016/03/11 22:23:44, Will Harris wrote: > On 2016/03/11 22:13:09, scottmg wrote: > > This seems a bit hokey. Would __declspec(noinline) do what you want? > > I needed this when using tracked_objects::GetProgramCounter as it uses > _ReturnAddress. In that case because it was doing a JMP to > tracked_objects::GetProgramCounter the tail call optimization got the wrong > return address, even when using NOINLINE. > > But, it's possible __ImageBase doesn't have this issue... although I doubt it > since it's using the same intrinsic calling as _ReturnAddress. > > I can certainly try again. > > EDIT: oh interesting seem using volatile disables tail call optimization - see > https://code.google.com/p/chromium/codesearch#chromium/src/base/profiler/stac... > maybe that will work too. I'll try several things here. I simplified this, seems __ImageBase is less sensitive to tail-call optimization. https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... File base/win/scoped_handle_test_dll.cc (right): https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:28: ScopedHandle handle_holder(handle); On 2016/03/11 22:29:00, scottmg wrote: > On 2016/03/11 22:23:44, Will Harris wrote: > > On 2016/03/11 22:13:09, scottmg wrote: > > > Does ActiveVerifier CHECK or DCHECK? Do we need to make it fatal-er always? > > > > it does a CHECK, so it is certainly quite fatal. > > > > the test failing looks like this: > > https://paste.googleplex.com/4738316201099264?raw > > OK, I was more worried about this test becoming pointless if someone decided > that should only be DCHECK later. Acknowledged. https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:53: if (threads_.size() != kNumThreads) { On 2016/03/11 22:29:00, scottmg wrote: > On 2016/03/11 22:23:44, Will Harris wrote: > > On 2016/03/11 22:13:09, scottmg wrote: > > > I wouldn't bother with this, just return false instead of break since you're > > > trying to get something to assert anyway. (And if CreateThread fails, the > > world > > > is over anyway.). > > > > > > Does LOG break the test? Otherwise, consider logging on failures for > > > CreateEvent, CreateThread, and WFSO. > > > > bah why u not like my cleanup code? > > I don't really care either way. You can keep it if you like, it just seems > pointless to me. I left it. I hate leaks.
wfh@chromium.org changed reviewers: + thakis@chromium.org
Description was changed from ========== Add test for multithreaded ActiveVerifier behavior. BUG=592753 ========== to ========== Add test for multithreaded ActiveVerifier behavior. Previous to crrev.com/678736a there was a race in the setting of the g_active_verifier global that could have caused multiple instantiations. This adds a test to verify the new behavior and detect any future regressions. The test failing with the old code can be seen here: http://pastebin.com/raw/iXz8j21C BUG=592753 ==========
+nico for owner
Description was changed from ========== Add test for multithreaded ActiveVerifier behavior. Previous to crrev.com/678736a there was a race in the setting of the g_active_verifier global that could have caused multiple instantiations. This adds a test to verify the new behavior and detect any future regressions. The test failing with the old code can be seen here: http://pastebin.com/raw/iXz8j21C BUG=592753 ========== to ========== Add test for multithreaded ActiveVerifier behavior. Previous to crrev.com/678736a there was a race in the setting of the g_active_verifier global that could have caused multiple instantiations. This adds a test to verify the new behavior and detect any future regressions. The test failing with the old code can be seen here: http://pastebin.com/raw/iXz8j21C BUG=592753 TEST=base_unittests --gtest_filter=ScopedHandleTest.MultiProcess ==========
base/win/ lgtm but you need someone for the rest of base. https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... File base/win/scoped_handle_test_dll.cc (right): https://codereview.chromium.org/1779333003/diff/80001/base/win/scoped_handle_... base/win/scoped_handle_test_dll.cc:53: if (threads_.size() != kNumThreads) { On 2016/03/11 23:07:47, Will Harris wrote: > On 2016/03/11 22:29:00, scottmg wrote: > > On 2016/03/11 22:23:44, Will Harris wrote: > > > On 2016/03/11 22:13:09, scottmg wrote: > > > > I wouldn't bother with this, just return false instead of break since > you're > > > > trying to get something to assert anyway. (And if CreateThread fails, the > > > world > > > > is over anyway.). > > > > > > > > Does LOG break the test? Otherwise, consider logging on failures for > > > > CreateEvent, CreateThread, and WFSO. > > > > > > bah why u not like my cleanup code? > > > > I don't really care either way. You can keep it if you like, it just seems > > pointless to me. > > I left it. I hate leaks. You wouldn't appreciate the irony of a handle-checker test leaking handles? Pfft.
brace for incoming stream-of-consciousness review comments. after reading the whole cl, the comments at the start still make sense for once though, i think (usually i have to go back and delete most of my early comments when i add comments as i go) https://codereview.chromium.org/1779333003/diff/110001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1779333003/diff/110001/base/BUILD.gn#newcode1601 base/BUILD.gn:1601: ":base", ...you really want to link base into your test dll? https://codereview.chromium.org/1779333003/diff/110001/base/BUILD.gn#newcode1969 base/BUILD.gn:1969: deps += [ "//base:scoped_handle_test_dll" ] is this a dll loaded by tests? does it need to be in a data section so that the test can run on swarming? https://codereview.chromium.org/1779333003/diff/110001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1779333003/diff/110001/base/base.gyp#newcode703 base/base.gyp:703: 'scoped_handle_test_dll' ...and in gyp you instead link base_unittests against this dll? https://codereview.chromium.org/1779333003/diff/110001/base/win/scoped_handle... File base/win/scoped_handle_test_dll.cc (right): https://codereview.chromium.org/1779333003/diff/110001/base/win/scoped_handle... base/win/scoped_handle_test_dll.cc:35: ScopedHandle handle_holder(handle); this is just a CloseHandle call, right? https://codereview.chromium.org/1779333003/diff/110001/base/win/scoped_handle... base/win/scoped_handle_test_dll.cc:91: ScopedHandle handle_holder(handle); linking this against base just for one scoped handle seems like overkill. i think i'd either have a tiny special-case raii class inline at the top of this, or just have the 4 manual closehandle calls. (but up to you) https://codereview.chromium.org/1779333003/diff/110001/base/win/scoped_handle... File base/win/scoped_handle_unittest.cc (right): https://codereview.chromium.org/1779333003/diff/110001/base/win/scoped_handle... base/win/scoped_handle_unittest.cc:119: ScopedNativeLibrary module(FilePath(L"scoped_handle_test_dll.dll")); yeah, you need to add this to base.isolate and to the data section
https://codereview.chromium.org/1779333003/diff/110001/base/win/scoped_handle... File base/win/scoped_handle_test_dll.cc (right): https://codereview.chromium.org/1779333003/diff/110001/base/win/scoped_handle... base/win/scoped_handle_test_dll.cc:35: ScopedHandle handle_holder(handle); On 2016/03/11 23:26:58, Nico wrote: > this is just a CloseHandle call, right? wfh patiently explained to me that the point of this CL is to test a dll that links against base, so the comments in this file don't make any sense. The others mostly do though, I think.
thanks for comments. PTAL. https://codereview.chromium.org/1779333003/diff/110001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1779333003/diff/110001/base/BUILD.gn#newcode1601 base/BUILD.gn:1601: ":base", On 2016/03/11 23:26:58, Nico wrote: > ...you really want to link base into your test dll? yes, because I need to test behavior of base's active verifier from an exe and a dll loaded within the exe. https://codereview.chromium.org/1779333003/diff/110001/base/BUILD.gn#newcode1969 base/BUILD.gn:1969: deps += [ "//base:scoped_handle_test_dll" ] On 2016/03/11 23:26:58, Nico wrote: > is this a dll loaded by tests? does it need to be in a data section so that the > test can run on swarming? once I changed this to loadable_library then isolate picked it up. https://codereview.chromium.org/1779333003/diff/110001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1779333003/diff/110001/base/base.gyp#newcode703 base/base.gyp:703: 'scoped_handle_test_dll' On 2016/03/11 23:26:58, Nico wrote: > ...and in gyp you instead link base_unittests against this dll? now it's a loadable_module it shouldn't do any kind of linking against base_unittests. https://codereview.chromium.org/1779333003/diff/110001/base/win/scoped_handle... File base/win/scoped_handle_test_dll.cc (right): https://codereview.chromium.org/1779333003/diff/110001/base/win/scoped_handle... base/win/scoped_handle_test_dll.cc:35: ScopedHandle handle_holder(handle); On 2016/03/11 23:34:21, Nico wrote: > On 2016/03/11 23:26:58, Nico wrote: > > this is just a CloseHandle call, right? > > wfh patiently explained to me that the point of this CL is to test a dll that > links against base, so the comments in this file don't make any sense. The > others mostly do though, I think. Acknowledged. https://codereview.chromium.org/1779333003/diff/110001/base/win/scoped_handle... File base/win/scoped_handle_unittest.cc (right): https://codereview.chromium.org/1779333003/diff/110001/base/win/scoped_handle... base/win/scoped_handle_unittest.cc:119: ScopedNativeLibrary module(FilePath(L"scoped_handle_test_dll.dll")); On 2016/03/11 23:26:58, Nico wrote: > yeah, you need to add this to base.isolate and to the data section it seems I don't have to now it's a loadable_module target.
lgtm
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1779333003/#ps150001 (title: "change to loadable_module")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1779333003/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1779333003/150001
Message was sent while issue was closed.
Description was changed from ========== Add test for multithreaded ActiveVerifier behavior. Previous to crrev.com/678736a there was a race in the setting of the g_active_verifier global that could have caused multiple instantiations. This adds a test to verify the new behavior and detect any future regressions. The test failing with the old code can be seen here: http://pastebin.com/raw/iXz8j21C BUG=592753 TEST=base_unittests --gtest_filter=ScopedHandleTest.MultiProcess ========== to ========== Add test for multithreaded ActiveVerifier behavior. Previous to crrev.com/678736a there was a race in the setting of the g_active_verifier global that could have caused multiple instantiations. This adds a test to verify the new behavior and detect any future regressions. The test failing with the old code can be seen here: http://pastebin.com/raw/iXz8j21C BUG=592753 TEST=base_unittests --gtest_filter=ScopedHandleTest.MultiProcess ==========
Message was sent while issue was closed.
Committed patchset #8 (id:150001)
Message was sent while issue was closed.
Description was changed from ========== Add test for multithreaded ActiveVerifier behavior. Previous to crrev.com/678736a there was a race in the setting of the g_active_verifier global that could have caused multiple instantiations. This adds a test to verify the new behavior and detect any future regressions. The test failing with the old code can be seen here: http://pastebin.com/raw/iXz8j21C BUG=592753 TEST=base_unittests --gtest_filter=ScopedHandleTest.MultiProcess ========== to ========== Add test for multithreaded ActiveVerifier behavior. Previous to crrev.com/678736a there was a race in the setting of the g_active_verifier global that could have caused multiple instantiations. This adds a test to verify the new behavior and detect any future regressions. The test failing with the old code can be seen here: http://pastebin.com/raw/iXz8j21C BUG=592753 TEST=base_unittests --gtest_filter=ScopedHandleTest.MultiProcess Committed: https://crrev.com/8f20e83de53c12033403e198b903f81112625d0b Cr-Commit-Position: refs/heads/master@{#380835} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/8f20e83de53c12033403e198b903f81112625d0b Cr-Commit-Position: refs/heads/master@{#380835} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
