Attachment 358031 Details for Bug 193016 – Patch v1

[patch] Patch v1

bug-193016-20181223151538.patch (text/plain), 14.64 KB, created by David Kilzer (:ddkilzer) on 2018-12-23 15:15:38 PST

(hide)

Description:

Filename:

MIME Type:

Creator: David Kilzer (:ddkilzer)

Created: 2018-12-23 15:15:38 PST

Size: 14.64 KB

patch

obsolete

>Subversion Revision: 239367
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index bf4166f6b1e5f30c08e92bb44b0f940b02f4c86a..c1c6ca830db3d44a41da0540cfcb9247deb3e9af 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,20 @@
>+2018-12-23 David Kilzer <ddkilzer@apple.com>
>+
>+ Leak of CMSampleBuffer (752 bytes) in com.apple.WebKit.WebContent running WebKit layout tests
>+ <https://webkit.org/b/193016>
>+ <rdar://problem/46925703>
>+
>+ Reviewed by NOBODY (OOPS!).
>+
>+ * wtf/RetainPtr.h:
>+ (WTF::RetainPtr::handle):
>+ - Add method that lets us pass RetainPtr<>::m_ptr directly to
>+ functions that employ +1 retained out parameters.
>+ (WTF::RetainPtr::fromStorageHandleType):
>+ - Use concept of SFINAE to decide whether we need a
>+ const_cast<void**>() or not when casting StorageType* to
>+ PtrHandleType.
>+
> 2018-12-10 David Kilzer <ddkilzer@apple.com>
> 
> Hack to make WebKit compile with USE_SYSTEM_MALLOC=1 on macOS
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index ff907605b66e1ed0cea74d9881becb774b0a6b4b..1ba42ea24ae1ba160de0d3ea98b9b42bd1f9d168 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,29 @@
>+2018-12-23 David Kilzer <ddkilzer@apple.com>
>+
>+ Leak of CMSampleBuffer (752 bytes) in com.apple.WebKit.WebContent running WebKit layout tests
>+ <https://webkit.org/b/193016>
>+ <rdar://problem/46925703>
>+
>+ Reviewed by NOBODY (OOPS!).
>+
>+ Test: imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-destroy-script-execution.html
>+
>+ * platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:
>+ (WebCore::copySampleBufferWithCurrentTimeStamp):
>+ - Change to return RetainPtr<>.
>+ - Use RetainPtr<>::handle() to capture +1 retained
>+ CMSampleBuffer object.
>+ - Check return value of CMSampleBufferCreateCopyWithNewTiming().
>+ (WebCore::MediaRecorderPrivateWriter::appendVideoSampleBuffer):
>+ - Check return value of copySampleBufferWithCurrentTimeStamp().
>+ - Fix leak by using RetainPtr<> returned from
>+ copySampleBufferWithCurrentTimeStamp() instead of leaking raw
>+ bufferWithCurrentTime by calling retainPtr() vs. adoptCF().
>+ (WebCore::MediaRecorderPrivateWriter::appendAudioSampleBuffer):
>+ - Check return value of CMAudioFormatDescriptionCreate().
>+ - Fix leaks of both CMFormatDescriptionRef and CMSampleBufferRef
>+ by using RetainPtr<>::handle() to capture +1 retained objects.
>+
> 2018-12-18 Justin Michaud <justin_michaud@apple.com>
> 
> Update CSS Properties and Values API to use new cycle fallback behaviour
>diff --git a/Source/WTF/wtf/RetainPtr.h b/Source/WTF/wtf/RetainPtr.h
>index aa96dea1ac967031c8084a2be11a76bd717a9675..a5c0bf0dd9e2aca563d3461a58cfd7bebf82a29f 100644
>--- a/Source/WTF/wtf/RetainPtr.h
>+++ b/Source/WTF/wtf/RetainPtr.h
>@@ -62,6 +62,11 @@ template<typename T> class RetainPtr {
> public:
> typedef typename std::remove_pointer<T>::type ValueType;
> typedef ValueType* PtrType;
>+#ifdef __OBJC__
>+ typedef ValueType* __strong * PtrHandleType;
>+#else
>+ typedef ValueType** PtrHandleType;
>+#endif
> typedef CFTypeRef StorageType;
> 
> RetainPtr() : m_ptr(nullptr) { }
>@@ -88,6 +93,7 @@ public:
> #endif
> 
> PtrType get() const { return fromStorageType(m_ptr); }
>+ PtrHandleType handle() { return fromStorageHandleType<PtrHandleType>(&m_ptr); }
> PtrType operator->() const { return fromStorageType(m_ptr); }
> explicit operator PtrType() const { return fromStorageType(m_ptr); }
> explicit operator bool() const { return m_ptr; }
>@@ -97,7 +103,7 @@ public:
> // This conversion operator allows implicit conversion to bool but not to other integer types.
> typedef StorageType RetainPtr::*UnspecifiedBoolType;
> operator UnspecifiedBoolType() const { return m_ptr ? &RetainPtr::m_ptr : nullptr; }
>- 
>+
> RetainPtr& operator=(const RetainPtr&);
> template<typename U> RetainPtr& operator=(const RetainPtr&);
> RetainPtr& operator=(PtrType);
>@@ -140,9 +146,22 @@ private:
> {
> return (PtrType)const_cast<CF_BRIDGED_TYPE(id) void*>(ptr);
> }
>+
> StorageType toStorageType(PtrType ptr) const { return (StorageType)ptr; }
> #endif
> 
>+ template<typename U, typename std::enable_if<std::is_const<typename std::remove_pointer<typename std::remove_pointer::type>::type>::value == true, int>::type = 0>
>+ U fromStorageHandleType(StorageType* handle)
>+ {
>+ return reinterpret_cast(handle);
>+ }
>+
>+ template<typename U, typename std::enable_if<std::is_const<typename std::remove_pointer<typename std::remove_pointer::type>::type>::value == false, int>::type = 0>
>+ U fromStorageHandleType(StorageType* handle)
>+ {
>+ return reinterpret_cast(const_cast<void**>(handle));
>+ }
>+
> #ifdef __OBJC__
> template<typename U> std::enable_if_t<std::is_convertible<U, id>::value, PtrType> autoreleaseHelper();
> template<typename U> std::enable_if_t<!std::is_convertible<U, id>::value, PtrType> autoreleaseHelper();
>diff --git a/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm b/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm
>index 57aeec32d4ed2751e69a763529ccf1aaecf0d8b0..4f19ed796f8da777476d6cf2d707cdec900c66e5 100644
>--- a/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm
>+++ b/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm
>@@ -196,7 +196,7 @@ bool MediaRecorderPrivateWriter::setAudioInput()
> return true;
> }
> 
>-static inline CMSampleBufferRef copySampleBufferWithCurrentTimeStamp(CMSampleBufferRef originalBuffer)
>+static inline RetainPtr<CMSampleBufferRef> copySampleBufferWithCurrentTimeStamp(CMSampleBufferRef originalBuffer)
> {
> CMTime startTime = CMClockGetTime(CMClockGetHostTimeClock());
> CMItemCount count = 0;
>@@ -210,8 +210,10 @@ static inline CMSampleBufferRef copySampleBufferWithCurrentTimeStamp(CMSampleBuf
> timeInfo[i].presentationTimeStamp = startTime;
> }
> 
>- CMSampleBufferRef newBuffer;
>- CMSampleBufferCreateCopyWithNewTiming(kCFAllocatorDefault, originalBuffer, count, timeInfo.data(), &newBuffer);
>+ RetainPtr<CMSampleBufferRef> newBuffer;
>+ OSStatus error = CMSampleBufferCreateCopyWithNewTiming(kCFAllocatorDefault, originalBuffer, count, timeInfo.data(), newBuffer.handle());
>+ if (error)
>+ return nullptr;
> return newBuffer;
> }
> 
>@@ -249,9 +251,11 @@ void MediaRecorderPrivateWriter::appendVideoSampleBuffer(CMSampleBufferRef sampl
> }];
> return;
> }
>- CMSampleBufferRef bufferWithCurrentTime = copySampleBufferWithCurrentTimeStamp(sampleBuffer);
>+ auto bufferWithCurrentTime = copySampleBufferWithCurrentTimeStamp(sampleBuffer);
>+ if (!bufferWithCurrentTime)
>+ return;
> auto locker = holdLock(m_videoLock);
>- m_videoBufferPool.append(retainPtr(bufferWithCurrentTime));
>+ m_videoBufferPool.append(bufferWithCurrentTime);
> }
> 
> void MediaRecorderPrivateWriter::appendAudioSampleBuffer(const PlatformAudioData& data, const AudioStreamDescription& description, const WTF::MediaTime&, size_t sampleCount)
>@@ -259,11 +263,12 @@ void MediaRecorderPrivateWriter::appendAudioSampleBuffer(const PlatformAudioData
> ASSERT(m_audioInput);
> if ((!m_hasStartedWriting && m_videoInput) || m_isStopped)
> return;
>- CMSampleBufferRef sampleBuffer;
>- CMFormatDescriptionRef format;
>+ RetainPtr<CMFormatDescriptionRef> format;
> OSStatus error;
> auto& basicDescription = *WTF::get<const AudioStreamBasicDescription*>(description.platformDescription().description);
>- error = CMAudioFormatDescriptionCreate(kCFAllocatorDefault, &basicDescription, 0, NULL, 0, NULL, NULL, &format);
>+ error = CMAudioFormatDescriptionCreate(kCFAllocatorDefault, &basicDescription, 0, NULL, 0, NULL, NULL, format.handle());
>+ if (error)
>+ return;
> if (m_isFirstAudioSample) {
> if (!m_videoInput) {
> // audio-only recording.
>@@ -295,15 +300,16 @@ void MediaRecorderPrivateWriter::appendAudioSampleBuffer(const PlatformAudioData
> }
> CMTime startTime = CMClockGetTime(CMClockGetHostTimeClock());
> 
>- error = CMAudioSampleBufferCreateWithPacketDescriptions(kCFAllocatorDefault, NULL, false, NULL, NULL, format, sampleCount, startTime, NULL, &sampleBuffer);
>+ RetainPtr<CMSampleBufferRef> sampleBuffer;
>+ error = CMAudioSampleBufferCreateWithPacketDescriptions(kCFAllocatorDefault, NULL, false, NULL, NULL, format.get(), sampleCount, startTime, NULL, sampleBuffer.handle());
> if (error)
> return;
>- error = CMSampleBufferSetDataBufferFromAudioBufferList(sampleBuffer, kCFAllocatorDefault, kCFAllocatorDefault, 0, downcast<WebAudioBufferList>(data).list());
>+ error = CMSampleBufferSetDataBufferFromAudioBufferList(sampleBuffer.get(), kCFAllocatorDefault, kCFAllocatorDefault, 0, downcast<WebAudioBufferList>(data).list());
> if (error)
> return;
> 
> auto locker = holdLock(m_audioLock);
>- m_audioBufferPool.append(retainPtr(sampleBuffer));
>+ m_audioBufferPool.append(sampleBuffer);
> }
> 
> void MediaRecorderPrivateWriter::stopRecording()
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 7eafcec1f1e4aec7b0b5695fbf8df3825ec42b0b..8d62cfb3c0aa2b6f12fe765d84bc5da114881538 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,24 @@
>+2018-12-23 David Kilzer <ddkilzer@apple.com>
>+
>+ Leak of CMSampleBuffer (752 bytes) in com.apple.WebKit.WebContent running WebKit layout tests
>+ <https://webkit.org/b/193016>
>+ <rdar://problem/46925703>
>+
>+ Reviewed by NOBODY (OOPS!).
>+
>+ * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
>+ - Add missing RetainPtr.cpp to TestWTF target.
>+ * TestWebKitAPI/Tests/WTF/cf/RetainPtr.cpp:
>+ (TestWebKitAPI::CreateCFStringWithOutParameter):
>+ - Add helper function.
>+ (TestWebKitAPI::TEST):
>+ - Add test for CFTypeRef* out parameters.
>+ * TestWebKitAPI/Tests/WTF/ns/RetainPtr.mm:
>+ (+[MyClass newNSStringWithOutParameter:]):
>+ - Add helper class method.
>+ (TestWebKitAPI::TEST):
>+ - Add test for NSObject ** out parameters.
>+
> 2018-12-07 David Kilzer <ddkilzer@apple.com>
> 
> Ignore false-positive leaks under bmalloc::Heap::Heap
>diff --git a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>index 7e8ed3be51484308e0d8a1a9b78ba358eff554a5..09fb77abbb986aeab34058c73717942ecf0f89d1 100644
>--- a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>+++ b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>@@ -170,6 +170,7 @@
> 		4433A396208044140091ED57 /* SynchronousTimeoutTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 4433A395208044130091ED57 /* SynchronousTimeoutTests.mm */; };
> 		44817A2F1F0486BF00003810 /* WKRequestActivatedElementInfo.mm in Sources */ = {isa = PBXBuildFile; fileRef = 44817A2E1F0486BF00003810 /* WKRequestActivatedElementInfo.mm */; };
> 		448D7E471EA6C55500ECC756 /* EnvironmentUtilitiesTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 448D7E451EA6C55500ECC756 /* EnvironmentUtilitiesTest.cpp */; };
>+		44AC8BC621D0245A00CAFB34 /* RetainPtr.cpp in Sources */ = {isa = PBXBuildFile; fileRef = BC029B161486AD6400817DA9 /* RetainPtr.cpp */; };
> 		4612C2B9210A6ACE00B788A6 /* LoadFileThenReload.mm in Sources */ = {isa = PBXBuildFile; fileRef = 4612C2B8210A6ABF00B788A6 /* LoadFileThenReload.mm */; };
> 		46397B951DC2C850009A78AE /* DOMNode.mm in Sources */ = {isa = PBXBuildFile; fileRef = 46397B941DC2C850009A78AE /* DOMNode.mm */; };
> 		4647B1261EBA3B850041D7EF /* ProcessDidTerminate.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 4647B1251EBA3B730041D7EF /* ProcessDidTerminate.cpp */; };
>@@ -3775,6 +3776,7 @@
> 				7C83DF151D0A590C00FEBCF3 /* RefCounter.cpp in Sources */,
> 				7C83DED71D0A590C00FEBCF3 /* RefLogger.cpp in Sources */,
> 				7C83DF161D0A590C00FEBCF3 /* RefPtr.cpp in Sources */,
>+				44AC8BC621D0245A00CAFB34 /* RetainPtr.cpp in Sources */,
> 				7C83DF241D0A590C00FEBCF3 /* RetainPtr.mm in Sources */,
> 				7C83DF051D0A590C00FEBCF3 /* RunLoop.cpp in Sources */,
> 				7C83DF261D0A590C00FEBCF3 /* SaturatedArithmeticOperations.cpp in Sources */,
>diff --git a/Tools/TestWebKitAPI/Tests/WTF/cf/RetainPtr.cpp b/Tools/TestWebKitAPI/Tests/WTF/cf/RetainPtr.cpp
>index 0033f3b8bc689b271ed064d478b8b6b675247085..270891a5aa12a8982a1e1d578e3d14600aaa7fc3 100644
>--- a/Tools/TestWebKitAPI/Tests/WTF/cf/RetainPtr.cpp
>+++ b/Tools/TestWebKitAPI/Tests/WTF/cf/RetainPtr.cpp
>@@ -32,6 +32,19 @@
> 
> namespace TestWebKitAPI {
> 
>+bool CreateCFStringWithOutParameter(CFStringRef* cfStringHandle)
>+{
>+ if (!cfStringHandle)
>+ return false;
>+
>+ CFStringRef cfString = CFStringCreateWithCString(kCFAllocatorDefault, __PRETTY_FUNCTION__, kCFStringEncodingISOLatin1);
>+ if (!cfString)
>+ return false;
>+
>+ *cfStringHandle = cfString;
>+ return true;
>+}
>+
> TEST(RetainPtr, AdoptCF)
> {
> RetainPtr<CFArrayRef> foo = adoptCF(CFArrayCreate(kCFAllocatorDefault, nullptr, 0, nullptr));
>@@ -39,4 +52,14 @@ TEST(RetainPtr, AdoptCF)
> EXPECT_EQ(1, CFGetRetainCount(foo.get()));
> }
> 
>+TEST(RetainPtr, OutParameterCF)
>+{
>+ RetainPtr<CFStringRef> string;
>+ bool result = CreateCFStringWithOutParameter(string.handle());
>+
>+ EXPECT_TRUE(result);
>+ EXPECT_TRUE(string.get());
>+ EXPECT_EQ(1, CFGetRetainCount(string.get()));
>+}
>+
> } // namespace TestWebKitAPI
>diff --git a/Tools/TestWebKitAPI/Tests/WTF/ns/RetainPtr.mm b/Tools/TestWebKitAPI/Tests/WTF/ns/RetainPtr.mm
>index f925ce7cf95935016d53ea630182a1d1820597fc..8bc0381493ffdd65a772b3f1bbf37965af0ce73e 100644
>--- a/Tools/TestWebKitAPI/Tests/WTF/ns/RetainPtr.mm
>+++ b/Tools/TestWebKitAPI/Tests/WTF/ns/RetainPtr.mm
>@@ -30,6 +30,25 @@
> 
> #include <wtf/RetainPtr.h>
> 
>+@interface MyClass : NSObject
>++ (BOOL)newNSStringWithOutParameter:(NSString * __strong *)nsStringHandle;
>+@end
>+
>+@implementation MyClass
>++ (BOOL)newNSStringWithOutParameter:(NSString * __strong *)nsStringHandle
>+{
>+ if (!nsStringHandle)
>+ return false;
>+
>+ NSString *nsString = [[NSString alloc] initWithUTF8String:__PRETTY_FUNCTION__];
>+ if (!nsString)
>+ return false;
>+
>+ *nsStringHandle = nsString;
>+ return true;
>+}
>+@end
>+
> namespace TestWebKitAPI {
> 
> TEST(RetainPtr, AdoptNS)
>@@ -123,4 +142,14 @@ TEST(RetainPtr, ConstructionFromSimilarType)
> EXPECT_EQ((NSString *)0, temp);
> }
> 
>+TEST(RetainPtr, OutParameterNS)
>+{
>+ RetainPtr<NSString> string;
>+ bool result = [MyClass newNSStringWithOutParameter:string.handle()];
>+
>+ EXPECT_TRUE(result);
>+ EXPECT_TRUE(string.get());
>+ EXPECT_EQ(1, CFGetRetainCount((CFStringRef)string.get()));
>+}
>+
> } // namespace TestWebKitAPI

Actions: View | Formatted Diff | Diff

Attachments on bug 193016: 358031 | 358032 | 358146