Bug 192502 - Ignore false-positive leaks under bmalloc::Heap::Heap
Summary: Ignore false-positive leaks under bmalloc::Heap::Heap
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-07 10:33 PST by David Kilzer (:ddkilzer)
Modified: 2019-02-19 19:04 PST (History)
11 users (show)

See Also:


Attachments
Patch v1 (1.39 KB, patch)
2018-12-07 10:42 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2018-12-07 10:33:56 PST
We should ignore false-positive leaks under bmalloc::Heap::Heap.

Eventually the `leaks` tool will handle this, but until then we need to skip these leaks.

STACK OF 1 INSTANCE OF 'ROOT LEAK: <OS_dispatch_source>':
[thread 0x16dd67000]:
29  libsystem_pthread.dylib               0x1a71ac068 start_wqthread + 4
28  libsystem_pthread.dylib               0x1a71ac24c _pthread_wqthread + 472
27  libdispatch.dylib                     0x1a6fbdd54 _dispatch_worker_thread2 + 116
26  libdispatch.dylib                     0x1a6fbd4f8 _dispatch_root_queue_drain + 344
25  libdispatch.dylib                     0x1a6fb13c0 _dispatch_queue_override_invoke + 364
24  libdispatch.dylib                     0x1a6fb63e8 _dispatch_lane_invoke$VARIANT$armv81 + 536
23  libdispatch.dylib                     0x1a6fbc4c0 _dispatch_lane_concurrent_drain + 856
22  libdispatch.dylib                     0x1a6f79808 _dispatch_client_callout + 16
21  libdispatch.dylib                     0x1a6f79848 _dispatch_call_block_and_release + 24
20  com.apple.Safari.Shared               0x1ba630abc -[WBSSiteMetadataImageCache _internalSetUpImageCache] + 468
19  com.apple.Safari.Shared               0x1ba631774 -[WBSSiteMetadataImageCache _dispatchDiskAccessBlock:] + 96
18  com.apple.Safari.Shared               0x1ba51635c SafariShared::SuddenTerminationDisabler::SuddenTerminationDisabler(NSString*) + 64
17  com.apple.Safari.Shared               0x1ba4fd5c8 SafariShared::SuddenTerminationDisabler::disableSuddenTermination() + 92
16  libdispatch.dylib                     0x1a6fbb154 _dispatch_lane_barrier_sync_invoke_and_complete + 56
15  libdispatch.dylib                     0x1a6f79808 _dispatch_client_callout + 16
14  com.apple.Safari.Shared               0x1ba4fd70c invocation function for block in SafariShared::SuddenTerminationDisabler::disableSuddenTermination() + 236
13  com.apple.Safari.Shared               0x1ba4fdc88 WTF::HashTable<SafariShared::SuddenTerminationDisabler*, SafariShared::SuddenTerminationDisabler*, WTF::IdentityExtractor, WTF::PtrHash<SafariShared::SuddenTerminationDisabler*>, WTF::HashTraits<SafariShared::SuddenTerminationDisabler*>, WTF::HashTraits<SafariShared::SuddenTerminationDisabler*> >::add(SafariShared::SuddenTerminationDisabler*&&) + 88
12  com.apple.Safari.Shared               0x1ba4fde14 WTF::HashTable<SafariShared::SuddenTerminationDisabler*, SafariShared::SuddenTerminationDisabler*, WTF::IdentityExtractor, WTF::PtrHash<SafariShared::SuddenTerminationDisabler*>, WTF::HashTraits<SafariShared::SuddenTerminationDisabler*>, WTF::HashTraits<SafariShared::SuddenTerminationDisabler*> >::rehash(unsigned int, SafariShared::SuddenTerminationDisabler**) + 56
11  JavaScriptCore                        0x1af1202b4 WTF::fastZeroedMalloc(unsigned long) + 124
10  JavaScriptCore                        0x1af28d688 bmalloc::Cache::allocateSlowCaseNullCache(bmalloc::HeapKind, unsigned long) + 112
9   JavaScriptCore                        0x1af28da5c bmalloc::PerHeapKindBase<bmalloc::Cache>::PerHeapKindBase<>() + 44
8   JavaScriptCore                        0x1af28d4b0 bmalloc::PerProcess<bmalloc::PerHeapKind<bmalloc::Heap> >::getSlowCase() + 180
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]:
25  libsystem_pthread.dylib               0x1a71ac068 start_wqthread + 4
24  libsystem_pthread.dylib               0x1a71ac24c _pthread_wqthread + 472
23  libdispatch.dylib                     0x1a6fbdd54 _dispatch_worker_thread2 + 116
22  libdispatch.dylib                     0x1a6fbd4f8 _dispatch_root_queue_drain + 344
21  libdispatch.dylib                     0x1a6fb13c0 _dispatch_queue_override_invoke + 364
20  libdispatch.dylib                     0x1a6fb63e8 _dispatch_lane_invoke$VARIANT$armv81 + 536
19  libdispatch.dylib                     0x1a6fbc4c0 _dispatch_lane_concurrent_drain + 856
18  libdispatch.dylib                     0x1a6f79808 _dispatch_client_callout + 16
17  libdispatch.dylib                     0x1a6f79848 _dispatch_call_block_and_release + 24
16  com.apple.Safari.Shared               0x1ba630abc -[WBSSiteMetadataImageCache _internalSetUpImageCache] + 468
15  com.apple.Safari.Shared               0x1ba631774 -[WBSSiteMetadataImageCache _dispatchDiskAccessBlock:] + 96
14  com.apple.Safari.Shared               0x1ba51635c SafariShared::SuddenTerminationDisabler::SuddenTerminationDisabler(NSString*) + 64
13  com.apple.Safari.Shared               0x1ba4fd5c8 SafariShared::SuddenTerminationDisabler::disableSuddenTermination() + 92
12  libdispatch.dylib                     0x1a6fbb154 _dispatch_lane_barrier_sync_invoke_and_complete + 56
11  libdispatch.dylib                     0x1a6f79808 _dispatch_client_callout + 16
10  com.apple.Safari.Shared               0x1ba4fd70c invocation function for block in SafariShared::SuddenTerminationDisabler::disableSuddenTermination() + 236
9   com.apple.Safari.Shared               0x1ba4fdc88 WTF::HashTable<SafariShared::SuddenTerminationDisabler*, SafariShared::SuddenTerminationDisabler*, WTF::IdentityExtractor, WTF::PtrHash<SafariShared::SuddenTerminationDisabler*>, WTF::HashTraits<SafariShared::SuddenTerminationDisabler*>, WTF::HashTraits<SafariShared::SuddenTerminationDisabler*> >::add(SafariShared::SuddenTerminationDisabler*&&) + 88
8   com.apple.Safari.Shared               0x1ba4fde14 WTF::HashTable<SafariShared::SuddenTerminationDisabler*, SafariShared::SuddenTerminationDisabler*, WTF::IdentityExtractor, WTF::PtrHash<SafariShared::SuddenTerminationDisabler*>, WTF::HashTraits<SafariShared::SuddenTerminationDisabler*>, WTF::HashTraits<SafariShared::SuddenTerminationDisabler*> >::rehash(unsigned int, SafariShared::SuddenTerminationDisabler**) + 56
7   JavaScriptCore                        0x1af1202b4 WTF::fastZeroedMalloc(unsigned long) + 124
6   JavaScriptCore                        0x1af28d688 bmalloc::Cache::allocateSlowCaseNullCache(bmalloc::HeapKind, unsigned long) + 112
5   JavaScriptCore                        0x1af28da5c bmalloc::PerHeapKindBase<bmalloc::Cache>::PerHeapKindBase<>() + 44
4   JavaScriptCore                        0x1af28d4b0 bmalloc::PerProcess<bmalloc::PerHeapKind<bmalloc::Heap> >::getSlowCase() + 180
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]
Comment 1 Radar WebKit Bug Importer 2018-12-07 10:34:44 PST
<rdar://problem/46557903>
Comment 2 David Kilzer (:ddkilzer) 2018-12-07 10:42:47 PST
Created attachment 356821 [details]
Patch v1
Comment 3 Alexey Proskuryakov 2018-12-07 10:43:52 PST
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 4 Alexey Proskuryakov 2018-12-07 10:44:11 PST
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.
Comment 5 David Kilzer (:ddkilzer) 2018-12-07 11:12:07 PST
(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)?
Comment 6 David Kilzer (:ddkilzer) 2018-12-13 04:03:00 PST
(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).
Comment 7 David Kilzer (:ddkilzer) 2019-02-19 19:04:28 PST
I also wonder if r241789 for Bug 194811 improved this prior to the latest dev tools.