Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(107)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 3076008: sync: change to demonstrate where syncable leak is. (Closed)

Created:
10 years, 5 months ago by Evan Martin
Modified:
9 years, 7 months ago
CC:
chromium-reviews, Raghu Simha, ncarter (slow), ben+cc_chromium.org, tim (not reviewing), idana
Base URL:
git://git.chromium.org/chromium.git
Visibility:
Public.

Description

sync: change to demonstrate where syncable leak is.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -1 line) Patch
M chrome/browser/sync/syncable/syncable.cc View 1 chunk +3 lines, -1 line 0 comments Download

Messages

Total messages: 5 (0 generated)
Evan Martin
i realized i have no way to test this at the moment, so i'd like ...
10 years, 5 months ago (2010-07-28 01:53:13 UTC) #1
ncarter (slow)
Looking now. On Tue, Jul 27, 2010 at 6:53 PM, <evan@chromium.org> wrote: > Reviewers: timsteele, ...
10 years, 5 months ago (2010-07-28 02:15:31 UTC) #2
ncarter (slow)
The right way to clean up is STLDeleteElements. Two options for how that cleanup might ...
10 years, 4 months ago (2010-07-28 03:41:44 UTC) #3
Evan Martin
It was hit on the valgrind bots, see the stack trace in the original mail? ...
10 years, 4 months ago (2010-07-28 17:21:08 UTC) #4
ncarter (slow)
10 years, 4 months ago (2010-07-28 17:36:04 UTC) #5 
I saw the stack trace, but a breakpoint on the failure point didn't trigger
when I ran through the whole of sync_unit_tests.  Will proceed with the fix,
unit test to follow.

 - nick

On Wed, Jul 28, 2010 at 10:20 AM, Evan Martin <evan@chromium.org> wrote:

