Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
308713007:
[Memory Sheriff] Force a well-defined path for system leak. (Closed)
|
Created:
6 years, 6 months ago by groby-ooo-7-16 Modified:
6 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@mem_257276 Visibility:
Public. |
Description[Memory Sheriff] Force a well-defined path for system leak.
BUG=379331
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276609
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address review comments. #
Total comments: 1
Messages
Total messages: 13 (0 generated)
Scott: That's the CL I was talking about. Nico: OWNERS review, please? Background: [NSCursor push] leaks the first time it's called, see bug. This forces the leak onto a well defined path so we can stop suppressing all invocations for [NSCursor set], and only suppress this path. (Push seems to be inlined, it's never on the callstack) The plan is to: 1) Land this. 2) Remove existing suppressions for NSCursor. Memory.FYI will deliver a callstack. (I'm currently mem sheriff, so yes, the sheriff knows ;) 3) File new suppression for just this path.
LGTM. There are probably NSFont and NSColor cases which could go here later.
It's a fairly trivial fix, and Nico is traveling - TBR OK?
Page him. This is important stuff. LGTM. Your call as to whether it should be at the top or bottom. It's pretty unlikely that anyone will ever successfully add anything to this code in the future. https://codereview.chromium.org/308713007/diff/1/ui/gfx/test/ui_cocoa_test_he... File ui/gfx/test/ui_cocoa_test_helper.mm (right): https://codereview.chromium.org/308713007/diff/1/ui/gfx/test/ui_cocoa_test_he... ui/gfx/test/ui_cocoa_test_helper.mm:68: Init(); I'm looking at this and thinking "Wait, what?" I bet I wrote it, too. https://codereview.chromium.org/308713007/diff/1/ui/gfx/test/ui_cocoa_test_he... ui/gfx/test/ui_cocoa_test_helper.mm:94: ForceSystemLeaks(); Stepping back and putting on my "What would another reviewer say?" cap, I find myself wondering if this shouldn't be at the top of the function?
On 2014/05/30 23:58:06, shess wrote: > Page him. This is important stuff. > > LGTM. Your call as to whether it should be at the top or bottom. It's pretty > unlikely that anyone will ever successfully add anything to this code in the > future. > > https://codereview.chromium.org/308713007/diff/1/ui/gfx/test/ui_cocoa_test_he... > File ui/gfx/test/ui_cocoa_test_helper.mm (right): > > https://codereview.chromium.org/308713007/diff/1/ui/gfx/test/ui_cocoa_test_he... > ui/gfx/test/ui_cocoa_test_helper.mm:68: Init(); > I'm looking at this and thinking "Wait, what?" I bet I wrote it, too. > > https://codereview.chromium.org/308713007/diff/1/ui/gfx/test/ui_cocoa_test_he... > ui/gfx/test/ui_cocoa_test_helper.mm:94: ForceSystemLeaks(); > Stepping back and putting on my "What would another reviewer say?" cap, I find > myself wondering if this shouldn't be at the top of the function? Um, TBR LGTM, I mean.
Yeah, I'm not paging him - it's not exactly life or death :) Calendar says he's mostly experiencing timezone shift, so I'll wait a moment or three. https://codereview.chromium.org/308713007/diff/1/ui/gfx/test/ui_cocoa_test_he... File ui/gfx/test/ui_cocoa_test_helper.mm (right): https://codereview.chromium.org/308713007/diff/1/ui/gfx/test/ui_cocoa_test_he... ui/gfx/test/ui_cocoa_test_helper.mm:68: Init(); On 2014/05/30 23:58:06, shess wrote: > I'm looking at this and thinking "Wait, what?" I bet I wrote it, too. Nope, Avi :) This used to call BootstrapCocoa, too, but that was moved into the Chrome-specific CocoaTest. (It's just setting the bundle path) https://codereview.chromium.org/308713007/diff/1/ui/gfx/test/ui_cocoa_test_he... ui/gfx/test/ui_cocoa_test_helper.mm:94: ForceSystemLeaks(); On 2014/05/30 23:58:06, shess wrote: > Stepping back and putting on my "What would another reviewer say?" cap, I find > myself wondering if this shouldn't be at the top of the function? Moved to top of ctor.
https://codereview.chromium.org/308713007/diff/20001/ui/gfx/test/ui_cocoa_tes... File ui/gfx/test/ui_cocoa_test_helper.mm (right): https://codereview.chromium.org/308713007/diff/20001/ui/gfx/test/ui_cocoa_tes... ui/gfx/test/ui_cocoa_test_helper.mm:14: // Some AppKit function leak intentionally, e.g. for caching purposes. Does anything actually report these leaks? I would've thought that since they're reachable from statics, they shouldn't be reported? (Well, at least the cursor one)
Yes, see bug #257276
On 2014/06/04 22:30:10, groby wrote: > Yes, see bug #257276 In this case: a) good idea, lgtm :-) b) add to BUG= line?
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/308713007/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/19006)
Message was sent while issue was closed.
Change committed as 276609 |
