Issue metadata
Sign in to add a comment
|
Many tests in gcm_unit_tests are flakily leaking |
||||||||||||||||||||||||
Issue descriptionGCMConnectionHandlerImplTest.InitIncompleteTimeout is flaky. Findit has detected 3 flake occurrences of this test within the past 24 hours. List of all flake occurrences can be found at: https://findit-for-me.appspot.com/flake/occurrences?key=ag9zfmZpbmRpdC1mb3ItbWVyVQsSBUZsYWtlIkpjaHJvbWl1bUBnY21fdW5pdF90ZXN0c0BHQ01Db25uZWN0aW9uSGFuZGxlckltcGxUZXN0LkluaXRJbmNvbXBsZXRlVGltZW91dAw. Unless the culprit CL is found and reverted, please disable this test first within 30 minutes then find an appropriate owner. If the result above is wrong, please file a bug using this link: https://bugs.chromium.org/p/chromium/issues/entry?status=Unconfirmed&labels=Pri-1,Test-Findit-Wrong&components=Tools%3ETest%3EFindit%3EFlakiness&summary=%5BFindit%5D%20Flake%20Detection%20-%20Wrong%20result%20for%20GCMConnectionHandlerImplTest.InitIncompleteTimeout&comment=Link%20to%20flake%20occurrences%3A%20https://findit-for-me.appspot.com/flake/occurrences?key=ag9zfmZpbmRpdC1mb3ItbWVyVQsSBUZsYWtlIkpjaHJvbWl1bUBnY21fdW5pdF90ZXN0c0BHQ01Db25uZWN0aW9uSGFuZGxlckltcGxUZXN0LkluaXRJbmNvbXBsZXRlVGltZW91dAw Automatically posted by the findit-for-me app (https://goo.gl/Ot9f7N).
,
Nov 20
There are many tests in gcm_unit_tests failing in the same manner. There's no obvious culprit in the recent CLs. I'm going to dedupe similar bugs into this.
,
Nov 20
Issue 906770 has been merged into this issue.
,
Nov 20
Issue 906771 has been merged into this issue.
,
Nov 20
Issue 906772 has been merged into this issue.
,
Nov 20
Issue 906773 has been merged into this issue.
,
Nov 20
Issue 906774 has been merged into this issue.
,
Nov 20
Issue 906775 has been merged into this issue.
,
Nov 20
Issue 906776 has been merged into this issue.
,
Nov 20
Issue 906777 has been merged into this issue.
,
Nov 20
Issue 906778 has been merged into this issue.
,
Nov 20
Issue 906819 has been merged into this issue.
,
Nov 20
Issue 906820 has been merged into this issue.
,
Nov 20
Issue 906821 has been merged into this issue.
,
Nov 20
Issue 906822 has been merged into this issue.
,
Nov 20
Issue 906823 has been merged into this issue.
,
Nov 20
Issue 906824 has been merged into this issue.
,
Nov 20
Issue 906825 has been merged into this issue.
,
Nov 20
Issue 906826 has been merged into this issue.
,
Nov 20
Issue 906827 has been merged into this issue.
,
Nov 20
Issue 906828 has been merged into this issue.
,
Nov 20
Really many objects are leaked, but the ultimate source of the leak seems like:
Indirect leak of 56 byte(s) in 1 object(s) allocated from:
#0 0x55a24ab11c12 in operator new(unsigned long) /b/swarming/w/ir/kitchen-workdir/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cc:106:3
#1 0x55a24e29bd2b in base::MessageLoopImpl::MessageLoopImpl(base::MessageLoopBase::Type) base/message_loop/message_loop_impl.cc:309:11
#2 0x55a24e2955e5 in make_unique<base::MessageLoopImpl, base::MessageLoopBase::Type &> buildtools/third_party/libc++/trunk/include/memory:3118:32
#3 0x55a24e2955e5 in CreateMessageLoopImpl base/message_loop/message_loop.cc:210
#4 0x55a24e2955e5 in base::MessageLoop::MessageLoop(base::MessageLoopBase::Type, base::OnceCallback<std::__1::unique_ptr<base::MessagePump, std::__1::default_delete<base::MessagePump> > ()>, base::MessageLoop::BackendType) base/message_loop/message_loop.cc:196
#5 0x55a24e295354 in MessageLoop base/message_loop/message_loop.cc:187:7
#6 0x55a24e295354 in base::MessageLoop::CreateUnbound(base::MessageLoopBase::Type, base::OnceCallback<std::__1::unique_ptr<base::MessagePump, std::__1::default_delete<base::MessagePump> > ()>) base/message_loop/message_loop.cc:183
#7 0x55a24e41387f in base::Thread::StartWithOptions(base::Thread::Options const&) base/threading/thread.cc:101:7
#8 0x55a24e611cdd in net::NetworkChangeNotifierLinux::NetworkChangeNotifierLinux(std::__1::unordered_set<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::hash<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::equal_to<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) net/base/network_change_notifier_linux.cc:107:21
#9 0x55a24e5f568e in net::NetworkChangeNotifier::Create() net/base/network_change_notifier.cc:219:14
#10 0x55a24f14d226 in CreateNetworkChangeNotifierIfNeeded services/network/network_service.cc:100:29
#11 0x55a24f14d226 in network::NetworkService::NetworkService(std::__1::unique_ptr<service_manager::BinderRegistryWithArgs<>, std::__1::default_delete<service_manager::BinderRegistryWithArgs<> > >, mojo::InterfaceRequest<network::mojom::NetworkService>, net::NetLog*) services/network/network_service.cc:182
#12 0x55a24f14f973 in network::NetworkService::CreateForTesting() services/network/network_service.cc:261:11
#13 0x55a24ab6c77d in gcm::ConnectionFactoryImplTest::ConnectionFactoryImplTest() google_apis/gcm/engine/connection_factory_impl_unittest.cc:323:24
#14 0x55a24ab8a60e in ConnectionFactoryImplTest_ConnectSuccess_Test google_apis/gcm/engine/connection_factory_impl_unittest.cc:368:1
#15 0x55a24ab8a60e in testing::internal::TestFactoryImpl<gcm::ConnectionFactoryImplTest_ConnectSuccess_Test>::CreateTest() third_party/googletest/src/googletest/include/gtest/internal/gtest-internal.h:472
#16 0x55a24b57ab1a in HandleExceptionsInMethodIfSupported<testing::internal::TestFactoryBase, testing::Test *> third_party/googletest/src/googletest/src/gtest.cc
#17 0x55a24b57ab1a in testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2693
#18 0x55a24b57c2a6 in testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2825:28
#19 0x55a24b5a4f66 in testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:5227:43
#20 0x55a24b5a42c2 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> third_party/googletest/src/googletest/src/gtest.cc
#21 0x55a24b5a42c2 in testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc:4835
#22 0x55a24e4e59a7 in RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2369:46
#23 0x55a24e4e59a7 in base::TestSuite::Run() base/test/test_suite.cc:294
#24 0x55a24e4ef864 in Run base/callback.h:99:12
#25 0x55a24e4ef864 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, bool, base::OnceCallback<void ()>) base/test/launcher/unit_test_launcher.cc:225
#26 0x55a24e4ef30d in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>) base/test/launcher/unit_test_launcher.cc:575:10
#27 0x55a24acabc22 in main google_apis/run_all_unittests.cc:13:10
#28 0x7f86adb02f44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)
,
Nov 20
Given the above stack, I suspect rmcelrath@'s NetworkChangeNotifier change r609498 could be the cause, but the appearances of flaky failures do not match the following timeline of the said patch, so I may be wrong. Reland r609498: Mon Nov 19 23:21:59 2018 +0000 Revert r608491: Thu Nov 15 20:23:54 2018 +0000 Reland r608265: Thu Nov 15 05:10:03 2018 +0000 Revert r607519: Tue Nov 13 08:28:14 2018 +0000
,
Nov 20
+Cloud Messaging owners
,
Nov 20
I don't have time to verify this right now (I will tomorrow if it's not fixed by then), but I'm guessing this was caused by another one of my CLs, crrev.com/c/1336066. I think adding a scoped_task_environment_.RunUntilIdle() to the test's destructor would fix it, or we could do what I did on crrev.com/c/1340913. I think the latter is cleaner than adding a seemingly random RunUntilIdle.
,
Nov 20
I think ideally we'd have network::NetworkService::CreateForTesting() create a mock NetworkChangeNotifier for us, but that could potentially break a lot of existing tests, so we'd probably want to do that after fixing this flake.
,
Nov 20
OK, let me try to do what's suggested in comment 25.
,
Nov 20
FTR, the test fixtures that are failing: GCMConnectionHandlerImplTest ConnectionFactoryImplTest GCMSocketStreamTest CheckinRequestTest AccountMappingTest
,
Nov 20
The last two seem innocent; they are not included in the leak reports but the test harness somehow thinks they have failed. The tests are run in a batch, so the innocent ones might have been included.
,
Nov 20
Speculative fix in CQ (going through the try runs because this is a real change): https://chromium-review.googlesource.com/c/chromium/src/+/1343592
,
Nov 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b6f5756e40f985d549bd147ad9c202fb2c539ad5 commit b6f5756e40f985d549bd147ad9c202fb2c539ad5 Author: Yuta Kitamura <yutak@chromium.org> Date: Tue Nov 20 08:49:05 2018 Speculative fix for flaky leaks in gcm_unit_tests. This CL tries to fix the test flakiness in gcm_unit_tests by using the mock NetworkChangeNotifier in tests, which avoids receiving real network status changes. Bug: 906769 Change-Id: I08227a6f303328fe2f44cecc84c0ea64bd145415 Tbr: rmcelrath@chromium.org Tbr: dimich@chromium.org Reviewed-on: https://chromium-review.googlesource.com/c/1343592 Reviewed-by: Yuta Kitamura <yutak@chromium.org> Commit-Queue: Yuta Kitamura <yutak@chromium.org> Cr-Commit-Position: refs/heads/master@{#609632} [modify] https://crrev.com/b6f5756e40f985d549bd147ad9c202fb2c539ad5/google_apis/gcm/base/socket_stream_unittest.cc [modify] https://crrev.com/b6f5756e40f985d549bd147ad9c202fb2c539ad5/google_apis/gcm/engine/connection_factory_impl_unittest.cc [modify] https://crrev.com/b6f5756e40f985d549bd147ad9c202fb2c539ad5/google_apis/gcm/engine/connection_handler_impl_unittest.cc
,
Nov 21
It looks an owner is assigned. Let me remove Sheriff-Chromium label.
,
Nov 22
The last occurrence of this was at 2018-11-20 07:07:22 UTC, which is about 46 hours ago. Seems like the fix is working! Thanks rmcelrath for your suggestion. |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by dtapu...@chromium.org
, Nov 19