> It was hit on the valgrind bots, see the stack trace in the original
> mail?  I'm not sure how they hit it though.
> I'm not seeing it on the bots now.
>
> On Tue, Jul 27, 2010 at 8:35 PM, Nick Carter <nick@chromium.org> wrote:
> > The right way to clean up is STLDeleteElements.  Two options for how that
> > cleanup might look are implemented
> > here: http://codereview.chromium.org/3026029 .  But before I go an write
> a
> > unit test that hits this case, how'd you hit this in the first case Evan?
> >  - nick
> >
> > On Tue, Jul 27, 2010 at 7:15 PM, Nick Carter <nick@chromium.org> wrote:
> >>
> >> Looking now.
> >>
> >> On Tue, Jul 27, 2010 at 6:53 PM, <evan@chromium.org> wrote:
> >>>
> >>> Reviewers: timsteele, ncarter,
> >>>
> >>> Message:
> >>> i realized i have no way to test this at the moment, so i'd like your
> >>> feedback
> >>> or perhaps for you to just fix this.
> >>>
> >>> Command: src/sconsbuild/Release/sync_unit_tests --gtest_print_time
> >>> 394 (200 direct, 194 indirect) bytes in 1 blocks are definitely lost in
> >>> loss
> >>> record 1,379 of 1,538
> >>>  operator new(unsigned int)
> >>>
> >>>
>
(sr/local/google/valgrind-for-chromium-client/valgrind/scripts/valgrind-memcheck/coregrind/m_replacemalloc/vg_replace_malloc.c:262)
> >>>  syncable::UnpackEntry(SQLStatement*, syncable::EntryKernel**)
> >>>
> >>>
> (uilder/build/src/chrome/browser/sync/syncable/directory_backing_store.cc:96)
> >>>
> >>>
>  syncable::DirectoryBackingStore::LoadEntries(std::set<syncable::EntryKernel*,
> >>> syncable::LessField<syncable::MetahandleField,
> >>> (syncable::MetahandleField)0>,
> >>> std::allocator<syncable::EntryKernel*> >*)
> >>>
> >>>
> (uilder/build/src/chrome/browser/sync/syncable/directory_backing_store.cc:468)
> >>>  syncable::DirectoryBackingStore::Load(std::set<syncable::EntryKernel*,
> >>> syncable::LessField<syncable::MetahandleField,
> >>> (syncable::MetahandleField)0>,
> >>> std::allocator<syncable::EntryKernel*> >*,
> >>> syncable::Directory::KernelLoadInfo*)
> >>>
> >>>
> (uilder/build/src/chrome/browser/sync/syncable/directory_backing_store.cc:226)
> >>>  syncable::Directory::OpenImpl(FilePath const&, std::basic_string<char,
> >>> std::char_traits<char>, std::allocator<char> > const&)
> >>> (uilder/build/src/chrome/browser/sync/syncable/syncable.cc:262)
> >>>  syncable::Directory::Open(FilePath const&, std::basic_string<char,
> >>> std::char_traits<char>, std::allocator<char> > const&)
> >>> (uilder/build/src/chrome/browser/sync/syncable/syncable.cc:222)
> >>>  syncable::(anonymous
> >>>
> >>>
>
namespace)::SyncableDirectoryTest_TestSaveChangesFailureWithPurge_Test::TestBody()
> >>>
> (uilder/build/src/chrome/browser/sync/syncable/syncable_unittest.cc:1227)
> >>>  testing::Test::Run()
> (uilder/build/src/testing/gtest/src/gtest.cc:2095)
> >>>  testing::internal::TestInfoImpl::Run()
> >>> (uilder/build/src/testing/gtest/src/gtest.cc:2314)
> >>>  testing::TestCase::Run()
> >>> (uilder/build/src/testing/gtest/src/gtest.cc:2420)
> >>>  testing::internal::UnitTestImpl::RunAllTests()
> >>> (uilder/build/src/testing/gtest/src/gtest.cc:4024)
> >>>  testing::UnitTest::Run()
> >>> (uilder/build/src/testing/gtest/src/gtest.cc:3687)
> >>>  TestSuite::Run() (uilder/build/src/./base/test/test_suite.h:151)
> >>>  main (uilder/build/src/chrome/test/unit/run_all_unittests.cc:8)
> >>>
> >>> Description:
> >>> sync: change to demonstrate where syncable leak is.
> >>>
> >>> Please review this at http://codereview.chromium.org/3076008/show
> >>>
> >>> SVN Base: git://git.chromium.org/chromium.git
> >>>
> >>> Affected files:
> >>>  M chrome/browser/sync/syncable/syncable.cc
> >>>
> >>>
> >>> Index: chrome/browser/sync/syncable/syncable.cc
> >>> diff --git a/chrome/browser/sync/syncable/syncable.cc
> >>> b/chrome/browser/sync/syncable/syncable.cc
> >>> index
> >>>
>
a2a9a98599b010e7d22b69e29ff1c8fd54a41fc4..38ddb45185724e4a57cb524ec95987d445dc77d5
> >>> 100644
> >>> --- a/chrome/browser/sync/syncable/syncable.cc
> >>> +++ b/chrome/browser/sync/syncable/syncable.cc
> >>> @@ -260,8 +260,10 @@ DirOpenResult Directory::OpenImpl(const FilePath&
> >>> file_path,
> >>>   // swap these later.
> >>>   MetahandlesIndex metas_bucket;
> >>>   DirOpenResult result = store_->Load(&metas_bucket, &info);
> >>> -  if (OPENED != result)
> >>> +  if (OPENED != result) {
> >>> +    // XXX need to free all the crap in metas_bucket here.
> >>>     return result;
> >>> +  }
> >>>
> >>>   kernel_ = new Kernel(db_path, name, info);
> >>>   kernel_->metahandles_index->swap(metas_bucket);
> >>>
> >>>
> >>
> >
> >
>

Powered by Google App Engine
This is Rietveld 408576698