Bug 193016 – Leak of CMSampleBuffer (752 bytes) in com.apple.WebKit.WebContent running WebKit layout tests

• Component: Media
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Leak of CMSampleBuffer (752 bytes) in com.apple.WebKit.WebContent running WebKit layout tests.

$ ./Tools/Scripts/run-webkit-tests --no-build --debug --batch-size=1000 --child-processes=1 --verbose --leaks --no-retry --no-show-results imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-destroy-script-execution.html

NOTE: Requires changes to run-webkit-tests to support --leaks with WebKit2.

STACK OF 1 INSTANCE OF 'ROOT LEAK: <CMSampleBuffer>':
[thread 0x1096575c0]:
23  libdyld.dylib                      0x7fff6611008d start + 1
22  com.apple.WebKit.WebContent           0x100de77bb invocation function for block in WebKit::XPCServiceEventHandler(NSObject<OS_xpc_object>*) + 0  XPCServiceMain.mm:46
21  com.apple.WebKit.WebContent           0x100de7636 WebKit::XPCServiceMain(int, char const**) + 547  XPCServiceMain.mm:0
20  libxpc.dylib                       0x7fff663469e5 _xpc_copy_xpcservice_dictionary + 0
19  libxpc.dylib                       0x7fff66346ee6 _xpc_objc_main + 555
18  com.apple.Foundation               0x7fff3b21b28f -[NSRunLoop(NSRunLoop) run] + 76
17  com.apple.Foundation               0x7fff3b21b3ba -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 280
16  com.apple.CoreFoundation           0x7fff38e96be6 CFRunLoopRunSpecific + 467
15  com.apple.CoreFoundation           0x7fff38e976c4 __CFRunLoopRun + 2187
14  com.apple.CoreFoundation           0x7fff38eb611d __CFRunLoopDoTimers + 333
13  com.apple.CoreFoundation           0x7fff38eb65e8 __CFRunLoopDoTimer + 871
12  com.apple.CoreFoundation           0x7fff38eb6a35 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
11  com.apple.JavaScriptCore              0x10651ffcf WTF::timerFired(__CFRunLoopTimer*, void*) + 31  MainThreadCocoa.mm:110
10  com.apple.JavaScriptCore              0x10651f239 WTF::dispatchFunctionsFromMainThread() + 249  memory:2597
9   com.apple.WebCore                     0x102bf6c38 WTF::Function<void ()>::CallableWrapper<WebCore::CanvasCaptureMediaStreamTrack::Source::create(WebCore::HTMLCanvasElement&, std::optional<double>&&)::$_4>::call() + 88  utility:896
8   com.apple.WebCore                     0x10354819b WebCore::RealtimeMediaSource::videoSampleAvailable(WebCore::MediaSample&) + 59  memory:2285
7   com.apple.WebCore                     0x103547fc7 WebCore::RealtimeMediaSource::forEachObserver(WTF::Function<void (WebCore::RealtimeMediaSource::Observer&)> const&) const + 663  Atomics.h:247
6   com.apple.WebCore                     0x103544a66 WebCore::MediaStreamTrackPrivate::videoSampleAvailable(WebCore::MediaSample&) + 182  memory:2285
5   com.apple.WebCore                     0x1035443a7 WebCore::MediaStreamTrackPrivate::forEachObserver(WTF::Function<void (WebCore::MediaStreamTrackPrivate::Observer&)> const&) const + 663  Atomics.h:247
4   com.apple.WebCore                     0x1026813eb WebCore::MediaRecorderPrivateWriter::appendVideoSampleBuffer(opaqueCMSampleBuffer*) + 603  MediaRecorderPrivateWriterCocoa.mm:215
3   com.apple.CoreMedia                0x7fff3a01fb35 CMSampleBufferCreateCopyWithNewTiming + 296
2   com.apple.CoreMedia                0x7fff39fefb59 sBufCreate + 140
1   com.apple.CoreFoundation           0x7fff38e5fc70 _CFRuntimeCreateInstance + 271
0   libsystem_malloc.dylib             0x7fff662bd82b malloc_zone_malloc + 139 
====
    9 (752 bytes) ROOT LEAK: <CMSampleBuffer 0x7ffa39529f50> [432]
       4 (176 bytes) <CMFormatDescription 0x7ffa39522a90> [48]
          3 (128 bytes) <CFDictionary 0x7ffa39529820> [64]
             1 (32 bytes) <CFDictionary (Key Storage) 0x7ffa39525b90> [32]
             1 (32 bytes) <CFDictionary (Value Storage) 0x7ffa395250f0> [32]
       4 (144 bytes) <NSArray 0x7ffa3951efc0> [16]
          3 (128 bytes) __strong _object --> <CFDictionary 0x7ffa39520f30> [64]
             1 (32 bytes) <CFDictionary (Key Storage) 0x7ffa39520000> [32]
             1 (32 bytes) <CFDictionary (Value Storage) 0x7ffa3951cb40> [32]

