Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
452723002:
Oilpan: blink_heap_unittests: Test case have memory leak checked by Lsan (Closed)
|
Created:
6 years, 4 months ago by Daniel Chow Modified:
6 years, 4 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionOilpan: blink_heap_unittests: Test case have memory leak checked by Lsan
1. For case TraceIfNeeded, modify TraceIfNeededTester to inherit from GarbageCollectedFinalized and at the end of test case TraceIfNeeded trigger a GC.
After GC, the destructor of TraceIfNeededTester will dereference the RefPtr<OffHeapInt> instance, stop the leak.
2. For case MapWithCustomWeaknessHandling and MapWithCustomWeaknessHandling2, need clear the Persistent handle and trigger a GC at the end of test case.
R=ager@chromium.org, oilpan-reviews@chromium.org
BUG=401018
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179938
Patch Set 1 #
Total comments: 3
Patch Set 2 : Trigger a GC before HeapTest main() exits #
Total comments: 3
Patch Set 3 : change collectGarbage(...) to collectAllGarbage() in HeapTest main() #
Messages
Total messages: 19 (0 generated)
Please Take A Look
https://codereview.chromium.org/452723002/diff/1/Source/platform/heap/HeapTes... File Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/452723002/diff/1/Source/platform/heap/HeapTes... Source/platform/heap/HeapTest.cpp:4157: livingInt.clear(); Hmm, I don't fully understand why we need to call clear() here. These persistent handles will be anyway destructed when the current scope exists. Can we probably move Heap::collectGarbage() to HeapTest::TearDown? It's a pain that if we have to explicitly clear all persistent handles and call Heap::collectGarabge() in each test. https://codereview.chromium.org/452723002/diff/1/Source/platform/heap/HeapTes... Source/platform/heap/HeapTest.cpp:4986: class TraceIfNeededTester : public GarbageCollectedFinalized<TraceIfNeededTester<T> > { This change looks correct. Ian: It would be great if the plugin can catch this error :)
https://codereview.chromium.org/452723002/diff/1/Source/platform/heap/HeapTes... File Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/452723002/diff/1/Source/platform/heap/HeapTes... Source/platform/heap/HeapTest.cpp:4157: livingInt.clear(); Hmm, I don't fully understand why we need to call clear() here. These persistent handles will be anyway destructed when the current scope exists. Can we probably move Heap::collectGarbage() to HeapTest::TearDown? It's a pain that if we have to explicitly clear all persistent handles and call Heap::collectGarabge() in each test. https://codereview.chromium.org/452723002/diff/1/Source/platform/heap/HeapTes... Source/platform/heap/HeapTest.cpp:4986: class TraceIfNeededTester : public GarbageCollectedFinalized<TraceIfNeededTester<T> > { This change looks correct. Ian: It would be great if the plugin can catch this error :)
https://codereview.chromium.org/452723002/diff/1/Source/platform/heap/HeapTes... File Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/452723002/diff/1/Source/platform/heap/HeapTes... Source/platform/heap/HeapTest.cpp:4157: livingInt.clear(); it's a good idea, I will make a try today On 2014/08/11 02:00:07, haraken wrote: > > Hmm, I don't fully understand why we need to call clear() here. These persistent > handles will be anyway destructed when the current scope exists. > > Can we probably move Heap::collectGarbage() to HeapTest::TearDown? It's a pain > that if we have to explicitly clear all persistent handles and call > Heap::collectGarabge() in each test.
Please Take Another Look.
LGTM https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... File Source/platform/heap/RunAllTests.cpp (right): https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... Source/platform/heap/RunAllTests.cpp:61: blink::Heap::collectGarbage(blink::ThreadState::NoHeapPointersOnStack); I'd prefer blink::Heap::collectAllGarbage(), (which calls Heap::collectGarbage() multiple times to ensure that everything is gone).
https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... File Source/platform/heap/RunAllTests.cpp (right): https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... Source/platform/heap/RunAllTests.cpp:61: blink::Heap::collectGarbage(blink::ThreadState::NoHeapPointersOnStack); On 2014/08/11 06:12:39, haraken wrote: > > I'd prefer blink::Heap::collectAllGarbage(), (which calls Heap::collectGarbage() > multiple times to ensure that everything is gone). Acknowledged. And there is another case crash that I did not make any change in this CL for it (I think it is not exactly a issue but it's a noise) : the test case CheckAndMarkPointer will trigger a ASSERT in ThreadState::copyStackUntilSafePointScope ASSERT(slotCount < 1024); // Catch potential performance issues.
On 2014/08/11 06:32:25, Daniel Chow wrote: > https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... > File Source/platform/heap/RunAllTests.cpp (right): > > https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... > Source/platform/heap/RunAllTests.cpp:61: > blink::Heap::collectGarbage(blink::ThreadState::NoHeapPointersOnStack); > On 2014/08/11 06:12:39, haraken wrote: > > > > I'd prefer blink::Heap::collectAllGarbage(), (which calls > Heap::collectGarbage() > > multiple times to ensure that everything is gone). > > Acknowledged. > > And there is another case crash that I did not make any change in this CL for it > (I think it is not exactly a issue but it's a noise) : > the test case CheckAndMarkPointer will trigger a ASSERT in > ThreadState::copyStackUntilSafePointScope > ASSERT(slotCount < 1024); // Catch potential performance issues. This looks like something unexpected is going on. In what test case are you facing the crash? (Of course you can address the issue in a separate CL.)
On 2014/08/11 06:34:29, haraken wrote: > On 2014/08/11 06:32:25, Daniel Chow wrote: > > > https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... > > File Source/platform/heap/RunAllTests.cpp (right): > > > > > https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... > > Source/platform/heap/RunAllTests.cpp:61: > > blink::Heap::collectGarbage(blink::ThreadState::NoHeapPointersOnStack); > > On 2014/08/11 06:12:39, haraken wrote: > > > > > > I'd prefer blink::Heap::collectAllGarbage(), (which calls > > Heap::collectGarbage() > > > multiple times to ensure that everything is gone). > > > > Acknowledged. > > > > And there is another case crash that I did not make any change in this CL for > it > > (I think it is not exactly a issue but it's a noise) : > > the test case CheckAndMarkPointer will trigger a ASSERT in > > ThreadState::copyStackUntilSafePointScope > > ASSERT(slotCount < 1024); // Catch potential performance issues. > > This looks like something unexpected is going on. In what test case are you > facing the crash? (Of course you can address the issue in a separate CL.) in TEST(HeapTest, CheckAndMarkPointer), for the first TestGCScope, slotCount is 983.
On 2014/08/11 07:09:05, Daniel Chow wrote: > On 2014/08/11 06:34:29, haraken wrote: > > On 2014/08/11 06:32:25, Daniel Chow wrote: > > > > > > https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... > > > File Source/platform/heap/RunAllTests.cpp (right): > > > > > > > > > https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... > > > Source/platform/heap/RunAllTests.cpp:61: > > > blink::Heap::collectGarbage(blink::ThreadState::NoHeapPointersOnStack); > > > On 2014/08/11 06:12:39, haraken wrote: > > > > > > > > I'd prefer blink::Heap::collectAllGarbage(), (which calls > > > Heap::collectGarbage() > > > > multiple times to ensure that everything is gone). > > > > > > Acknowledged. > > > > > > And there is another case crash that I did not make any change in this CL > for > > it > > > (I think it is not exactly a issue but it's a noise) : > > > the test case CheckAndMarkPointer will trigger a ASSERT in > > > ThreadState::copyStackUntilSafePointScope > > > ASSERT(slotCount < 1024); // Catch potential performance issues. > > > > This looks like something unexpected is going on. In what test case are you > > facing the crash? (Of course you can address the issue in a separate CL.) > > in TEST(HeapTest, CheckAndMarkPointer), for the first TestGCScope, slotCount is > 983. Just to confirm: The crash does not happen without your CL but happens with your CL? slotCount=983 seems too big and implies something wrong is going on, but 983 is less than 1024. Why do you hit the crash?
On 2014/08/11 07:12:56, haraken wrote: > On 2014/08/11 07:09:05, Daniel Chow wrote: > > On 2014/08/11 06:34:29, haraken wrote: > > > On 2014/08/11 06:32:25, Daniel Chow wrote: > > > > > > > > > > https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... > > > > File Source/platform/heap/RunAllTests.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... > > > > Source/platform/heap/RunAllTests.cpp:61: > > > > blink::Heap::collectGarbage(blink::ThreadState::NoHeapPointersOnStack); > > > > On 2014/08/11 06:12:39, haraken wrote: > > > > > > > > > > I'd prefer blink::Heap::collectAllGarbage(), (which calls > > > > Heap::collectGarbage() > > > > > multiple times to ensure that everything is gone). > > > > > > > > Acknowledged. > > > > > > > > And there is another case crash that I did not make any change in this CL > > for > > > it > > > > (I think it is not exactly a issue but it's a noise) : > > > > the test case CheckAndMarkPointer will trigger a ASSERT in > > > > ThreadState::copyStackUntilSafePointScope > > > > ASSERT(slotCount < 1024); // Catch potential performance issues. > > > > > > This looks like something unexpected is going on. In what test case are you > > > facing the crash? (Of course you can address the issue in a separate CL.) > > > > in TEST(HeapTest, CheckAndMarkPointer), for the first TestGCScope, slotCount > is > > 983. > > Just to confirm: The crash does not happen without your CL but happens with your > CL? > > slotCount=983 seems too big and implies something wrong is going on, but 983 is > less than 1024. Why do you hit the crash? Oh, I am sorry to confuse you with that replay. I am trying to make the Heap_Test clean with Lsan. and there are 3 memory leaks and a crash. this CL have deal with the 3 memory leaks. and have not fix the crash of TEST(HeapTest, CheckAndMarkPointer). I will keep investigate the crash reason after this CL.
On 2014/08/11 07:37:00, Daniel Chow wrote: > On 2014/08/11 07:12:56, haraken wrote: > > On 2014/08/11 07:09:05, Daniel Chow wrote: > > > On 2014/08/11 06:34:29, haraken wrote: > > > > On 2014/08/11 06:32:25, Daniel Chow wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... > > > > > File Source/platform/heap/RunAllTests.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Run... > > > > > Source/platform/heap/RunAllTests.cpp:61: > > > > > blink::Heap::collectGarbage(blink::ThreadState::NoHeapPointersOnStack); > > > > > On 2014/08/11 06:12:39, haraken wrote: > > > > > > > > > > > > I'd prefer blink::Heap::collectAllGarbage(), (which calls > > > > > Heap::collectGarbage() > > > > > > multiple times to ensure that everything is gone). > > > > > > > > > > Acknowledged. > > > > > > > > > > And there is another case crash that I did not make any change in this > CL > > > for > > > > it > > > > > (I think it is not exactly a issue but it's a noise) : > > > > > the test case CheckAndMarkPointer will trigger a ASSERT in > > > > > ThreadState::copyStackUntilSafePointScope > > > > > ASSERT(slotCount < 1024); // Catch potential performance issues. > > > > > > > > This looks like something unexpected is going on. In what test case are > you > > > > facing the crash? (Of course you can address the issue in a separate CL.) > > > > > > in TEST(HeapTest, CheckAndMarkPointer), for the first TestGCScope, slotCount > > is > > > 983. > > > > Just to confirm: The crash does not happen without your CL but happens with > your > > CL? > > > > slotCount=983 seems too big and implies something wrong is going on, but 983 > is > > less than 1024. Why do you hit the crash? > > Oh, I am sorry to confuse you with that replay. I am trying to make the > Heap_Test clean with Lsan. and there are 3 memory leaks and a crash. > this CL have deal with the 3 memory leaks. and have not fix the crash of > TEST(HeapTest, CheckAndMarkPointer). > I will keep investigate the crash reason after this CL. Thanks for the clarification. Blink prefers addressing one-issue-per-CL and making changes as incrementally as possible. So you can commit this CL and address the slotCount issue in a separate CL.
lgtm https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Hea... File Source/platform/heap/HeapTest.cpp (right): https://codereview.chromium.org/452723002/diff/20001/Source/platform/heap/Hea... Source/platform/heap/HeapTest.cpp:4978: class TraceIfNeededTester : public GarbageCollectedFinalized<TraceIfNeededTester<T> > { Thanks! I indeed forgot the finalization requirement when T is instantiated with a RefPtr.
Please Take Another Look: I have changed collectGarbage(...) to collectAllGarbage() in HeapTest main()
Still LGTM
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhishun.zhou@samsung.com/452723002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
Message was sent while issue was closed.
Change committed as 179938 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
