StaleHostResolverTest.StaleUsability reports memory leaks under ASAN with leak detection enabled. |
|||||
Issue description
Some of the test-cases (looks like two of them, IIUC) in StaleHostResolverTest.StaleUsability report a leak of the internal RequestImpl() instance. This is because of the Resolve() job being left active, if it is still running, when we return stale host results to the caller, in the hope of refreshing the host cache, e.g:
Direct leak of 640 byte(s) in 2 object(s) allocated from:
#0 0xeaa772 in operator new(unsigned long) /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cc:92:3
#1 0xed0a37 in cronet::StaleHostResolver::Resolve(net::HostResolver::RequestInfo const&, net::RequestPriority, net::AddressList*, base::RepeatingCallback<void (int)> const&, std::__1::unique_ptr<net::HostResolver::Request, std::__1::default_delete<net::HostResolver::Request> >*, net::NetLogWithSource const&) components/cronet/stale_host_resolver.cc:389:26
#2 0xef0c79 in cronet::(anonymous namespace)::StaleHostResolverTest::Resolve() components/cronet/stale_host_resolver_unittest.cc:207:20
#3 0xefac0b in cronet::(anonymous namespace)::StaleHostResolverTest_StaleUsability_Test::TestBody() components/cronet/stale_host_resolver_unittest.cc:459:5
#4 0xfa54ac in testing::Test::Run() third_party/googletest/src/googletest/src/gtest-internal-inl.h
#5 0xfa6bb4 in testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2661:11
#6 0xfa7f66 in testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2779:28
#7 0xfcdc66 in testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:5036:43
#8 0xfcceb3 in testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc
#9 0x45655c8 in RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2314:46
#10 0x45655c8 in base::TestSuite::Run() base/test/test_suite.cc:275
#11 0x456ec94 in Run base/callback.h:95:12
#12 0x456ec94 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, bool, base::OnceCallback<void ()>) base/test/launcher/unit_test_launcher.cc:225
#13 0x456e72a in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>) base/test/launcher/unit_test_launcher.cc:576:10
#14 0xeed770 in main components/cronet/run_all_unittests.cc:11:10
#15 0x7ff40ab892b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
Ideally the test should cause the outstanding request to complete, allowing the RequestImpl to tear itself down, before the test exits.
,
Apr 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c9d03f9ef7d636af0719b8c0cadfe46856e28c70 commit c9d03f9ef7d636af0719b8c0cadfe46856e28c70 Author: Stephen McGruer <smcgruer@chromium.org> Date: Fri Apr 06 14:27:42 2018 Revert "Enable Cronet tests on desktop platforms." This reverts commit 8fa7ecedfa0a5de4a27d93979d330b91e531b00c. Reason for revert: HistogramManager.HistogramBucketFields crashes consistently on Win7 Tests (dbg): https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=cronet_unittests&tests=HistogramManager.HistogramBucketFields First failing run: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win7%20Tests%20%28dbg%29%281%29/68309 Original change's description: > Enable Cronet tests on desktop platforms. > > - Only initialize global state (e.g. AtExitManager) in non-Debug builds, so that our component- > build Debug bots can run cronet_tests. > - Fix memory leaks in a number of unit-tests. > - Disable some StaleHostResolver tests under ASAN, since they have leaks > that need resolving. > > Bug: 812268 , 816705 , 829097 > Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet > Change-Id: I5084dd578682b2a8c128487c62de12e437e646f3 > Reviewed-on: https://chromium-review.googlesource.com/981787 > Commit-Queue: Wez <wez@chromium.org> > Reviewed-by: John Budorick <jbudorick@chromium.org> > Reviewed-by: Misha Efimov <mef@chromium.org> > Cr-Commit-Position: refs/heads/master@{#548672} TBR=wez@chromium.org,mef@chromium.org,jbudorick@chromium.org Change-Id: Ic30729594ae2b1264702c7bb77c971d55e5e9ae9 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 812268 , 816705 , 829097 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet Reviewed-on: https://chromium-review.googlesource.com/999334 Reviewed-by: Stephen McGruer <smcgruer@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#548771} [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/components/cronet/cronet_global_state_stubs.cc [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/components/cronet/native/runnables_unittest.cc [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/components/cronet/native/test/buffer_test.cc [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/components/cronet/native/test/executors_test.cc [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/components/cronet/native/test/url_request_test.cc [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/components/cronet/native/url_request.cc [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/components/cronet/stale_host_resolver.cc [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/components/cronet/stale_host_resolver_unittest.cc [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/testing/buildbot/chromium.clang.json [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/testing/buildbot/chromium.fyi.json [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/testing/buildbot/chromium.linux.json [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/testing/buildbot/chromium.mac.json [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/testing/buildbot/chromium.memory.json [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/testing/buildbot/chromium.win.json [modify] https://crrev.com/c9d03f9ef7d636af0719b8c0cadfe46856e28c70/testing/buildbot/test_suites.pyl
,
Apr 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/37176bc20f476b852a6139d1fa3d1fda76791709 commit 37176bc20f476b852a6139d1fa3d1fda76791709 Author: Wez <wez@chromium.org> Date: Fri Apr 06 20:07:44 2018 Reland "Enable Cronet tests on desktop platforms." This is a reland of 8fa7ecedfa0a5de4a27d93979d330b91e531b00c, which revealed several issues: - Histogram recording did not cope correctly with an empty metrics proto. - Several call-sites triggered MSan failures (e.g. due to read of uninitialized data). Original change's description: > Enable Cronet tests on desktop platforms. > > - Only initialize global state (e.g. AtExitManager) in non-Debug builds, so that our component- > build Debug bots can run cronet_tests. > - Fix memory leaks in a number of unit-tests. > - Disable some StaleHostResolver tests under ASAN, since they have leaks > that need resolving. > > Bug: 812268 , 816705 , 829097 > Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet > Change-Id: I5084dd578682b2a8c128487c62de12e437e646f3 > Reviewed-on: https://chromium-review.googlesource.com/981787 > Commit-Queue: Wez <wez@chromium.org> > Reviewed-by: John Budorick <jbudorick@chromium.org> > Reviewed-by: Misha Efimov <mef@chromium.org> > Cr-Commit-Position: refs/heads/master@{#548672} Bug: 812268 , 816705 , 829097 , 786559 Change-Id: Ia4027c994d475c48673c8e961f5d96255e703a15 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.linux:linux_chromium_msan_rel_ng;master.tryserver.chromium.linux:linux_chromium_asan_rel_ng Reviewed-on: https://chromium-review.googlesource.com/999932 Reviewed-by: Wez <wez@chromium.org> Reviewed-by: Misha Efimov <mef@chromium.org> Reviewed-by: John Budorick <jbudorick@chromium.org> Commit-Queue: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#548914} [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/components/cronet/cronet_global_state_stubs.cc [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/components/cronet/native/runnables_unittest.cc [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/components/cronet/native/test/buffer_test.cc [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/components/cronet/native/test/executors_test.cc [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/components/cronet/native/test/url_request_test.cc [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/components/cronet/native/url_request.cc [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/components/cronet/stale_host_resolver.cc [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/components/cronet/stale_host_resolver_unittest.cc [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/testing/buildbot/chromium.clang.json [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/testing/buildbot/chromium.fyi.json [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/testing/buildbot/chromium.linux.json [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/testing/buildbot/chromium.mac.json [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/testing/buildbot/chromium.memory.json [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/testing/buildbot/chromium.win.json [modify] https://crrev.com/37176bc20f476b852a6139d1fa3d1fda76791709/testing/buildbot/test_suites.pyl
,
Apr 9 2018
=> wez since he's been submitting CLs for this crbug.
,
Apr 9 2018
Re-assigning to mef@, since my contribution to this particular issue was just to suppress the leaky tests under ASAN. The leak itself still needs investigating.
,
Apr 25 2018
It became noticible because the retry also failed.
,
May 23 2018
I have reproduced it locally and debugged, but I'm not sure about good course of action. Basically we want network request to continue / complete after we serve the stale results, so the next time dns will be fresh and good. The problem is that from the point of view of the test resolve completes with stale results and test exits before network request completes. It seems like a problem with the test rather than a code, and we need some way for test to wait for completion of network request.
,
May 23 2018
I think StaleHostResolver::RequestImpl::OnHandleDestroyed() should never transition to unowned, instead it could transfer ownership to the StaleHostResolver. StaleHostResolver could have a pool of these unowned requests. When StaleHostResolver shuts down it could either wait for them to complete or cancel them.
,
May 24 2018
,
Jun 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e24384b33c4b7b5457e709b683e53bfd4cb1d37 commit 4e24384b33c4b7b5457e709b683e53bfd4cb1d37 Author: Misha Efimov <mef@chromium.org> Date: Mon Jun 04 15:10:25 2018 [Cronet] Fix flaky leaks in StaleHostResolverUnittest. - Add WaitForNetworkResolveComplete method to wait for completion of network requests before test completion. Bug: 829097 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I5190cf02dbfad6ceee18f79d228a0c4dfaa9ae25 Reviewed-on: https://chromium-review.googlesource.com/1067608 Reviewed-by: Paul Jensen <pauljensen@chromium.org> Commit-Queue: Misha Efimov <mef@chromium.org> Cr-Commit-Position: refs/heads/master@{#564094} [modify] https://crrev.com/4e24384b33c4b7b5457e709b683e53bfd4cb1d37/components/cronet/stale_host_resolver.cc [modify] https://crrev.com/4e24384b33c4b7b5457e709b683e53bfd4cb1d37/components/cronet/stale_host_resolver_unittest.cc
,
Jun 4 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by bugdroid1@chromium.org
, Apr 6 2018