local failure on net_perftests CookieMonsterTest.TestAddCookieOnManyHosts |
||||
Issue descriptionI was cleaning up the memory-infra callback stuff and at some point I got this while running net_perftests on OSX. ToT @ b5416b84aba8ec3442788d6bbea54091ef18b6b0 GN args: ----- is_component_build = true is_debug = false symbol_level = 2 dcheck_always_on = true ----- [ RUN ] CookieMonsterTest.TestAddCookieOnManyHosts [65870:775:0606/135134.831494:79466357540625:FATAL:cookie_monster.cc(231)] Check failed: static_cast<int>(num_sort) < it_end - it_begin (301 vs. 0) 0 libbase.dylib 0x000000010655330c base::debug::StackTrace::StackTrace(unsigned long) + 28 1 libbase.dylib 0x000000010657ef30 logging::LogMessage::~LogMessage() + 224 2 libnet.dylib 0x0000000105803471 net::CookieMonster::GarbageCollectLeastRecentlyAccessed(base::Time const&, base::Time const&, unsigned long, std::__1::vector<std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::__value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::unique_ptr<net::CanonicalCookie, std::__1::default_delete<net::CanonicalCookie> > >, std::__1::__tree_node<std::__1::__value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::unique_ptr<net::CanonicalCookie, std::__1::default_delete<net::CanonicalCookie> > >, void*>*, long> >, std::__1::allocator<std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::__value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::unique_ptr<net::CanonicalCookie, std::__1::default_delete<net::CanonicalCookie> > >, std::__1::__tree_node<std::__1::__value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::unique_ptr<net::CanonicalCookie, std::__1::default_delete<net::CanonicalCookie> > >, void*>*, long> > > >) + 225 3 libnet.dylib 0x0000000105802ea8 net::CookieMonster::GarbageCollect(base::Time const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 1816 4 libnet.dylib 0x00000001057fe8f0 net::CookieMonster::SetCanonicalCookie(std::__1::unique_ptr<net::CanonicalCookie, std::__1::default_delete<net::CanonicalCookie> >, GURL const&, net::CookieOptions const&) + 848 5 libnet.dylib 0x00000001057fef4d net::CookieMonster::SetCookieWithCreationTimeAndOptions(GURL const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, base::Time const&, net::CookieOptions const&) + 349 6 libnet.dylib 0x00000001057fb5d2 net::CookieMonster::SetCookieWithOptions(GURL const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, net::CookieOptions const&) + 162 7 libnet.dylib 0x00000001057fb504 net::CookieMonster::SetCookieWithOptionsTask::Run() + 36 8 libnet.dylib 0x00000001057fc69d net::CookieMonster::DoCookieTaskForURL(scoped_refptr<net::CookieMonster::CookieMonsterTask> const&, GURL const&) + 237 9 libnet.dylib 0x00000001057fd03f net::CookieMonster::SetCookieWithOptionsAsync(GURL const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, net::CookieOptions const&, base::Callback<void (bool), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) + 191 10 net_perftests 0x00000001056352fb net::CookieMonsterTest_TestAddCookieOnManyHosts_Test::TestBody() + 571 11 net_perftests 0x0000000105668b66 testing::Test::Run() + 246 12 net_perftests 0x0000000105669340 testing::TestInfo::Run() + 288 13 net_perftests 0x0000000105669867 testing::TestCase::Run() + 263 14 net_perftests 0x000000010566f877 testing::internal::UnitTestImpl::RunAllTests() + 871 15 net_perftests 0x000000010566f4e3 testing::UnitTest::Run() + 163 16 net_perftests 0x000000010568a4c3 base::TestSuite::Run() + 163 17 net_perftests 0x0000000105659f19 main + 41 18 libdyld.dylib 0x00007fffc6436235 start + 1
,
Jun 6 2017
[+mkwst just in case he has an opinion on the topic.] So I don't have any particular belief in cookie_monster_perftest.cc having value for the codebase. It hasn't been touched for reasons other than external refactors for more than a year, and when I was familiar with it (which was 7 years ago, so don't put a lot of weight on that) it really didn't seem to me to be very useful. Having said that, two caveats: * The fact that it's *crashing* suggests to me a potential problem in the cookie code, so I'd rather not "solve" that problem by deleting the test, and * I do think cookies are an area where it actually would be good to track performance at the unit level--it matters a fair amount to web performance, and could regress in a way that was hard to track. Which I guess is all by way of saying I think it's worth someone's time to try and come up with a good set of cookie performance tests before we nuke this (and debug the above crash in passing). +cbentzel for resource allocation.
,
Jun 6 2017
The DCHECK is trivial to reproduce. I'll dig into it and figure out what to do about it, so this doesn't get dropped on the floor. Also increasing the priority, as I don't think there's a bug in the test, and DCHECK failures should be given a fairly high priority, unless we're sure they're benign (And even then, should at least remove the DCHECK).
,
Jun 6 2017
The issue looks to be a real bug. When purging cookies, we first purge non-secure cookies with one limit, and then we purge secure+non-secure one. For the non-secure purge, we use a limit of max(limit, unsecure.size() - 1)...When there are no insecure cookies, the latter value overflows, and we pass in a nonsense final cookie iterator to partial_sort, which leads to undefined behavior. When there are insecure cookies, we may delete one more than we should (For example, if there's only one insecure cookie, even if it's newer than all the secure ones, we'll delete it).
,
Jun 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f4721d99f41f62c860b20f1cd53911bdd069d338 commit f4721d99f41f62c860b20f1cd53911bdd069d338 Author: mmenke <mmenke@chromium.org> Date: Wed Jun 07 17:13:59 2017 Fix CookieMonster garbage collection when no insecure cookies. The code wanted to always keep at least one insecure cookie around, so it could get the earliest access time of that cookie to update earliest_access_time_, but when there weren't any such cookies, this broke rather badly. That logic for calculating the earliest_access_time_ was also wrong, since it was set either only based on secure cookies, or only based on insecure cookies. This CL fixes both those issues. BUG= 730000 Review-Url: https://codereview.chromium.org/2924933002 Cr-Commit-Position: refs/heads/master@{#477689} [modify] https://crrev.com/f4721d99f41f62c860b20f1cd53911bdd069d338/net/cookies/cookie_monster.cc [modify] https://crrev.com/f4721d99f41f62c860b20f1cd53911bdd069d338/net/cookies/cookie_monster.h [modify] https://crrev.com/f4721d99f41f62c860b20f1cd53911bdd069d338/net/cookies/cookie_monster_unittest.cc
,
Jun 7 2017
Feel like we should have more tests around GC, but not sufficiently motivated to add them myself. Maybe I'll take it on some Friday, when I don't want to start anything new.
,
Jun 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bdefc4f71c8dd8dcd4b53d9adb2600c8574630bb commit bdefc4f71c8dd8dcd4b53d9adb2600c8574630bb Author: xunjieli <xunjieli@chromium.org> Date: Wed Jun 07 18:58:59 2017 Run net_perftests on Linux perf bots This CL makes Linux perf bots run net_perftests. BUG= 701387 , 730000 Review-Url: https://codereview.chromium.org/2748073003 Cr-Commit-Position: refs/heads/master@{#477715} [modify] https://crrev.com/bdefc4f71c8dd8dcd4b53d9adb2600c8574630bb/net/cookies/cookie_monster_perftest.cc [modify] https://crrev.com/bdefc4f71c8dd8dcd4b53d9adb2600c8574630bb/testing/buildbot/chromium.perf.json [modify] https://crrev.com/bdefc4f71c8dd8dcd4b53d9adb2600c8574630bb/testing/buildbot/gn_isolate_map.pyl [modify] https://crrev.com/bdefc4f71c8dd8dcd4b53d9adb2600c8574630bb/tools/perf/benchmark.csv [modify] https://crrev.com/bdefc4f71c8dd8dcd4b53d9adb2600c8574630bb/tools/perf/core/perf_data_generator.py
,
Jun 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/06a62c12314c7d283c707c1949901665d715b716 commit 06a62c12314c7d283c707c1949901665d715b716 Author: xunjieli <xunjieli@chromium.org> Date: Fri Jun 09 12:53:58 2017 Renable CookieMonsterTest.TestAddCookieOnManyHosts With crrev.com/f4721d99f41f62c860b20f1cd53911bdd069d338, this test is fixed. This CL renables this test. TBR=mmenke@chromium.org BUG= 701387 , 730000 Review-Url: https://codereview.chromium.org/2930033002 Cr-Commit-Position: refs/heads/master@{#478256} [modify] https://crrev.com/06a62c12314c7d283c707c1949901665d715b716/net/cookies/cookie_monster_perftest.cc |
||||
►
Sign in to add a comment |
||||
Comment 1 by xunji...@chromium.org
, Jun 6 2017Cc: rdsmith@chromium.org xunji...@chromium.org
Components: -Internals>Network>QUIC Internals>Network>Cookies
Owner: ----
Status: Available (was: Untriaged)