Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1632763003:
Fix leaks in exensions_unittests after enabling Oilpan (Closed)
|
Created:
4 years, 11 months ago by haraken Modified:
4 years, 11 months ago CC:
bruening+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, glider+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix leaks in exensions_unittests after enabling Oilpan
This CL adds an Oilpan's GC before tearing down the extensions_unittests.
BUG=581092
Committed: https://crrev.com/6ae6611de742bb291305a1df558e710213556fa4
Cr-Commit-Position: refs/heads/master@{#371408}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 27 (7 generated)
haraken@chromium.org changed reviewers: + oilpan-reviews@chromium.org, oshima@chromium.org
PTAL This is a speculative fix. What bots should I kick off to confirm that this CL fixes the leak?
lgtm
Let me land this as a speculative fix.
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1632763003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1632763003/1
haraken@chromium.org changed reviewers: + rockot@chromium.org
rockot: Would you take a look as an OWNER?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1632763003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1632763003/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix leaks in exensions_unittests after enabling Oilpan This CL adds an Oilpan's GC before tearing down the extensions_unittests. BUG=581092 ========== to ========== Fix leaks in exensions_unittests after enabling Oilpan This CL adds an Oilpan's GC before tearing down the extensions_unittests. BUG=581092 Committed: https://crrev.com/6ae6611de742bb291305a1df558e710213556fa4 Cr-Commit-Position: refs/heads/master@{#371408} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/6ae6611de742bb291305a1df558e710213556fa4 Cr-Commit-Position: refs/heads/master@{#371408}
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... File extensions/renderer/module_system_test.cc (right): https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... extensions/renderer/module_system_test.cc:244: blink::WebHeap::collectAllGarbageForTesting(); The v8 major GC will trigger Oilpan GCs also, so unclear if this extra round will make a difference.
Message was sent while issue was closed.
https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... File extensions/renderer/module_system_test.cc (right): https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... extensions/renderer/module_system_test.cc:244: blink::WebHeap::collectAllGarbageForTesting(); On 2016/01/26 06:09:43, sof wrote: > The v8 major GC will trigger Oilpan GCs also, so unclear if this extra round > will make a difference. https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20(va... shows that it didn't. Should we force a GC with collectGarbageForTesting() instead? (I cannot reproduce these leaks locally with memcheck, btw.)
Message was sent while issue was closed.
https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... File extensions/renderer/module_system_test.cc (right): https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... extensions/renderer/module_system_test.cc:244: blink::WebHeap::collectAllGarbageForTesting(); On 2016/01/26 08:05:44, sof wrote: > On 2016/01/26 06:09:43, sof wrote: > > The v8 major GC will trigger Oilpan GCs also, so unclear if this extra round > > will make a difference. > > https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20(va... > shows that it didn't. > > Should we force a GC with collectGarbageForTesting() instead? (I cannot > reproduce these leaks locally with memcheck, btw.) Doesn't collectAllGarbage always collect more objects than collectGarbage?
Message was sent while issue was closed.
On 2016/01/26 08:26:40, haraken wrote: > https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... > File extensions/renderer/module_system_test.cc (right): > > https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... > extensions/renderer/module_system_test.cc:244: > blink::WebHeap::collectAllGarbageForTesting(); > On 2016/01/26 08:05:44, sof wrote: > > On 2016/01/26 06:09:43, sof wrote: > > > The v8 major GC will trigger Oilpan GCs also, so unclear if this extra round > > > will make a difference. > > > > > https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20(va... > > shows that it didn't. > > > > Should we force a GC with collectGarbageForTesting() instead? (I cannot > > reproduce these leaks locally with memcheck, btw.) > > Doesn't collectAllGarbage always collect more objects than collectGarbage? Quite right :) Ah, now i understand the leak -- static Persistent singletons will not be released on shutdown. We only do that if LEAK_SANITIZER is in effect.
Message was sent while issue was closed.
On 2016/01/26 12:25:13, sof wrote: > On 2016/01/26 08:26:40, haraken wrote: > > > https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... > > File extensions/renderer/module_system_test.cc (right): > > > > > https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... > > extensions/renderer/module_system_test.cc:244: > > blink::WebHeap::collectAllGarbageForTesting(); > > On 2016/01/26 08:05:44, sof wrote: > > > On 2016/01/26 06:09:43, sof wrote: > > > > The v8 major GC will trigger Oilpan GCs also, so unclear if this extra > round > > > > will make a difference. > > > > > > > > > https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20(va... > > > shows that it didn't. > > > > > > Should we force a GC with collectGarbageForTesting() instead? (I cannot > > > reproduce these leaks locally with memcheck, btw.) > > > > Doesn't collectAllGarbage always collect more objects than collectGarbage? > > Quite right :) > > Ah, now i understand the leak -- static Persistent singletons will not be > released on shutdown. We only do that if LEAK_SANITIZER is in effect. The leaking test seems to be GCCallbackTest.GCBeforeContextInvalidated, which isn't covered by the change here. (Not that it makes much of a leaking difference if I "port" it to GCCallbackTest::RequestGarbageCollection().)
Message was sent while issue was closed.
On 2016/01/26 12:25:13, sof wrote: > On 2016/01/26 08:26:40, haraken wrote: > > > https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... > > File extensions/renderer/module_system_test.cc (right): > > > > > https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... > > extensions/renderer/module_system_test.cc:244: > > blink::WebHeap::collectAllGarbageForTesting(); > > On 2016/01/26 08:05:44, sof wrote: > > > On 2016/01/26 06:09:43, sof wrote: > > > > The v8 major GC will trigger Oilpan GCs also, so unclear if this extra > round > > > > will make a difference. > > > > > > > > > https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20(va... > > > shows that it didn't. > > > > > > Should we force a GC with collectGarbageForTesting() instead? (I cannot > > > reproduce these leaks locally with memcheck, btw.) > > > > Doesn't collectAllGarbage always collect more objects than collectGarbage? > > Quite right :) > > Ah, now i understand the leak -- static Persistent singletons will not be > released on shutdown. We only do that if LEAK_SANITIZER is in effect. So do we need to clean up the static persistents when valgrind is enabled as well? (I'm not sure if there is a way to detect if valgrind is running or not.)
Message was sent while issue was closed.
On 2016/01/26 12:33:52, haraken wrote: > On 2016/01/26 12:25:13, sof wrote: > > On 2016/01/26 08:26:40, haraken wrote: > > > > > > https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... > > > File extensions/renderer/module_system_test.cc (right): > > > > > > > > > https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... > > > extensions/renderer/module_system_test.cc:244: > > > blink::WebHeap::collectAllGarbageForTesting(); > > > On 2016/01/26 08:05:44, sof wrote: > > > > On 2016/01/26 06:09:43, sof wrote: > > > > > The v8 major GC will trigger Oilpan GCs also, so unclear if this extra > > round > > > > > will make a difference. > > > > > > > > > > > > > > https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20(va... > > > > shows that it didn't. > > > > > > > > Should we force a GC with collectGarbageForTesting() instead? (I cannot > > > > reproduce these leaks locally with memcheck, btw.) > > > > > > Doesn't collectAllGarbage always collect more objects than collectGarbage? > > > > Quite right :) > > > > Ah, now i understand the leak -- static Persistent singletons will not be > > released on shutdown. We only do that if LEAK_SANITIZER is in effect. > > So do we need to clean up the static persistents when valgrind is enabled as > well? (I'm not sure if there is a way to detect if valgrind is running or not.) There is; it would mean we have to expose the code for doing the static persistent registration always, but conditionally do so. What does valgrind leak detection give us over LSan to merit this? Do we really want to do that (it is possible)?
Message was sent while issue was closed.
On 2016/01/26 12:41:04, sof wrote: > On 2016/01/26 12:33:52, haraken wrote: > > On 2016/01/26 12:25:13, sof wrote: > > > On 2016/01/26 08:26:40, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... > > > > File extensions/renderer/module_system_test.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... > > > > extensions/renderer/module_system_test.cc:244: > > > > blink::WebHeap::collectAllGarbageForTesting(); > > > > On 2016/01/26 08:05:44, sof wrote: > > > > > On 2016/01/26 06:09:43, sof wrote: > > > > > > The v8 major GC will trigger Oilpan GCs also, so unclear if this extra > > > round > > > > > > will make a difference. > > > > > > > > > > > > > > > > > > > > https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20(va... > > > > > shows that it didn't. > > > > > > > > > > Should we force a GC with collectGarbageForTesting() instead? (I cannot > > > > > reproduce these leaks locally with memcheck, btw.) > > > > > > > > Doesn't collectAllGarbage always collect more objects than collectGarbage? > > > > > > Quite right :) > > > > > > Ah, now i understand the leak -- static Persistent singletons will not be > > > released on shutdown. We only do that if LEAK_SANITIZER is in effect. > > > > So do we need to clean up the static persistents when valgrind is enabled as > > well? (I'm not sure if there is a way to detect if valgrind is running or > not.) > > There is; it would mean we have to expose the code for doing the static > persistent registration always, but conditionally do so. > > What does valgrind leak detection give us over LSan to merit this? > Do we really want to do that (it is possible)? Ignore last trailing sentence.. but i do wonder if we should explore doing one (?) suppression here instead first.
Message was sent while issue was closed.
On 2016/01/26 12:44:52, sof wrote: > On 2016/01/26 12:41:04, sof wrote: > > On 2016/01/26 12:33:52, haraken wrote: > > > On 2016/01/26 12:25:13, sof wrote: > > > > On 2016/01/26 08:26:40, haraken wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... > > > > > File extensions/renderer/module_system_test.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1632763003/diff/1/extensions/renderer/module_... > > > > > extensions/renderer/module_system_test.cc:244: > > > > > blink::WebHeap::collectAllGarbageForTesting(); > > > > > On 2016/01/26 08:05:44, sof wrote: > > > > > > On 2016/01/26 06:09:43, sof wrote: > > > > > > > The v8 major GC will trigger Oilpan GCs also, so unclear if this > extra > > > > round > > > > > > > will make a difference. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20(va... > > > > > > shows that it didn't. > > > > > > > > > > > > Should we force a GC with collectGarbageForTesting() instead? (I > cannot > > > > > > reproduce these leaks locally with memcheck, btw.) > > > > > > > > > > Doesn't collectAllGarbage always collect more objects than > collectGarbage? > > > > > > > > Quite right :) > > > > > > > > Ah, now i understand the leak -- static Persistent singletons will not be > > > > released on shutdown. We only do that if LEAK_SANITIZER is in effect. > > > > > > So do we need to clean up the static persistents when valgrind is enabled as > > > well? (I'm not sure if there is a way to detect if valgrind is running or > > not.) > > > > There is; it would mean we have to expose the code for doing the static > > persistent registration always, but conditionally do so. > > > > What does valgrind leak detection give us over LSan to merit this? > > Do we really want to do that (it is possible)? > > Ignore last trailing sentence.. but i do wonder if we should explore doing one > (?) suppression here instead first. Yeah, agreed. At the moment, suppressing the leak sounds like a reasonable solution here. I'll revert this CL.
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/1637013002/ by haraken@chromium.org. The reason for reverting is: This CL didn't fix the leak. Also per the discussion in https://codereview.chromium.org/1632763003/, suppressing the leak looks like a reasonable solution at the moment. So revert this CL. . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
