Bug 193016

Summary:

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

Product:

WebKit

Reporter:

David Kilzer (:ddkilzer) <ddkilzer>

Component:

Media

Assignee:

David Kilzer (:ddkilzer) <ddkilzer>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, darin, dbates, eric.carlson, ews-watchlist, jer.noble, mitz, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch v1	none
Patch v2	none
Patch v3	simon.fraser: review+, simon.fraser: commit-queue-

David Kilzer (:ddkilzer)

Reported 2018-12-23 15:11:26 PST

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]

Attachments
Patch v1 (14.64 KB, patch) 2018-12-23 15:15 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch v2 (14.62 KB, patch) 2018-12-23 16:50 PST, David Kilzer (:ddkilzer)	no flags	Details Formatted Diff Diff
Patch v3 (7.15 KB, patch) 2018-12-29 21:07 PST, David Kilzer (:ddkilzer)	simon.fraser: review+ simon.fraser: commit-queue-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

David Kilzer (:ddkilzer)

Comment 1 2018-12-23 15:11:36 PST

<rdar://problem/46925703>

David Kilzer (:ddkilzer)

Comment 2 2018-12-23 15:15:38 PST

Created attachment 358031 [details] Patch v1

EWS Watchlist

Comment 3 2018-12-23 15:18:10 PST

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 4 2018-12-23 15:23:56 PST

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)

Comment 5 2018-12-23 16:50:13 PST

Created attachment 358032 [details] Patch v2 Fix style warnings.

David Kilzer (:ddkilzer)

Comment 6 2018-12-24 04:18:58 PST

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 7 2018-12-24 04:44:18 PST

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 8 2018-12-24 07:54:22 PST

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

Comment 9 2018-12-24 12:09:39 PST

(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

Comment 10 2018-12-24 12:26:01 PST

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)

Comment 11 2018-12-28 10:15:29 PST

(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)

Comment 12 2018-12-29 21:07:35 PST

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 13 2019-01-02 11:00:57 PST

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 14 2019-01-02 15:44:33 PST

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 15 2019-01-02 22:06:59 PST

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)

Comment 16 2019-01-02 22:43:12 PST

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

Note You need to log in before you can comment on or make changes to this bug.