[David Kilzer (:ddkilzer)]

<rdar://problem/46925703>

[David Kilzer (:ddkilzer)]

Created attachment 358031 [details]
Patch v1

[EWS Watchlist]

Attachment 358031 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/RetainPtr.h:153:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WTF/wtf/RetainPtr.h:159:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Comment on attachment 358031 [details]
Patch v1

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

> Source/WTF/wtf/RetainPtr.h:96
> +    PtrHandleType handle() { return fromStorageHandleType<PtrHandleType>(&m_ptr); }

I'm most interested in feedback on this approach of exposing RetainPtr<>::m_ptr so that it can be set by out parameters directly to avoid all the extra stack variable normally required to put an out parameter into a RetainPtr<>:

    CMSampleBufferRef rawNewBuffer;
    OSStatus error = CMSampleBufferCreateCopyWithNewTiming(kCFAllocatorDefault, originalBuffer, count, timeInfo.data(), &rawNewBuffer);
    if (error)
        return;
    RetainPtr<CMSampleBufferRef> newBuffer = adoptCF(rawNewBuffer);

Versus:

    RetainPtr<CMSampleBufferRef> newBuffer;
    OSStatus error = CMSampleBufferCreateCopyWithNewTiming(kCFAllocatorDefault, originalBuffer, count, timeInfo.data(), newBuffer.handle());
    if (error)
        return nullptr;

[David Kilzer (:ddkilzer)]

Created attachment 358032 [details]
Patch v2

Fix style warnings.

[David Kilzer (:ddkilzer)]

Comment on attachment 358032 [details]
Patch v2

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

