Summary: | Ignore false-positive leaks under bmalloc::Heap::Heap | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Component: | Tools / Tests | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Status: | RESOLVED CONFIGURATION CHANGED | ||||||
Severity: | Normal | CC: | ews-watchlist, fpizlo, ggaren, glenn, joepeck, lforschler, msaboff, simon.fraser, thorton, webkit-bug-importer, ysuzuki | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Local Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2018-12-07 10:33:56 PST
Created attachment 356821 [details]
Patch v1
What exactly is the tools bug here? run-webkit-tests --leaks sets the MallocStackLogging environment variable, which should make bmalloc disable any functionality that's still incompatible with the leaks tool. So I think that this is a bmalloc bug. Comment on attachment 356821 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=356821&action=review > Tools/Scripts/webkitpy/port/leakdetector.py:63 > + 'bmalloc::Heap::Heap', # <rdar://problem/40305994> As just commented, I don't see how this is the right approach. (In reply to Alexey Proskuryakov from comment #3) > What exactly is the tools bug here? run-webkit-tests --leaks sets the > MallocStackLogging environment variable, which should make bmalloc disable > any functionality that's still incompatible with the leaks tool. This is fixed in a future version of the leaks tool by <rdar://problem/40305994>. The bug apparently had to do with the way either fastMalloc or bmalloc tagged its allocated memory. > So I think that this is a bmalloc bug. This change is to ignore these false-positive leaks until the new leaks tool (with the fix) is widely available as "bmalloc::Heap::Heap" will ignore both of these stacks, which are allocating memory using malloc (see stack frame 0): STACK OF 1 INSTANCE OF 'ROOT LEAK: <OS_dispatch_source>': [thread 0x16dd67000]: [...] 7 JavaScriptCore 0x1af290594 bmalloc::Heap::Heap(bmalloc::HeapKind, std::__1::lock_guard<bmalloc::Mutex>&) + 884 6 JavaScriptCore 0x1af28cdec bmalloc::PerProcess<bmalloc::Scavenger>::getSlowCase() + 140 5 JavaScriptCore 0x1af29632c bmalloc::Scavenger::Scavenger(std::__1::lock_guard<bmalloc::Mutex>&) + 212 4 libdispatch.dylib 0x1a6fbfe70 dispatch_source_create$VARIANT$armv81 + 56 3 libdispatch.dylib 0x1a6f79880 _os_object_alloc_realized + 40 2 libobjc.A.dylib 0x1a684eb48 class_createInstance + 72 1 libsystem_malloc.dylib 0x1a716c708 calloc + 40 0 libsystem_malloc.dylib 0x1a716c7e4 malloc_zone_calloc + 184 ==== 1 (128 bytes) ROOT LEAK: <OS_dispatch_source 0x11ff74740> [128] STACK OF 1 INSTANCE OF 'ROOT LEAK: <std::__1::__shared_ptr_emplace<std::__1::mutex, std::__1::allocator<std::__1::mutex> >>': [thread 0x16dd67000]: [...] 3 JavaScriptCore 0x1af290434 bmalloc::Heap::Heap(bmalloc::HeapKind, std::__1::lock_guard<bmalloc::Mutex>&) + 532 2 libc++abi.dylib 0x1a68236d4 operator new(unsigned long) + 44 1 libsystem_malloc.dylib 0x1a716c8a0 malloc + 32 0 libsystem_malloc.dylib 0x1a716b4e8 malloc_zone_malloc + 200 ==== 1 (96 bytes) ROOT LEAK: <std::__1::__shared_ptr_emplace<std::__1::mutex, std::__1::allocator<std::__1::mutex> > 0x11fd5e620> [96] Or are you saying that the fix for <rdar://problem/40305994> was the wrong way to fix this, and the change should be in bmalloc instead to disable the bmalloc code and to call malloc directly from WTF::fastZeroedMalloc() (since the Scavenger isn't even used when malloc is used to allocate memory)? (In reply to Alexey Proskuryakov from comment #3) > What exactly is the tools bug here? run-webkit-tests --leaks sets the > MallocStackLogging environment variable, which should make bmalloc disable > any functionality that's still incompatible with the leaks tool. > > So I think that this is a bmalloc bug. I did some testing: - Enabling USE_SYSTEM_MALLOC at build time makes this go away since these objects are never created. However, that requires a separate build and some local hacks to make it compile, and it's even further from the configuration that customers use because bmalloc is not used at all. (Maybe using system malloc instead of bmalloc is far enough from a customer configuration that this doesn't matter, though?) - These false-positive leaks only affect bmalloc itself, not the memory bmalloc allocates on behalf of other code, so this change will only ignore these 4 leaks (there are 3 instances of the mutex mentioned in Comment #0) in each com.apple.WebKit.* process and the UI process. - There may not be another chokepoint for bmalloc vs. system malloc that both (a) doesn't impact customer performance, and (b) eliminates these allocations. However, doing nothing means updated tools will eventually stop reporting these leaks anyway, so let's just move this bug to config changed since it will now never land, and it only affects a person or a bot running leaks with modern WebKit processes (which isn't supported with `run-webkit-tests --leaks` yet). I also wonder if r241789 for Bug 194811 improved this prior to the latest dev tools. |