Bug 172395 - WebAVStreamDataParserListener String leak
Summary: WebAVStreamDataParserListener String leak
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-19 15:37 PDT by Joseph Pecoraro
Modified: 2017-05-19 21:24 PDT (History)
6 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (2.34 KB, patch)
2017-05-19 15:46 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (2.31 KB, patch)
2017-05-19 16:29 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-05-19 15:37:25 PDT
Summary:
WebAVStreamDataParserListener String leak seen after some browsing:

Leak: 0x7fa13af36e10  size=32  zone: WebKit Using System Malloc_0x1017e1000
	0x00000002 0x00000004 0x3af36e24 0x00007fa1 	........$n.:....
	0x00000008 0x31637661 0x20666f20 0x00023a33 	....avc1 of 3:..
	Call stack: [thread 0x70000784a000]:
    |  start_wqthread
    |  _pthread_wqthread
    |  _dispatch_worker_thread4
    |  _dispatch_root_queue_drain
    |  _dispatch_async_redirect_invoke
    |  _dispatch_continuation_pop
    |  _dispatch_client_callout
    |  _dispatch_call_block_and_release
    |  ___ZN7WebCore26SourceBufferPrivateAVFObjC6appendEPKhj_block_invoke
    |  -[AVStreamDataParser appendStreamData:withFlags:]
    |  -[AVStreamDataParser _appendStreamData:withFlags:]
    |  0x7fff89f705dc
    |  0x7fff89f723ae
    |  0x7fff89f7588e
    |  -[AVStreamDataParser(AVStreamDataParser_FigManifold) _figManifold:pushedSampleBuffer:trackID:flags:]
    |  -[WebAVStreamDataParserListener streamDataParser:didProvideMediaData:forTrackID:mediaType:flags:]
    |  WTF::String::String(NSString*)
    |  WTF::StringImpl::create(unsigned char const*, unsigned int)
    |  WTF::fastMalloc(unsigned long)
    |  bmalloc::DebugHeap::malloc(unsigned long) 

There seem to be a few places doing:

    String mediaType = ...;
    callOnMainThread([..., mediaType, flags] {
        ...
    });

String is not thread safe, so if this is cross thread (which the backtrace above appears to indicate) then we need to have an isolated copy for the lambda. A race condition may have caused the leak to happen here, and there could be worse issues.
Comment 1 Joseph Pecoraro 2017-05-19 15:46:14 PDT
Created attachment 310717 [details]
[PATCH] Proposed Fix

I'm still building, so no cq yet.
Comment 2 Chris Dumez 2017-05-19 16:07:45 PDT
Comment on attachment 310717 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=310717&action=review

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:178
> +    callOnMainThread([parent = _parent, protectedSample = WTFMove(protectedSample), trackID, mediaType = String(nsMediaType).isolatedCopy(), flags] {

I am not convinced you need the isolatedCopy here if you move the String constructor to the lambda initialization.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:188
> +    callOnMainThread([parent = _parent, trackID, mediaType = String(nsMediaType).isolatedCopy()] {

ditto.
Comment 3 Joseph Pecoraro 2017-05-19 16:29:24 PDT
Created attachment 310721 [details]
[PATCH] Proposed Fix
Comment 4 Chris Dumez 2017-05-19 16:31:00 PDT
Comment on attachment 310721 [details]
[PATCH] Proposed Fix

r=me
Comment 5 Chris Dumez 2017-05-19 16:32:42 PDT
Comment on attachment 310721 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=310721&action=review

> Source/WebCore/ChangeLog:11
> +        Use an isolated String in a lambda used across threads.

We should update this comment though.
Comment 6 WebKit Commit Bot 2017-05-19 21:24:45 PDT
Comment on attachment 310721 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 310721

Committed r217183: <http://trac.webkit.org/changeset/217183>
Comment 7 WebKit Commit Bot 2017-05-19 21:24:47 PDT
All reviewed patches have been landed.  Closing bug.