Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
321283002:
Clear SDCH information on "Clear browsing data" path. (Closed)
|
Created:
6 years, 6 months ago by Randy Smith (Not in Mondays) Modified:
6 years, 6 months ago CC:
chromium-reviews, markusheintz_, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionClear SDCH information on "Clear browsing data" path.
BUG=370567
BUG=327783
R=jar@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277607
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278297
Patch Set 1 #
Total comments: 2
Patch Set 2 : Brought up to date with latest changes to 298063006. #Patch Set 3 : Incorporated comment. #
Total comments: 2
Patch Set 4 : Move conditional comment before if. #Patch Set 5 : Empty queue by hand (swap is C++11 only). #Patch Set 6 : Sync'd to r277335 #Patch Set 7 : New patch vs. master rather than dependent branch. #
Total comments: 2
Patch Set 8 : Fix Cancel() race. #Patch Set 9 : Sync'd to r277522. #Patch Set 10 : Fixed memory leak. #
Total comments: 12
Patch Set 11 : Rebased to r277696, reverting revert. #Patch Set 12 : Incorporated comments and expanded scoped_refptr<> usage. #
Total comments: 2
Patch Set 13 : Incorporated comments. #
Messages
Total messages: 23 (0 generated)
Jim: PTAL?
Noting that this is dependent on CL https://codereview.chromium.org/298063006.
LGTM % nit below. If the nit change offends you... then skip the nit... as the code is IMO correct... it is just a style issue that is sometimes safer. https://chromiumcodereview.appspot.com/321283002/diff/1/chrome/browser/net/sd... File chrome/browser/net/sdch_dictionary_fetcher.cc (right): https://chromiumcodereview.appspot.com/321283002/diff/1/chrome/browser/net/sd... chrome/browser/net/sdch_dictionary_fetcher.cc:59: fetch_queue_.swap(trash_queue); nit: (personal style): It is probably safer to first effectively clear the queue (via your swap technique), and then invalidate the weak pointers, on the *off* chance that some side effect of the pointer release would induce reentrant callbacks which examine or test the queue. I don't think this happens... but it is a personal style issue which sometimes has saved me (and failure to do so has historically hurt me in some code). YMMV.
Jim: Thanks! But if you could spare a moment, I'd be curious as to more details around the weak pointer invalidation -> reentrancy possibility you saw. Scott: Another stamp? (browsing_data_remover.cc) https://codereview.chromium.org/321283002/diff/1/chrome/browser/net/sdch_dict... File chrome/browser/net/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/321283002/diff/1/chrome/browser/net/sdch_dict... chrome/browser/net/sdch_dictionary_fetcher.cc:59: fetch_queue_.swap(trash_queue); On 2014/06/11 23:23:49, jar wrote: > nit: (personal style): It is probably safer to first effectively clear the queue > (via your swap technique), and then invalidate the weak pointers, on the *off* > chance that some side effect of the pointer release would induce reentrant > callbacks which examine or test the queue. > > I don't think this happens... but it is a personal style issue which sometimes > has saved me (and failure to do so has historically hurt me in some code). > YMMV. Hmmm. So your comment made me think about ordering, and the conclusions I came to were: * I can't see a way in which invalidating weak pointers can result in re-entrancy, because invalidating weak pointers breaks links to this class without affecting ownership or deletion semantics. * I *could* imagine the reset of the URLFetcher resulting in some interesting re-entrancy. So I did the URLFetcher reset last, and did the weak pointer invalidation second-to-last as per your request/in case I'm missing something. But I'd be interested in more details about how you think invalidating weak pointers could result in re-entrancy if you're willing to share.
LGTM https://chromiumcodereview.appspot.com/321283002/diff/40001/chrome/browser/br... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://chromiumcodereview.appspot.com/321283002/diff/40001/chrome/browser/br... chrome/browser/browsing_data/browsing_data_remover.cc:938: // The test is probably overkill, since chrome should always nit: Comments generally come before the code in question. Since your comment is above the conditional move it above the if. Doing so is also nice since then the if is a single line.
Thanks! https://codereview.chromium.org/321283002/diff/40001/chrome/browser/browsing_... File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/321283002/diff/40001/chrome/browser/browsing_... chrome/browser/browsing_data/browsing_data_remover.cc:938: // The test is probably overkill, since chrome should always On 2014/06/12 19:46:44, sky wrote: > nit: Comments generally come before the code in question. Since your comment is > above the conditional move it above the if. Doing so is also nice since then the > if is a single line. Done.
https://codereview.chromium.org/321283002/diff/160001/chrome/browser/net/sdch... File chrome/browser/net/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/321283002/diff/160001/chrome/browser/net/sdch... chrome/browser/net/sdch_dictionary_fetcher.cc:75: task_is_pending_ = false; Now that there is a Cancel() method, there seems to be a chance that a task was posted (line 65) while there was something in the fetch_queue_, but now (due to a Cancel()?) the fetch queue could be empty. If so, it would be good to test on line 76: if (fetch_queue_.empty()) return;
PTAL? https://codereview.chromium.org/321283002/diff/160001/chrome/browser/net/sdch... File chrome/browser/net/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/321283002/diff/160001/chrome/browser/net/sdch... chrome/browser/net/sdch_dictionary_fetcher.cc:75: task_is_pending_ = false; On 2014/06/16 18:55:04, jar wrote: > Now that there is a Cancel() method, there seems to be a chance that a task was > posted (line 65) while there was something in the fetch_queue_, but now (due to > a Cancel()?) the fetch queue could be empty. > > If so, it would be good to test on line 76: > if (fetch_queue_.empty()) > return; Whoops. Thanks for catching that. Done.
lgtm
The CQ bit was checked by rdsmith@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/321283002/200001
Message was sent while issue was closed.
Change committed as 277607
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/339763003/ by sorin@chromium.org. The reason for reverting is: Fails ASAN tests: (view as text) SdchManagerTest.ClearDictionaryData (run #1): [ RUN ] SdchManagerTest.ClearDictionaryData [ OK ] SdchManagerTest.ClearDictionaryData (0 ms) [----------] 1 test from SdchManagerTest (0 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (2 ms total) [ PASSED ] 1 test. YOU HAVE 107 DISABLED TESTS ================================================================= ==16633==ERROR: LeakSanitizer: detected memory leaks Direct leak of 192 byte(s) in 1 object(s) allocated from: #0 0x51301b in operator new(unsigned long) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:55 #1 0x34c6174 in net::SdchManager::AddSdchDictionary(std::string const&, GURL const&) net/base/sdch_manager.cc:469 #2 0x808536 in net::SdchManagerTest_ClearDictionaryData_Test::TestBody() net/base/sdch_manager_unittest.cc:454 #3 0x33173a8 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #4 0x33173a8 in testing::Test::Run() testing/gtest/src/gtest.cc:2061 #5 0x3319b89 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #6 0x331a916 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #7 0x332da2a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #8 0x332d060 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #9 0x332d060 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 <truncated, full output is in gzipped JSON output at end of step> #5 0x7eff91b98391 in std::string::append(unsigned long, char) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:290 #6 0x7eff91b97aeb in std::string::resize(unsigned long, char) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:647 #7 0x31f1b12 in resize /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/basic_string.h:749 #8 0x31f1b12 in base::Base64Encode(base::BasicStringPiece\u003Cstd::string> const&, std::string*) base/base64.cc:13 #9 0x34c73d5 in net::SdchManager::UrlSafeBase64Encode(std::string const&, std::string*) net/base/sdch_manager.cc:558 #10 0x34c67e7 in net::SdchManager::GenerateHash(std::string const&, std::string*, std::string*) net/base/sdch_manager.cc:524 #11 0x34c519b in net::SdchManager::AddSdchDictionary(std::string const&, GURL const&) net/base/sdch_manager.cc:380 #12 0x808536 in net::SdchManagerTest_ClearDictionaryData_Test::TestBody() net/base/sdch_manager_unittest.cc:454 #13 0x33173a8 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #14 0x33173a8 in testing::Test::Run() testing/gtest/src/gtest.cc:2061 #15 0x3319b89 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #16 0x331a916 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #17 0x332da2a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #18 0x332d060 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #19 0x332d060 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 #20 0x3fc8ddc in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231 #21 0x3fc8ddc in base::TestSuite::Run() base/test/test_suite.cc:227 #22 0x3fbd5a2 in Run base/callback.h:401 #23 0x3fbd5a2 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, bool, base::Callback\u003Cvoid ()> const&) base/test/launcher/unit_test_launcher.cc:498 #24 0x3fbcf1d in base::LaunchUnitTests(int, char**, base::Callback\u003Cint ()> const&) base/test/launcher/unit_test_launcher.cc:553 #25 0x2ae277c in main net/test/run_all_unittests.cc:64 #26 0x7eff9152d76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 SUMMARY: AddressSanitizer: 449 byte(s) leaked in 5 allocation(s). SdchManagerTest.ClearDictionaryData (run #2): [ RUN ] SdchManagerTest.ClearDictionaryData [ OK ] SdchManagerTest.ClearDictionaryData (1 ms) [----------] 1 test from SdchManagerTest (1 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (1 ms total) [ PASSED ] 1 test. YOU HAVE 107 DISABLED TESTS ================================================================= ==3818==ERROR: LeakSanitizer: detected memory leaks Direct leak of 192 byte(s) in 1 object(s) allocated from: #0 0x51301b in operator new(unsigned long) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:55 #1 0x34c6174 in net::SdchManager::AddSdchDictionary(std::string const&, GURL const&) net/base/sdch_manager.cc:469 #2 0x808536 in net::SdchManagerTest_ClearDictionaryData_Test::TestBody() net/base/sdch_manager_unittest.cc:454 #3 0x33173a8 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #4 0x33173a8 in testing::Test::Run() testing/gtest/src/gtest.cc:2061 #5 0x3319b89 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #6 0x331a916 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #7 0x332da2a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #8 0x332d060 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #9 0x332d060 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 <truncated, full output is in gzipped JSON output at end of step> #5 0x7ffa92ba6391 in std::string::append(unsigned long, char) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:290 #6 0x7ffa92ba5aeb in std::string::resize(unsigned long, char) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:647 #7 0x31f1b12 in resize /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/basic_string.h:749 #8 0x31f1b12 in base::Base64Encode(base::BasicStringPiece\u003Cstd::string> const&, std::string*) base/base64.cc:13 #9 0x34c73d5 in net::SdchManager::UrlSafeBase64Encode(std::string const&, std::string*) net/base/sdch_manager.cc:558 #10 0x34c67e7 in net::SdchManager::GenerateHash(std::string const&, std::string*, std::string*) net/base/sdch_manager.cc:524 #11 0x34c519b in net::SdchManager::AddSdchDictionary(std::string const&, GURL const&) net/base/sdch_manager.cc:380 #12 0x808536 in net::SdchManagerTest_ClearDictionaryData_Test::TestBody() net/base/sdch_manager_unittest.cc:454 #13 0x33173a8 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #14 0x33173a8 in testing::Test::Run() testing/gtest/src/gtest.cc:2061 #15 0x3319b89 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #16 0x331a916 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #17 0x332da2a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #18 0x332d060 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #19 0x332d060 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 #20 0x3fc8ddc in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231 #21 0x3fc8ddc in base::TestSuite::Run() base/test/test_suite.cc:227 #22 0x3fbd5a2 in Run base/callback.h:401 #23 0x3fbd5a2 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, bool, base::Callback\u003Cvoid ()> const&) base/test/launcher/unit_test_launcher.cc:498 #24 0x3fbcf1d in base::LaunchUnitTests(int, char**, base::Callback\u003Cint ()> const&) base/test/launcher/unit_test_launcher.cc:553 #25 0x2ae277c in main net/test/run_all_unittests.cc:64 #26 0x7ffa9253b76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 SUMMARY: AddressSanitizer: 449 byte(s) leaked in 5 allocation(s). SdchManagerTest.ClearDictionaryData (run #3): [ RUN ] SdchManagerTest.ClearDictionaryData [ OK ] SdchManagerTest.ClearDictionaryData (0 ms) [----------] 1 test from SdchManagerTest (1 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (1 ms total) [ PASSED ] 1 test. YOU HAVE 107 DISABLED TESTS ================================================================= ==3824==ERROR: LeakSanitizer: detected memory leaks Direct leak of 192 byte(s) in 1 object(s) allocated from: #0 0x51301b in operator new(unsigned long) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:55 #1 0x34c6174 in net::SdchManager::AddSdchDictionary(std::string const&, GURL const&) net/base/sdch_manager.cc:469 #2 0x808536 in net::SdchManagerTest_ClearDictionaryData_Test::TestBody() net/base/sdch_manager_unittest.cc:454 #3 0x33173a8 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #4 0x33173a8 in testing::Test::Run() testing/gtest/src/gtest.cc:2061 #5 0x3319b89 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #6 0x331a916 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #7 0x332da2a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #8 0x332d060 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #9 0x332d060 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 <truncated, full output is in gzipped JSON output at end of step> #5 0x7f8ff892e391 in std::string::append(unsigned long, char) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:290 #6 0x7f8ff892daeb in std::string::resize(unsigned long, char) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:647 #7 0x31f1b12 in resize /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/basic_string.h:749 #8 0x31f1b12 in base::Base64Encode(base::BasicStringPiece\u003Cstd::string> const&, std::string*) base/base64.cc:13 #9 0x34c73d5 in net::SdchManager::UrlSafeBase64Encode(std::string const&, std::string*) net/base/sdch_manager.cc:558 #10 0x34c67e7 in net::SdchManager::GenerateHash(std::string const&, std::string*, std::string*) net/base/sdch_manager.cc:524 #11 0x34c519b in net::SdchManager::AddSdchDictionary(std::string const&, GURL const&) net/base/sdch_manager.cc:380 #12 0x808536 in net::SdchManagerTest_ClearDictionaryData_Test::TestBody() net/base/sdch_manager_unittest.cc:454 #13 0x33173a8 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #14 0x33173a8 in testing::Test::Run() testing/gtest/src/gtest.cc:2061 #15 0x3319b89 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #16 0x331a916 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #17 0x332da2a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #18 0x332d060 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #19 0x332d060 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 #20 0x3fc8ddc in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231 #21 0x3fc8ddc in base::TestSuite::Run() base/test/test_suite.cc:227 #22 0x3fbd5a2 in Run base/callback.h:401 #23 0x3fbd5a2 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, bool, base::Callback\u003Cvoid ()> const&) base/test/launcher/unit_test_launcher.cc:498 #24 0x3fbcf1d in base::LaunchUnitTests(int, char**, base::Callback\u003Cint ()> const&) base/test/launcher/unit_test_launcher.cc:553 #25 0x2ae277c in main net/test/run_all_unittests.cc:64 #26 0x7f8ff82c376c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 SUMMARY: AddressSanitizer: 449 byte(s) leaked in 5 allocation(s). SdchManagerTest.ClearDictionaryData (run #4): [ RUN ] SdchManagerTest.ClearDictionaryData [ OK ] SdchManagerTest.ClearDictionaryData (1 ms) [----------] 1 test from SdchManagerTest (1 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (1 ms total) [ PASSED ] 1 test. YOU HAVE 107 DISABLED TESTS ================================================================= ==3827==ERROR: LeakSanitizer: detected memory leaks Direct leak of 192 byte(s) in 1 object(s) allocated from: #0 0x51301b in operator new(unsigned long) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:55 #1 0x34c6174 in net::SdchManager::AddSdchDictionary(std::string const&, GURL const&) net/base/sdch_manager.cc:469 #2 0x808536 in net::SdchManagerTest_ClearDictionaryData_Test::TestBody() net/base/sdch_manager_unittest.cc:454 #3 0x33173a8 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #4 0x33173a8 in testing::Test::Run() testing/gtest/src/gtest.cc:2061 #5 0x3319b89 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #6 0x331a916 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #7 0x332da2a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #8 0x332d060 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #9 0x332d060 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 <truncated, full output is in gzipped JSON output at end of step> #5 0x7ff270a3c391 in std::string::append(unsigned long, char) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:290 #6 0x7ff270a3baeb in std::string::resize(unsigned long, char) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:647 #7 0x31f1b12 in resize /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/basic_string.h:749 #8 0x31f1b12 in base::Base64Encode(base::BasicStringPiece\u003Cstd::string> const&, std::string*) base/base64.cc:13 #9 0x34c73d5 in net::SdchManager::UrlSafeBase64Encode(std::string const&, std::string*) net/base/sdch_manager.cc:558 #10 0x34c67e7 in net::SdchManager::GenerateHash(std::string const&, std::string*, std::string*) net/base/sdch_manager.cc:524 #11 0x34c519b in net::SdchManager::AddSdchDictionary(std::string const&, GURL const&) net/base/sdch_manager.cc:380 #12 0x808536 in net::SdchManagerTest_ClearDictionaryData_Test::TestBody() net/base/sdch_manager_unittest.cc:454 #13 0x33173a8 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2045 #14 0x33173a8 in testing::Test::Run() testing/gtest/src/gtest.cc:2061 #15 0x3319b89 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2237 #16 0x331a916 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2344 #17 0x332da2a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4065 #18 0x332d060 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2045 #19 0x332d060 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:3697 #20 0x3fc8ddc in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2231 #21 0x3fc8ddc in base::TestSuite::Run() base/test/test_suite.cc:227 #22 0x3fbd5a2 in Run base/callback.h:401 #23 0x3fbd5a2 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, bool, base::Callback\u003Cvoid ()> const&) base/test/launcher/unit_test_launcher.cc:498 #24 0x3fbcf1d in base::LaunchUnitTests(int, char**, base::Callback\u003Cint ()> const&) base/test/launcher/unit_test_launcher.cc:553 #25 0x2ae277c in main net/test/run_all_unittests.cc:64 #26 0x7ff2703d176c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 SUMMARY: AddressSanitizer: 449 byte(s) leaked in 5 allocation(s)..
Jim: Could you please review PS10 to fix the memory leak that resulted in the revert of this CL?
https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.c... net/base/sdch_manager.cc:237: dictionaries_.clear(); I suspect this is the source of the leak. You are clearing the list of dictionaries, but not releasing the pointers one by one (as you would do in the destructor, on lines 220-223). With the new code patch set 10 code, this would almost work via the scoped_ptr... but then you'd have worse problems during map reallocations. https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.c... net/base/sdch_manager.cc:471: dictionaries_[server_hash] = scoped_refptr<Dictionary>(dictionary); This is actually a bad pattern. You typically can't put scoped_refptr<> into an STL container. See http://www.chromium.org/developers/smart-pointer-guidelines for a longer explanation. The existing code (manual AddRef and Release) would achieve what you wanted... so this could not be the cause. https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager_u... File net/base/sdch_manager_unittest.cc (right): https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager_u... net/base/sdch_manager_unittest.cc:466: sdch_manager()->ClearData(); This is probably the line of code that was critical to inducing (exposing) the leak in the reported revert stack. The dictionary created during the call in line 457 gets leaked.
On 2014/06/17 05:48:55, jar wrote: > https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.cc > File net/base/sdch_manager.cc (right): > > https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.c... > net/base/sdch_manager.cc:237: dictionaries_.clear(); > I suspect this is the source of the leak. You are clearing the list of > dictionaries, but not releasing the pointers one by one (as you would do in the > destructor, on lines 220-223). > > With the new code patch set 10 code, this would almost work via the > scoped_ptr... but then you'd have worse problems during map reallocations. > > https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.c... > net/base/sdch_manager.cc:471: dictionaries_[server_hash] = > scoped_refptr<Dictionary>(dictionary); > This is actually a bad pattern. You typically can't put scoped_refptr<> into an > STL container. See http://www.chromium.org/developers/smart-pointer-guidelines > for a longer explanation. > > The existing code (manual AddRef and Release) would achieve what you wanted... > so this could not be the cause. > > https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager_u... > File net/base/sdch_manager_unittest.cc (right): > > https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager_u... > net/base/sdch_manager_unittest.cc:466: sdch_manager()->ClearData(); > This is probably the line of code that was critical to inducing (exposing) the > leak in the reported revert stack. > > The dictionary created during the call in line 457 gets leaked. I believe that use of scoped_refptr<>, in contrast to other smart pointers, doesn't run into the problems with STL containers you refer to. See, for instance, https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/callba... (Ryan's comment). It's a large amount of adding and releasing references when they're moved around, but given that we don't add or remove dictionaries very often, I think it's the right idiom to guard against future leaks.
LGTM % nits https://codereview.chromium.org/321283002/diff/220001/chrome/browser/net/sdch... File chrome/browser/net/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/321283002/diff/220001/chrome/browser/net/sdch... chrome/browser/net/sdch_dictionary_fetcher.cc:55: while (!fetch_queue_.empty()) nit: If you're now counting on scoped_ref_ptr<>, wouldn't fetch_queue_.clear() be sufficient? https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.c... net/base/sdch_manager.cc:237: dictionaries_.clear(); (comment): Using the scoped_refptr<>.... this construct will be fine here, and resolve the leak. https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.c... net/base/sdch_manager.cc:471: dictionaries_[server_hash] = scoped_refptr<Dictionary>(dictionary); nit: If you're going to use scoped_refptr, there is no need for this cast, just do the assignment.... or better yet... since you don't need to manually AddRef, don't waste time with the temporary. ...but if you really like the temp... it is fine... just kill the cast.
Jim: Sorry, I expanded the use of scoped_refptr<> (if I'm going to use it to prevent accidentally causing future leaks, I should use it everywhere), so I need another round of review? https://codereview.chromium.org/321283002/diff/220001/chrome/browser/net/sdch... File chrome/browser/net/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/321283002/diff/220001/chrome/browser/net/sdch... chrome/browser/net/sdch_dictionary_fetcher.cc:55: while (!fetch_queue_.empty()) On 2014/06/17 17:03:27, jar wrote: > nit: If you're now counting on scoped_ref_ptr<>, wouldn't fetch_queue_.clear() > be sufficient? So http://www.cplusplus.com/reference/queue/queue/ doesn't show a clear() method. (And the swap() method is C++11 only, which is what prompted this change in the first place.) So this was the best I could come up with. https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.c... net/base/sdch_manager.cc:237: dictionaries_.clear(); On 2014/06/17 05:48:55, jar wrote: > I suspect this is the source of the leak. You are clearing the list of > dictionaries, but not releasing the pointers one by one (as you would do in the > destructor, on lines 220-223). > > With the new code patch set 10 code, this would almost work via the > scoped_ptr... but then you'd have worse problems during map reallocations. Done. https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.c... net/base/sdch_manager.cc:237: dictionaries_.clear(); On 2014/06/17 17:03:28, jar wrote: > (comment): Using the scoped_refptr<>.... this construct will be fine here, and > resolve the leak. Done. https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.c... net/base/sdch_manager.cc:471: dictionaries_[server_hash] = scoped_refptr<Dictionary>(dictionary); On 2014/06/17 05:48:55, jar wrote: > This is actually a bad pattern. You typically can't put scoped_refptr<> into an > STL container. See http://www.chromium.org/developers/smart-pointer-guidelines > for a longer explanation. > > The existing code (manual AddRef and Release) would achieve what you wanted... > so this could not be the cause. Done. https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager.c... net/base/sdch_manager.cc:471: dictionaries_[server_hash] = scoped_refptr<Dictionary>(dictionary); On 2014/06/17 17:03:28, jar wrote: > nit: If you're going to use scoped_refptr, there is no need for this cast, just > do the assignment.... or better yet... since you don't need to manually AddRef, > don't waste time with the temporary. > > ...but if you really like the temp... it is fine... just kill the cast. Really done (to distinguish from the other "Done"s which are just dismissing comments.) https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager_u... File net/base/sdch_manager_unittest.cc (right): https://codereview.chromium.org/321283002/diff/220001/net/base/sdch_manager_u... net/base/sdch_manager_unittest.cc:466: sdch_manager()->ClearData(); On 2014/06/17 05:48:55, jar wrote: > This is probably the line of code that was critical to inducing (exposing) the > leak in the reported revert stack. > > The dictionary created during the call in line 457 gets leaked. Done.
LGTM % nit https://codereview.chromium.org/321283002/diff/260001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/321283002/diff/260001/net/base/sdch_manager.c... net/base/sdch_manager.cc:477: const GURL& referring_url, scoped_refptr<Dictionary>* dictionary) { nit: one arg per line when you overflow in declation or definition.
Thanks! https://codereview.chromium.org/321283002/diff/260001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/321283002/diff/260001/net/base/sdch_manager.c... net/base/sdch_manager.cc:477: const GURL& referring_url, scoped_refptr<Dictionary>* dictionary) { On 2014/06/18 01:52:33, jar wrote: > nit: one arg per line when you overflow in declation or definition. Done.
The CQ bit was checked by rdsmith@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/321283002/280001
Message was sent while issue was closed.
Change committed as 278297 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
