ListContainerTest.AppendByMovingDoesNotDestruct fails |
||||||||
Issue descriptionListContainerTest.AppendByMovingDoesNotDestruct has been failing on the Win7 bot for a while. According to FindIt: https://findit-for-me.appspot.com/waterfall/failure?redirect=1&url=https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win7%20Tests%20%28dbg%29%281%29/68450 this is traceable back to the googletest roll: https://chromium-review.googlesource.com/c/chromium/src/+/1004440
,
Apr 13 2018
[ RUN ] ListContainerTest.AppendByMovingDoesNotDestruct C:\b\c\win_toolchain\vs_files\1180cb75833ea365097e279efb2d5d7a42dee4b0\win_sdk\bin\..\..\VC\Tools\MSVC\14.11.25503\include\vector(52) : Assertion failed: vector iterator not dereferencable Received fatal exception EXCEPTION_BREAKPOINT Backtrace: base::TestSuite::SuppressErrorDialogs [0x028715A7+311] invalid_parameter [0x72D2E551+161] std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<testing::internal::linked_ptr<testing::internal::ExpectationBase> > > >::operator* [0x013FCFBE+174] std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<testing::internal::linked_ptr<testing::internal::ExpectationBase> > > >::operator-> [0x013FCEC1+17] std::_Operator_arrow<std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<testing::internal::linked_ptr<testing::internal::ExpectationBase> > > > &> [0x013FCE7E+30] std::reverse_iterator<std::_Vector_const_iterator<std::_Vector_val<std::_Simple_types<testing::internal::linked_ptr<testing::internal::ExpectationBase> > > > >::operator-> [0x013FC756+70] testing::internal::FunctionMockerBase<void __cdecl(void)>::FindMatchingExpectationLocked [0x013FC1D5+149] testing::internal::FunctionMockerBase<void __cdecl(void)>::UntypedFindMatchingExpectation [0x013F789D+109] testing::internal::UntypedFunctionMockerBase::UntypedInvokeWith [0x023BA062+1090] testing::internal::FunctionMockerBase<void __cdecl(void)>::InvokeWith [0x01400DDE+46] testing::internal::FunctionMocker<void __cdecl(void)>::Invoke [0x01400D82+50] testing::internal::TypedExpectation<void __cdecl(void)>::repeated_action [0x01400D3A+170] testing::internal::TypedExpectation<void __cdecl(void)>::repeated_action [0x01400CCC+60] testing::internal::FunctionMocker<void __cdecl(void)>::FunctionMocker<void __cdecl(void)> [0x013F73E5+101] testing::internal2::TypeWithoutFormatter<cc::IndexRect,2>::PrintValue [0x013F574F+16271] testing::Message::operator<<<char [5]> [0x014209C8+108152] testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void> [0x023FB1AB+107] testing::Test::Run [0x023FB0A8+184] testing::TestInfo::Run [0x023FBE6B+251] testing::TestCase::Run [0x023FCBDD+269] testing::internal::UnitTestImpl::RunAllTests [0x0240793B+715] testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool> [0x02407620+112] testing::UnitTest::Run [0x024073E9+297] RUN_ALL_TESTS [0x028712CF+15] base::TestSuite::Run [0x0287069E+142] ??$Invoke@PAVCCTestSuite@cc@@$$V@?$FunctorTraits@P8TestSuite@base@@AEHXZX@internal@base@@SAHP8TestSuite@2@AEHXZ$$QAPAVCCTestSuite@cc@@@Z [0x02385C0C+28] base::internal::InvokeHelper<0,int>::MakeItSo<int (__thiscall base::TestSuite::*const &)(void),cc::CCTestSuite *> [0x02385B2F+79] base::internal::Invoker<base::internal::BindState<int (__thiscall base::TestSuite::*)(void),base::internal::UnretainedWrapper<cc::CCTestSuite> >,int __cdecl(void)>::RunImpl<int (__thiscall base::TestSuite::*const &)(void),std::tuple<base::internal::Unreta [0x02385AA5+85] base::internal::Invoker<base::internal::BindState<int (__thiscall base::TestSuite::*)(void),base::internal::UnretainedWrapper<cc::CCTestSuite> >,int __cdecl(void)>::Run [0x0238590F+63] base::OnceCallback<int __cdecl(void)>::Run [0x0287ACF1+81] base::LaunchUnitTests [0x02878679+825] base::LaunchUnitTests [0x02878428+232] main [0x023855FB+219] invoke_main [0x02B2132E+30] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:78) __scrt_common_main_seh [0x02B211D0+336] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283) __scrt_common_main [0x02B2106D+13] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:326) mainCRTStartup [0x02B213A8+8] (f:\dd\vctools\crt\vcstartup\src\startup\exe_main.cpp:17) BaseThreadInitThunk [0x75EE336A+18] RtlInitializeExceptionChain [0x770A92B2+99] RtlInitializeExceptionChain [0x770A9285+54]
,
Apr 13 2018
The test defines some mock classes without virtual destructors. I'll submit a CL soon.
,
Apr 13 2018
,
Apr 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/939d436c2e5d98ad49600437f4c976e89c3bd9cf commit 939d436c2e5d98ad49600437f4c976e89c3bd9cf Author: Victor Costan <pwnall@chromium.org> Date: Sat Apr 14 00:16:21 2018 Define virtual destructors for mocks in list_container_unittest.cc. Bug: 832726 Change-Id: I43d694483586e3e944f4494308b213ebf8a7bd3d Reviewed-on: https://chromium-review.googlesource.com/1013141 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#550823} [modify] https://crrev.com/939d436c2e5d98ad49600437f4c976e89c3bd9cf/cc/base/list_container_unittest.cc
,
Apr 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/615ad4134db50d264d18a08717b3f69584475b0c commit 615ad4134db50d264d18a08717b3f69584475b0c Author: Victor Costan <pwnall@chromium.org> Date: Sat Apr 14 09:54:31 2018 Disable crashy part of ListContainerTest.AppendByMovingDoesNotDestruct. The test had the following behavior: 1) Construct a first mock object. 2) Move the first mock object to a new buffer by memcpy-ing a void*. 3) Construct a second mock object at the first object's address via placement-new. 4) Create a call expectation on the (now moved) first object. 5) Create a call expectation on the second object. 6) Destroy the second object (built at the first object's address). 7) Destroy the (now moved) first object. gmock objects don't support being moved via memcpy, as done by cc::ListContainer::AppendByMoving. In particular, each MOCK_METHODn macro creates a testing::FunctionMocker<> instance which (via testing::internal::FunctionMockerBase<> -> testing::internal::UntypedFunctionMockerBase) contains a list of expectations, which is std::vector<internal::linked_ptr<ExpectationBase>>. googletest's internal::linked_ptr is very similar to Chromium's version, and moving a node breaks the list pointers. So, while the ListContainerTest.AppendByMoving* tests have always used gmock in an unsupported way, the latest googletest roll turned this usage into a crash (debug assert) on Windows debug builds. This CL disables the test that causes the assertion to get the bot green, while a CL fixing all the tests is prepared and reviewed. TBR=chrishtr Bug: 832726 Change-Id: Ice1ff1930d2d2d40231a82bdb29eb5200a05fac7 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.win:win_chrome_official;master.tryserver.chromium.linux:linux_chromium_cfi_rel_ng Reviewed-on: https://chromium-review.googlesource.com/1013407 Reviewed-by: Victor Costan <pwnall@chromium.org> Commit-Queue: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#550889} [modify] https://crrev.com/615ad4134db50d264d18a08717b3f69584475b0c/cc/base/list_container_unittest.cc
,
Apr 14 2018
https://crrev.com/c/1013141 stopped the failures -- https://luci-milo.appspot.com/p/chromium/builders/luci.chromium.ci/Win7%20Tests%20%28dbg%29%281%29 https://crrev.com/c/1013408 is the long-term fix. I'll mark this bug as fixed once that CL goes through review and is committed.
,
Apr 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/25b2f3b24546921a0fd086dbc332440482e7acf4 commit 25b2f3b24546921a0fd086dbc332440482e7acf4 Author: Victor Costan <pwnall@chromium.org> Date: Sat Apr 14 15:34:36 2018 Remove unsupported gmock usage in ListContainerTest. gmock objects don't support being moved via memcpy, as done by cc::ListContainer::AppendByMoving. In particular, each MOCK_METHODn macro creates a testing::FunctionMocker<> instance which (via testing::internal::FunctionMockerBase<> -> testing::internal::UntypedFunctionMockerBase) contains a list of expectations, which is std::vector<internal::linked_ptr<ExpectationBase>>. googletest's internal::linked_ptr is very similar to Chromium's version, and moving a node breaks the list pointers. This CL removes gmock usage in ListContainerTest and replaces it with hand-rolled mocking code that does support having its state moved via memcpy(). Bug: 832726 Change-Id: I9abfe3146931a11eda8d52bd0f5621e08736f7d1 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.win:win_chrome_official;master.tryserver.chromium.linux:linux_chromium_cfi_rel_ng Reviewed-on: https://chromium-review.googlesource.com/1013408 Reviewed-by: enne <enne@chromium.org> Commit-Queue: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#550896} [modify] https://crrev.com/25b2f3b24546921a0fd086dbc332440482e7acf4/cc/base/list_container_unittest.cc
,
Apr 14 2018
cc_unittests passed on the Windows debug bot after the fix CL was submitted: https://luci-milo.appspot.com/p/chromium/builders/luci.chromium.ci/Win7%20Tests%20(dbg)(1)/68470
,
Apr 14 2018
Sooo... Your fix https://crrev.com/c/1013141 allowed CFI to start identifying types, which broke the CFI bot: https://ci.chromium.org/buildbot/chromium.memory/Linux%20CFI/ [ RUN ] ListContainerTest.AppendByMovingReplacesSourceWithNewDerivedElement ../../cc/base/list_container.h:198:14: runtime error: control flow integrity check for type 'cc::(anonymous namespace)::SimpleDerivedElementConstructMagicNumberTwo' failed during cast to unrelated type (vtable address 0x000000567010) 0x000000567010: note: vtable is of type 'cc::(anonymous namespace)::SimpleDerivedElementConstructMagicNumberOne' 00 00 00 00 40 c3 da 00 00 00 00 00 d0 d7 da 00 00 00 00 00 00 00 00 00 00 00 00 00 30 71 36 00 ^ #0 0xdaedca in cc::ListContainer<cc::(anonymous namespace)::SimpleDerivedElementConstructMagicNumberTwo>::Iterator::operator*() const ./../../cc/base/list_container.h:198:14 #1 0xdae6b4 in cc::ListContainer<cc::(anonymous namespace)::SimpleDerivedElementConstructMagicNumberTwo>::front() ./../../cc/base/list_container.h:88:37 #2 0xdae182 in cc::(anonymous namespace)::ListContainerTest_AppendByMovingReplacesSourceWithNewDerivedElement_Test::TestBody() ./../../cc/base/list_container_unittest.cc:1090:3 #3 0x17f3a33 in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2491:5 #4 0x17f42fc in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2667:11 #5 0x17f4a71 in testing::TestCase::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2785:28 #6 0x17fb772 in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:5047:43 #7 0x17fb3fd in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:4663:10 #8 0x19dc7e3 in base::TestSuite::Run() ./../../base/test/test_suite.cc:275:16 #9 0x14af580 in int base::internal::Invoker<base::internal::BindState<int (base::TestSuite::*)(), base::internal::UnretainedWrapper<cc::CCTestSuite> >, int ()>::RunImpl<int (base::TestSuite::* const&)(), std::__1::tuple<base::internal::UnretainedWrapper<cc::CCTestSuite> > const&, 0ul>(int (base::TestSuite::* const&&&)(), std::__1::tuple<base::internal::UnretainedWrapper<cc::CCTestSuite> > const&&&, std::__1::integer_sequence<unsigned long, 0ul>) ./../../base/bind_internal.h:604:12 #10 0x17d90c4 in base::OnceCallback<int ()>::Run() && ./../../base/callback.h:95:12 #11 0x19dff0f in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, bool, base::OnceCallback<void ()>) ./../../base/test/launcher/unit_test_launcher.cc:225:38 #12 0x19dfdd7 in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>) ./../../base/test/launcher/unit_test_launcher.cc:576:10 #13 0x14af422 in main ./../../cc/test/run_all_unittests.cc:15:10 #14 0x7f2b00165f44 in __libc_start_main /build/eglibc-SvCtMH/eglibc-2.19/csu/libc-start.c:287:0 #15 0xcf6029 in _start ??:0:0 Can you take a look?
,
Apr 14 2018
:( The last 2 CLs in this bug included the CFI bot on the CQ, and were green.
,
Apr 14 2018
Who owns the CFI bot? Something is really wrong if it can pass the CFI trybots and kill the CFI waterfall bot.
,
Apr 14 2018
Nico— All the changes in this bug have cleanly passed the CFI trybots, but killed the CFI waterfall bot. Who works on CFI? Who should know about this?
,
Apr 14 2018
Last time I had CFI problems, pcc@ helped. I think the problem is that the CL that added virtual destructors didn't check the CFI bot. The CFI builds for the other CL saw that cc_unittests was crashing, and said "shrug". https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_cfi_rel_ng/465 https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_cfi_rel_ng/463 FWIW, I'm guessing that what ListContainer is doing is non-portable (memcpy-ing types that aren't standard-layout), and my plan is to look carefully into casts. I'll poke around and try to make tests pass locally. Obviously, I have no idea what I'm doing, and I would appreciate help from someone who does :)
,
Apr 14 2018
At this point you could be forgiven for turning the test off and finding the owner.
,
Apr 14 2018
Thanks for the suggestion. That is a great first step -- even if I figure out a fix, getting the CFI bot green will make its future runs meaningful. I've put together https://crrev.com/c/1013538 and will see how it fares through the bots.
,
Apr 14 2018
BTW, is that a problem with the test, or is it looking like ListContainer itself is sketchy?
,
Apr 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/153813ad5ce9bc31afd2193c175bb6ec98547f23 commit 153813ad5ce9bc31afd2193c175bb6ec98547f23 Author: Victor Costan <pwnall@chromium.org> Date: Sat Apr 14 21:43:40 2018 Disable ListContainerTest.AppendByMovingReplacesSourceWithNewDerivedElement The test now fails on the Linux CFI bot. Disabling until a fix is figured out. TBR=enne Bug: 832726 Change-Id: I7624c38747a1787fdd470233de358f2b093e86da Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.win:win_chrome_official;master.tryserver.chromium.linux:linux_chromium_cfi_rel_ng Reviewed-on: https://chromium-review.googlesource.com/1013538 Reviewed-by: Victor Costan <pwnall@chromium.org> Commit-Queue: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#550910} [modify] https://crrev.com/153813ad5ce9bc31afd2193c175bb6ec98547f23/cc/base/list_container_unittest.cc
,
Apr 14 2018
I'm not enough of a C++ expert to call things "sketchy", but my suspicion is towards the code. ListContainer::AppendByMoving [1] looks like it'd require instances to be standard-layout, and the code is used with classes that have vtables. I'll do an actual investigation later today/tomorrow. [1] https://cs.chromium.org/chromium/src/cc/base/list_container.h?q=AppendByMoving&l=155
,
Apr 14 2018
Gah. We should DCHECK that the types used in construction are standard layout. This is not good.
,
Apr 14 2018
I know enough C++ to know that it's sketchy enough to confirm with the experts. We could ask on cxx if you want.
,
Apr 15 2018
Fixing a test bug is sufficient to make the CFI happy: https://crrev.com/c/1013617
,
Apr 16 2018
Commented here: https://bugs.chromium.org/p/chromium/issues/detail?id=833475#c1
,
Apr 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/09a5279e1c391273aa3d273bd812250bf5b05e2c commit 09a5279e1c391273aa3d273bd812250bf5b05e2c Author: Victor Costan <pwnall@chromium.org> Date: Mon Apr 16 20:50:18 2018 Fix ListContainerTest.AppendByMovingReplacesSourceWithNewDerivedElement. The test was disabled due to crashing on the Linux CFI bot. The test was using the following class hierarchy: * SimpleDerivedElement (SDE) - base class for our purposes * SimpleDerivedElementConstructMagicNumberOne (SDE1) - derived from SDE * SimpleDerivedElementConstructMagicNumberTwo (SDE2) - derived from SDE The test was doing the following steps: 1) Create a ListContainer<SDE1> and a ListContainer<SDE2> 2) Create an SDE1 instance in the ListContainer<SDE1> 3) Move the SDE1 instance in the ListContainer<SDE2> 4) Access the SDE2 instance via ListContainer<SDE2>::back() The CFI bot (correctly) crashed because step 4 is undefined behavior. SDE2 is not on SDE1's ancestor chain. ListContainer's templated definition is "ListContainer<BaseElementType>", suggesting that the list's templated type should be a base class for all elements. This CL fixes the undefined behavior by switching both ListContainers to ListContainer<SDE>, and verifies that both an SDE1 and an SDE2 instance are moved correctly between containers. Bug: 832726 Change-Id: Ic94acde5bd1a981d6bb763170a70a24f4306f409 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.win:win_chrome_official;master.tryserver.chromium.linux:linux_chromium_cfi_rel_ng Reviewed-on: https://chromium-review.googlesource.com/1013617 Commit-Queue: Victor Costan <pwnall@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: enne <enne@chromium.org> Cr-Commit-Position: refs/heads/master@{#551107} [modify] https://crrev.com/09a5279e1c391273aa3d273bd812250bf5b05e2c/cc/base/list_container_unittest.cc
,
Apr 16 2018
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/939d436c2e5d98ad49600437f4c976e89c3bd9cf commit 939d436c2e5d98ad49600437f4c976e89c3bd9cf Author: Victor Costan <pwnall@chromium.org> Date: Sat Apr 14 00:16:21 2018 Define virtual destructors for mocks in list_container_unittest.cc. Bug: 832726 Change-Id: I43d694483586e3e944f4494308b213ebf8a7bd3d Reviewed-on: https://chromium-review.googlesource.com/1013141 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#550823} [modify] https://crrev.com/939d436c2e5d98ad49600437f4c976e89c3bd9cf/cc/base/list_container_unittest.cc
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/615ad4134db50d264d18a08717b3f69584475b0c commit 615ad4134db50d264d18a08717b3f69584475b0c Author: Victor Costan <pwnall@chromium.org> Date: Sat Apr 14 09:54:31 2018 Disable crashy part of ListContainerTest.AppendByMovingDoesNotDestruct. The test had the following behavior: 1) Construct a first mock object. 2) Move the first mock object to a new buffer by memcpy-ing a void*. 3) Construct a second mock object at the first object's address via placement-new. 4) Create a call expectation on the (now moved) first object. 5) Create a call expectation on the second object. 6) Destroy the second object (built at the first object's address). 7) Destroy the (now moved) first object. gmock objects don't support being moved via memcpy, as done by cc::ListContainer::AppendByMoving. In particular, each MOCK_METHODn macro creates a testing::FunctionMocker<> instance which (via testing::internal::FunctionMockerBase<> -> testing::internal::UntypedFunctionMockerBase) contains a list of expectations, which is std::vector<internal::linked_ptr<ExpectationBase>>. googletest's internal::linked_ptr is very similar to Chromium's version, and moving a node breaks the list pointers. So, while the ListContainerTest.AppendByMoving* tests have always used gmock in an unsupported way, the latest googletest roll turned this usage into a crash (debug assert) on Windows debug builds. This CL disables the test that causes the assertion to get the bot green, while a CL fixing all the tests is prepared and reviewed. TBR=chrishtr Bug: 832726 Change-Id: Ice1ff1930d2d2d40231a82bdb29eb5200a05fac7 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.win:win_chrome_official;master.tryserver.chromium.linux:linux_chromium_cfi_rel_ng Reviewed-on: https://chromium-review.googlesource.com/1013407 Reviewed-by: Victor Costan <pwnall@chromium.org> Commit-Queue: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#550889} [modify] https://crrev.com/615ad4134db50d264d18a08717b3f69584475b0c/cc/base/list_container_unittest.cc
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/25b2f3b24546921a0fd086dbc332440482e7acf4 commit 25b2f3b24546921a0fd086dbc332440482e7acf4 Author: Victor Costan <pwnall@chromium.org> Date: Sat Apr 14 15:34:36 2018 Remove unsupported gmock usage in ListContainerTest. gmock objects don't support being moved via memcpy, as done by cc::ListContainer::AppendByMoving. In particular, each MOCK_METHODn macro creates a testing::FunctionMocker<> instance which (via testing::internal::FunctionMockerBase<> -> testing::internal::UntypedFunctionMockerBase) contains a list of expectations, which is std::vector<internal::linked_ptr<ExpectationBase>>. googletest's internal::linked_ptr is very similar to Chromium's version, and moving a node breaks the list pointers. This CL removes gmock usage in ListContainerTest and replaces it with hand-rolled mocking code that does support having its state moved via memcpy(). Bug: 832726 Change-Id: I9abfe3146931a11eda8d52bd0f5621e08736f7d1 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.win:win_chrome_official;master.tryserver.chromium.linux:linux_chromium_cfi_rel_ng Reviewed-on: https://chromium-review.googlesource.com/1013408 Reviewed-by: enne <enne@chromium.org> Commit-Queue: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#550896} [modify] https://crrev.com/25b2f3b24546921a0fd086dbc332440482e7acf4/cc/base/list_container_unittest.cc
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/153813ad5ce9bc31afd2193c175bb6ec98547f23 commit 153813ad5ce9bc31afd2193c175bb6ec98547f23 Author: Victor Costan <pwnall@chromium.org> Date: Sat Apr 14 21:43:40 2018 Disable ListContainerTest.AppendByMovingReplacesSourceWithNewDerivedElement The test now fails on the Linux CFI bot. Disabling until a fix is figured out. TBR=enne Bug: 832726 Change-Id: I7624c38747a1787fdd470233de358f2b093e86da Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.win:win_chrome_official;master.tryserver.chromium.linux:linux_chromium_cfi_rel_ng Reviewed-on: https://chromium-review.googlesource.com/1013538 Reviewed-by: Victor Costan <pwnall@chromium.org> Commit-Queue: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#550910} [modify] https://crrev.com/153813ad5ce9bc31afd2193c175bb6ec98547f23/cc/base/list_container_unittest.cc
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/09a5279e1c391273aa3d273bd812250bf5b05e2c commit 09a5279e1c391273aa3d273bd812250bf5b05e2c Author: Victor Costan <pwnall@chromium.org> Date: Mon Apr 16 20:50:18 2018 Fix ListContainerTest.AppendByMovingReplacesSourceWithNewDerivedElement. The test was disabled due to crashing on the Linux CFI bot. The test was using the following class hierarchy: * SimpleDerivedElement (SDE) - base class for our purposes * SimpleDerivedElementConstructMagicNumberOne (SDE1) - derived from SDE * SimpleDerivedElementConstructMagicNumberTwo (SDE2) - derived from SDE The test was doing the following steps: 1) Create a ListContainer<SDE1> and a ListContainer<SDE2> 2) Create an SDE1 instance in the ListContainer<SDE1> 3) Move the SDE1 instance in the ListContainer<SDE2> 4) Access the SDE2 instance via ListContainer<SDE2>::back() The CFI bot (correctly) crashed because step 4 is undefined behavior. SDE2 is not on SDE1's ancestor chain. ListContainer's templated definition is "ListContainer<BaseElementType>", suggesting that the list's templated type should be a base class for all elements. This CL fixes the undefined behavior by switching both ListContainers to ListContainer<SDE>, and verifies that both an SDE1 and an SDE2 instance are moved correctly between containers. Bug: 832726 Change-Id: Ic94acde5bd1a981d6bb763170a70a24f4306f409 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.win:win_chrome_official;master.tryserver.chromium.linux:linux_chromium_cfi_rel_ng Reviewed-on: https://chromium-review.googlesource.com/1013617 Commit-Queue: Victor Costan <pwnall@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: enne <enne@chromium.org> Cr-Commit-Position: refs/heads/master@{#551107} [modify] https://crrev.com/09a5279e1c391273aa3d273bd812250bf5b05e2c/cc/base/list_container_unittest.cc
,
Apr 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3cbcdf18fe427993f5312cbdab8debed30e18202 commit 3cbcdf18fe427993f5312cbdab8debed30e18202 Author: danakj <danakj@chromium.org> Date: Wed Apr 18 17:56:58 2018 cc: Remove unused AppendByMoving from ListContainer R=enne@chromium.org Bug: 832726 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I3e21bd4a54b82fcf7f9f07ae36cdfdb65034fe39 Reviewed-on: https://chromium-review.googlesource.com/1015820 Reviewed-by: enne <enne@chromium.org> Commit-Queue: danakj <danakj@chromium.org> Cr-Commit-Position: refs/heads/master@{#551739} [modify] https://crrev.com/3cbcdf18fe427993f5312cbdab8debed30e18202/cc/base/list_container.h [modify] https://crrev.com/3cbcdf18fe427993f5312cbdab8debed30e18202/cc/base/list_container_unittest.cc |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by a...@chromium.org
, Apr 13 2018