> Source/WTF/wtf/RetainPtr.h:162
> +    template<typename U, typename std::enable_if<!std::is_const<typename std::remove_pointer<typename std::remove_pointer<U>::type>::type>::value, int>::type = 0>
> +    U fromStorageHandleType(StorageType* handle)
> +    {
> +        return reinterpret_cast<U>(const_cast<void**>(handle));

Casting through `void**` is not allowed in ARC:

In file included from Tools/TestWebKitAPI/Tests/WTF/ns/RetainPtr.mm:31:
$BUILD_DIR/Release/usr/local/include/wtf/RetainPtr.h:162:16: error: cast of a non-Objective-C pointer type 'void **' to 'NSString *__strong *' is disallowed with ARC
        return reinterpret_cast<U>(const_cast<void**>(handle));
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Also, while there are warnings with ARC for passing an `NSObject * __strong *` as a parameter with type `NSObject * __autoreleasing *`, there are no warnings in MRR.  However, that mistake is easy to make without RetainPtr<>::handle(), so making handle() available for NSObjects still seems beneficial in the long run (assuming ARC is enabled for all of WebKit).

[David Kilzer (:ddkilzer)]

Comment on attachment 358032 [details]
Patch v2

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

> Source/WTF/wtf/RetainPtr.h:70
>      typedef CFTypeRef StorageType;

Continuing from my comment about `const_cast<void**>` below:

The fundamental problem may be that we're defining StorageType for NS object types as `CFTypeRef` (which is `const void*`) rather than `id`.

That could be fixed by doing something like this, then fixing the fallout of the now-unnecessary casts when compiling with MRR and ARC:

#ifdef __OBJC__
    typedef typename std::conditional<std::is_convertible<U, id>::value, id, CFTypeRef>::type StorageType;
#else
    typedef CFTypeRef StorageType;
#endif

[David Kilzer (:ddkilzer)]

Comment on attachment 358032 [details]
Patch v2

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

>> Source/WTF/wtf/RetainPtr.h:70
>>      typedef CFTypeRef StorageType;
> 
> Continuing from my comment about `const_cast<void**>` below:
> 
> The fundamental problem may be that we're defining StorageType for NS object types as `CFTypeRef` (which is `const void*`) rather than `id`.
> 
> That could be fixed by doing something like this, then fixing the fallout of the now-unnecessary casts when compiling with MRR and ARC:
> 
> #ifdef __OBJC__
>     typedef typename std::conditional<std::is_convertible<U, id>::value, id, CFTypeRef>::type StorageType;
> #else
>     typedef CFTypeRef StorageType;
> #endif

Of course, RetainPtr<> really isn't needed with ARC, so the use of CFTypeRef now is really just a way to control the retain/release calls without ARC doing its automatic thing.

I think it would be best to expose handle() as handleCF() and not provide the same feature for NS objects unless there is a compelling use case.

[Daniel Bates]

(In reply to David Kilzer (:ddkilzer) from comment #8)
> I think it would be best to expose handle() as handleCF() and not provide
> the same feature for NS objects unless there is a compelling use case.

Happy Holidays, Dave!

I understand your motivation for this patch. I have an alternative strategy (at the end). I am not in favor of exposing a handle()/handleCF() because I feel it makes using RetainPtr more error prone. Here is my contrived (for simplicity) counter example:

class A {
public:
    ...
    void update()
    {
        // Code that sets up originalBuffer, count and timeInfo, but does not touch m_buffer.
        CMSampleBufferCreateCopyWithNewTiming(kCFAllocatorDefault, originalBuffer, count, timeInfo.data(), m_buffer.handle());
    }
    ...
private
    RetainPtr<...> m_buffer;
};

Suppose there is driver code as follows:

A anA;
anA.update();
...
anA.someOtherOperation();
...
anA.update();

Do you see the bug? In A::update() we need to have a line analogous to "m_buffer = nullptr;" or we will leak m_buffer on the second call to A::update().

Whenever dealing with C API I tend to find the strategy of extracting the C code into its own function or object that works nicely with the C++ runtime a good approach. This is what you were doing when you changed the return type for copySampleBufferWithCurrentTimeStamp(). The side effect is that the implementation of copySampleBufferWithCurrentTimeStamp() will be ugly, but that seems acceptable so that call sites of copySampleBufferWithCurrentTimeStamp() are easier to read and less error prone. If you desire you can simply repeat this approach until copySampleBufferWithCurrentTimeStamp() is to your liking...extract the code for CMSampleBufferCreateCopyWithNewTiming() into a new helper function h that returns a RetainPtr<CMSampleBufferRef> and have copySampleBufferWithCurrentTimeStamp() call h.

[Daniel Bates]

If you were still interested in pursuing a handleCF() then I would consider naming it something scary (in the same vein as leakRef()).

[David Kilzer (:ddkilzer)]

(In reply to Daniel Bates from comment #9)
> (In reply to David Kilzer (:ddkilzer) from comment #8)
> > I think it would be best to expose handle() as handleCF() and not provide
> > the same feature for NS objects unless there is a compelling use case.
> 
> Happy Holidays, Dave!

Thanks Dan!  Hope you're able to disconnect.  (I've failed so badly at disconnecting that I am writing C++ template code!)

> I understand your motivation for this patch. I have an alternative strategy
> (at the end). I am not in favor of exposing a handle()/handleCF() because I
> feel it makes using RetainPtr more error prone. Here is my contrived (for
> simplicity) counter example:
> 
> class A {
> public:
>     ...
>     void update()
>     {
>         // Code that sets up originalBuffer, count and timeInfo, but does
> not touch m_buffer.
>         CMSampleBufferCreateCopyWithNewTiming(kCFAllocatorDefault,
> originalBuffer, count, timeInfo.data(), m_buffer.handle());
>     }
>     ...
> private
>     RetainPtr<...> m_buffer;
> };
> 
> Suppose there is driver code as follows:
> 
> A anA;
> anA.update();
> ...
> anA.someOtherOperation();
> ...
> anA.update();
> 
> Do you see the bug? In A::update() we need to have a line analogous to
> "m_buffer = nullptr;" or we will leak m_buffer on the second call to
> A::update().

Very good point that I didn't consider when originally writing handleCF().

In thinking about this problem the last couple of days, I realized that I could check if m_ptr is not nullptr, then release it each time handleCF() is called, and maybe add a Debug assertion.

However, my "simple" idea now has side-effects, so I'm not really sold on it anymore.

> Whenever dealing with C API I tend to find the strategy of extracting the C
> code into its own function or object that works nicely with the C++ runtime
> a good approach. This is what you were doing when you changed the return
> type for copySampleBufferWithCurrentTimeStamp(). The side effect is that the
> implementation of copySampleBufferWithCurrentTimeStamp() will be ugly, but
> that seems acceptable so that call sites of
> copySampleBufferWithCurrentTimeStamp() are easier to read and less error
> prone. If you desire you can simply repeat this approach until
> copySampleBufferWithCurrentTimeStamp() is to your liking...extract the code
> for CMSampleBufferCreateCopyWithNewTiming() into a new helper function h
> that returns a RetainPtr<CMSampleBufferRef> and have
> copySampleBufferWithCurrentTimeStamp() call h.

I agree.  (I really need that "extract a method" refactoring tool now! :)

(In reply to Daniel Bates from comment #10)
> If you were still interested in pursuing a handleCF() then I would consider
> naming it something scary (in the same vein as leakRef()).

Agreed.  I should have mentioned that handleCF() was more of a working name.  I also experimented with retainedOutParamCF(), but probably should put something to denote a raw pointer.  Also wanted to keep it shorter, though!

[David Kilzer (:ddkilzer)]

Created attachment 358146 [details]
Patch v3

- Removed RetainPtr<>::handleCF() changes and went with static helper methods per Dan's suggestion.
- Adding RetainPtr.cpp to TestWTFLibrary is now in patch for Bug 193056.

[Simon Fraser (smfr)]

Comment on attachment 358146 [details]
Patch v3

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

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:199
> +static inline RetainPtr<CMSampleBufferRef> copySampleBufferWithCurrentTimeStamp(CMSampleBufferRef originalBuffer)

Maybe this shouldn't have 'copy' in the name now that it returns a RetainPtr<>

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:259
> +    m_videoBufferPool.append(bufferWithCurrentTime);

WTFMove()?

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:264
> +    auto& basicDescription = *WTF::get<const AudioStreamBasicDescription*>(description.platformDescription().description);

Why get basicDescription as a reference and then turn it back into a pointer two lines later?

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:328
> +    m_audioBufferPool.append(sampleBuffer);

WTFMove()?

[Eric Carlson]

Comment on attachment 358146 [details]
Patch v3

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

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:214
> +    OSStatus error = CMSampleBufferCreateCopyWithNewTiming(kCFAllocatorDefault, originalBuffer, count, timeInfo.data(), &newBuffer);

Nit: s/OSStatus/auto/

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:266
> +    OSStatus error = CMAudioFormatDescriptionCreate(kCFAllocatorDefault, &basicDescription, 0, NULL, 0, NULL, NULL, &format);

Ditto.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:276
> +    OSStatus error = CMAudioSampleBufferCreateWithPacketDescriptions(kCFAllocatorDefault, NULL, false, NULL, NULL, format, sampleCount, startTime, NULL, &sampleBuffer);

Ditto.

> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:323
> +    OSStatus error = CMSampleBufferSetDataBufferFromAudioBufferList(sampleBuffer.get(), kCFAllocatorDefault, kCFAllocatorDefault, 0, downcast<WebAudioBufferList>(data).list());

Ditto.

[David Kilzer (:ddkilzer)]

Comment on attachment 358146 [details]
Patch v3

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

>> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:199
>> +static inline RetainPtr<CMSampleBufferRef> copySampleBufferWithCurrentTimeStamp(CMSampleBufferRef originalBuffer)
> 
> Maybe this shouldn't have 'copy' in the name now that it returns a RetainPtr<>

It's still copying it and returning a +1 retained CMSampleBufferRef--it's just in a leak-proof container now.  :)

>> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:214
>> +    OSStatus error = CMSampleBufferCreateCopyWithNewTiming(kCFAllocatorDefault, originalBuffer, count, timeInfo.data(), &newBuffer);
> 
> Nit: s/OSStatus/auto/

Will fix.

>> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:259
>> +    m_videoBufferPool.append(bufferWithCurrentTime);
> 
> WTFMove()?

Will fix.

>> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:264
>> +    auto& basicDescription = *WTF::get<const AudioStreamBasicDescription*>(description.platformDescription().description);
> 
> Why get basicDescription as a reference and then turn it back into a pointer two lines later?

I was focused on preserving the original code when moving it around; didn't notice the inefficiency here.  Will fix.

>> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:266
>> +    OSStatus error = CMAudioFormatDescriptionCreate(kCFAllocatorDefault, &basicDescription, 0, NULL, 0, NULL, NULL, &format);
> 
> Ditto.

Will fix.

>> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:276
>> +    OSStatus error = CMAudioSampleBufferCreateWithPacketDescriptions(kCFAllocatorDefault, NULL, false, NULL, NULL, format, sampleCount, startTime, NULL, &sampleBuffer);
> 
> Ditto.

Will fix.

>> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:323
>> +    OSStatus error = CMSampleBufferSetDataBufferFromAudioBufferList(sampleBuffer.get(), kCFAllocatorDefault, kCFAllocatorDefault, 0, downcast<WebAudioBufferList>(data).list());
> 
> Ditto.

Will fix.

>> Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:328
>> +    m_audioBufferPool.append(sampleBuffer);
> 
> WTFMove()?

Will fix.

[David Kilzer (:ddkilzer)]

Committed r239587: <https://trac.webkit.org/changeset/239587>