Bug 172395 – WebAVStreamDataParserListener String leak

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 310717 [details]
[PATCH] Proposed Fix

I'm still building, so no cq yet.

[Chris Dumez]

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.

[Joseph Pecoraro]

Created attachment 310721 [details]
[PATCH] Proposed Fix

[Chris Dumez]

Comment on attachment 310721 [details]
[PATCH] Proposed Fix

r=me

[Chris Dumez]

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.

[WebKit Commit Bot]

Comment on attachment 310721 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 310721

Committed r217183: <http://trac.webkit.org/changeset/217183>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.