SharedBuffer::createNSData should return a RetainPtr<NSData>
authorandersca@apple.com <andersca@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Sep 2013 19:52:04 +0000 (19:52 +0000)
committerandersca@apple.com <andersca@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 12 Sep 2013 19:52:04 +0000 (19:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=121237

Reviewed by Darin Adler.

Source/WebCore:

This makes memory management cleared and fixes a leak in PDFDocumentImage::createPDFDocument.
We use a RetainPtr subclass as a stopgap measure to prevent code that does [buffer->createNSData() autorelease]
from compiling and crashing due to the NSData object being overreleased.

* loader/ResourceBuffer.h:
* loader/mac/ResourceBuffer.mm:
(WebCore::ResourceBuffer::createNSData):
* platform/SharedBuffer.h:
(WebCore::SharedBuffer::NSDataRetainPtr::NSDataRetainPtr):
* platform/graphics/mac/PDFDocumentImageMac.mm:
(WebCore::PDFDocumentImage::createPDFDocument):
* platform/mac/HTMLConverter.mm:
(-[WebHTMLConverter _addAttachmentForElement:URL:needsParagraph:usePlaceholder:]):
(fileWrapperForURL):
* platform/mac/PasteboardMac.mm:
(WebCore::fileWrapper):
(WebCore::Pasteboard::read):
(WebCore::documentFragmentWithRTF):
(WebCore::fragmentFromWebArchive):
* platform/mac/PlatformPasteboardMac.mm:
(WebCore::PlatformPasteboard::setBufferForType):
* platform/mac/SharedBufferMac.mm:
(WebCore::SharedBuffer::createNSData):

Source/WebKit/mac:

Update for WebCore changes. This also fixes a leak where we'd create an NSFileWrapper from NSData but never release the data.

* WebView/WebDataSource.mm:
(-[WebDataSource data]):
* WebView/WebHTMLRepresentation.mm:
(-[WebHTMLRepresentation documentSource]):
* WebView/WebHTMLView.mm:
(-[WebHTMLView namesOfPromisedFilesDroppedAtDestination:]):
* WebView/WebResource.mm:
(-[WebResource encodeWithCoder:]):
(-[WebResource data]):

Source/WebKit2:

Update for WebCore changes.

* UIProcess/API/mac/WKView.mm:
(-[WKView writeSelectionToPasteboard:types:]):
(-[WKView _setPromisedData:WebCore::withFileName:withExtension:withTitle:withURL:withVisibleURL:withArchive:WebCore::forPasteboard:]):
(-[WKView namesOfPromisedFilesDroppedAtDestination:]):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@155641 268f45cc-cd09-0410-ab3c-d52691b4dbfc

16 files changed:
Source/WebCore/ChangeLog
Source/WebCore/loader/ResourceBuffer.h
Source/WebCore/loader/mac/ResourceBuffer.mm
Source/WebCore/platform/SharedBuffer.h
Source/WebCore/platform/graphics/mac/PDFDocumentImageMac.mm
Source/WebCore/platform/mac/HTMLConverter.mm
Source/WebCore/platform/mac/PasteboardMac.mm
Source/WebCore/platform/mac/PlatformPasteboardMac.mm
Source/WebCore/platform/mac/SharedBufferMac.mm
Source/WebKit/mac/ChangeLog
Source/WebKit/mac/WebView/WebDataSource.mm
Source/WebKit/mac/WebView/WebHTMLRepresentation.mm
Source/WebKit/mac/WebView/WebHTMLView.mm
Source/WebKit/mac/WebView/WebResource.mm
Source/WebKit2/ChangeLog
Source/WebKit2/UIProcess/API/mac/WKView.mm

index a729b1c85fd97d5eb55965f9899d68444f2f645e..5df943dca9a78e119c44113c5e0ba57890975564 100644 (file)
@@ -1,3 +1,34 @@
+2013-09-12  Anders Carlsson  <andersca@apple.com>
+
+        SharedBuffer::createNSData should return a RetainPtr<NSData>
+        https://bugs.webkit.org/show_bug.cgi?id=121237
+
+        Reviewed by Darin Adler.
+
+        This makes memory management cleared and fixes a leak in PDFDocumentImage::createPDFDocument.
+        We use a RetainPtr subclass as a stopgap measure to prevent code that does [buffer->createNSData() autorelease]
+        from compiling and crashing due to the NSData object being overreleased.
+
+        * loader/ResourceBuffer.h:
+        * loader/mac/ResourceBuffer.mm:
+        (WebCore::ResourceBuffer::createNSData):
+        * platform/SharedBuffer.h:
+        (WebCore::SharedBuffer::NSDataRetainPtr::NSDataRetainPtr):
+        * platform/graphics/mac/PDFDocumentImageMac.mm:
+        (WebCore::PDFDocumentImage::createPDFDocument):
+        * platform/mac/HTMLConverter.mm:
+        (-[WebHTMLConverter _addAttachmentForElement:URL:needsParagraph:usePlaceholder:]):
+        (fileWrapperForURL):
+        * platform/mac/PasteboardMac.mm:
+        (WebCore::fileWrapper):
+        (WebCore::Pasteboard::read):
+        (WebCore::documentFragmentWithRTF):
+        (WebCore::fragmentFromWebArchive):
+        * platform/mac/PlatformPasteboardMac.mm:
+        (WebCore::PlatformPasteboard::setBufferForType):
+        * platform/mac/SharedBufferMac.mm:
+        (WebCore::SharedBuffer::createNSData):
+
 2013-09-12  Timothy Hatcher  <timothy@apple.com>
 
         Remove more Timeline stuff we don't use
index ae0542bd76ac9bc5001c3ca0f419eb2476ffb017..44389ce304156da0793fe5ed65ce04d3385e25d4 100644 (file)
@@ -76,7 +76,7 @@ public:
     PassOwnPtr<PurgeableBuffer> releasePurgeableBuffer();
 
 #if PLATFORM(MAC)
-    NSData *createNSData();
+    SharedBuffer::NSDataRetainPtrWithoutImplicitConversionOperator createNSData();
 #endif
 #if USE(CF)
     RetainPtr<CFDataRef> createCFData();
index 13e0cb08877701be339935812ac82a55734f8435..47d5a65d7a9d625e69c816ae83796c645700f9ec 100644 (file)
@@ -28,7 +28,7 @@
 
 namespace WebCore {
 
-NSData* ResourceBuffer::createNSData()
+SharedBuffer::NSDataRetainPtrWithoutImplicitConversionOperator ResourceBuffer::createNSData()
 {
     return m_sharedBuffer->createNSData();
 }
index d7a9355eac6339aed6a6e6fe6015c1b6884246a4..58485d1c2ceb88877db80fab4679bccd91b5973f 100644 (file)
@@ -63,7 +63,23 @@ public:
     ~SharedBuffer();
     
 #if PLATFORM(MAC)
-    NSData *createNSData();
+    // FIXME: This class exists as a temporary workaround so that code that does:
+    // [buffer->createNSData() autorelease] will fail to compile.
+    // Once both Mac and iOS builds with this change we can change the return type to be RetainPtr<NSData>,
+    // since we're mostly worried about existing code breaking (it's unlikely that we'd use retain/release together
+    // with RetainPtr in new code.
+    class NSDataRetainPtrWithoutImplicitConversionOperator : public RetainPtr<NSData*> {
+    public:
+        template<typename T>
+        NSDataRetainPtrWithoutImplicitConversionOperator(RetainPtr<T*>&& other)
+            : RetainPtr<NSData*>(std::move(other))
+        {
+        }
+
+        explicit operator PtrType() = delete;
+    };
+
+    NSDataRetainPtrWithoutImplicitConversionOperator createNSData();
     static PassRefPtr<SharedBuffer> wrapNSData(NSData *data);
 #endif
 #if USE(CF)
index 98a8d7ae0491af98a502d1cb5eff9c10a2ad6b60..c52972e6be690fdb6d0664514ccfff9e5203d815 100644 (file)
@@ -53,7 +53,7 @@ namespace WebCore {
 
 void PDFDocumentImage::createPDFDocument()
 {
-    m_document = adoptNS([[getPDFDocumentClass() alloc] initWithData:data()->createNSData()]);
+    m_document = adoptNS([[getPDFDocumentClass() alloc] initWithData:data()->createNSData().get()]);
 }
 
 void PDFDocumentImage::computeBoundsForCurrentPage()
index 109a1f8e454646e8ec8a779660de6e8ee31e4014..0e052db48a6b75d32c28acfc85f6a9ab37dc55d2 100644 (file)
@@ -803,7 +803,7 @@ static inline NSShadow *_shadowForShadowStyle(NSString *shadowStyle)
         if (flag && resource && mimeType == "text/html")
             notFound = YES;
         if (resource && !notFound) {
-            fileWrapper = [[[NSFileWrapper alloc] initRegularFileWithContents:[resource->data()->createNSData() autorelease]] autorelease];
+            fileWrapper = [[[NSFileWrapper alloc] initRegularFileWithContents:resource->data()->createNSData().get()] autorelease];
             [fileWrapper setPreferredFilename:suggestedFilenameWithMIMEType(url, mimeType)];
         }
     }
@@ -1731,7 +1731,7 @@ static NSFileWrapper *fileWrapperForURL(DocumentLoader *dataSource, NSURL *URL)
     
     RefPtr<ArchiveResource> resource = dataSource->subresource(URL);
     if (resource) {
-        NSFileWrapper *wrapper = [[[NSFileWrapper alloc] initRegularFileWithContents:[resource->data()->createNSData() autorelease]] autorelease];
+        NSFileWrapper *wrapper = [[[NSFileWrapper alloc] initRegularFileWithContents:resource->data()->createNSData().get()] autorelease];
         NSString *filename = resource->response().suggestedFilename();
         if (!filename || ![filename length])
             filename = suggestedFilenameWithMIMEType(resource->url(), resource->mimeType());
index b53d83c1ab647a530ab47b21ec73bbb6df679b96..09b328b5d9c5d8e283c1997325728ef3974d600e 100644 (file)
@@ -227,9 +227,7 @@ void Pasteboard::write(const PasteboardURL& pasteboardURL)
 
 static NSFileWrapper* fileWrapper(const PasteboardImage& pasteboardImage)
 {
-    NSData *data = pasteboardImage.resourceData->createNSData();
-    NSFileWrapper *wrapper = [[[NSFileWrapper alloc] initRegularFileWithContents:data] autorelease];
-    [data release];
+    NSFileWrapper *wrapper = [[[NSFileWrapper alloc] initRegularFileWithContents:pasteboardImage.resourceData->createNSData().get()] autorelease];
     [wrapper setPreferredFilename:suggestedFilenameWithMIMEType(pasteboardImage.url.url, pasteboardImage.resourceMIMEType)];
     return wrapper;
 }
@@ -285,7 +283,7 @@ void Pasteboard::read(PasteboardPlainText& text)
     
     if (types.contains(String(NSRTFDPboardType))) {
         if (RefPtr<SharedBuffer> data = platformStrategies()->pasteboardStrategy()->bufferForType(NSRTFDPboardType, m_pasteboardName)) {
-            if (auto attributedString = adoptNS([[NSAttributedString alloc] initWithRTFD:[data->createNSData() autorelease] documentAttributes:NULL])) {
+            if (auto attributedString = adoptNS([[NSAttributedString alloc] initWithRTFD:data->createNSData().get() documentAttributes:NULL])) {
                 text.plainText = [attributedString string];
                 return;
             }
@@ -294,7 +292,7 @@ void Pasteboard::read(PasteboardPlainText& text)
 
     if (types.contains(String(NSRTFPboardType))) {
         if (RefPtr<SharedBuffer> data = platformStrategies()->pasteboardStrategy()->bufferForType(NSRTFPboardType, m_pasteboardName)) {
-            if (auto attributedString = adoptNS([[NSAttributedString alloc] initWithRTF:[data->createNSData() autorelease] documentAttributes:NULL])) {
+            if (auto attributedString = adoptNS([[NSAttributedString alloc] initWithRTF:data->createNSData().get() documentAttributes:NULL])) {
                 text.plainText = [attributedString string];
                 return;
             }
@@ -349,12 +347,12 @@ static PassRefPtr<DocumentFragment> documentFragmentWithRTF(Frame* frame, NSStri
     if (pasteboardType == NSRTFDPboardType) {
         RefPtr<SharedBuffer> data = platformStrategies()->pasteboardStrategy()->bufferForType(NSRTFDPboardType, pastebordName);
         if (data)
-            string = [[NSAttributedString alloc] initWithRTFD:[data->createNSData() autorelease] documentAttributes:NULL];
+            string = [[NSAttributedString alloc] initWithRTFD:data->createNSData().get() documentAttributes:NULL];
     }
     if (string == nil) {
         RefPtr<SharedBuffer> data = platformStrategies()->pasteboardStrategy()->bufferForType(NSRTFPboardType, pastebordName);
         if (data)
-            string = [[NSAttributedString alloc] initWithRTF:[data->createNSData() autorelease] documentAttributes:NULL];
+            string = [[NSAttributedString alloc] initWithRTF:data->createNSData().get() documentAttributes:NULL];
     }
     if (string == nil)
         return nil;
@@ -404,7 +402,7 @@ static PassRefPtr<DocumentFragment> fragmentFromWebArchive(Frame* frame, PassRef
         return 0;
 
     if (frame->loader().client().canShowMIMETypeAsHTML(MIMEType)) {
-        RetainPtr<NSString> markupString = adoptNS([[NSString alloc] initWithData:[mainResource->data()->createNSData() autorelease] encoding:NSUTF8StringEncoding]);
+        RetainPtr<NSString> markupString = adoptNS([[NSString alloc] initWithData:mainResource->data()->createNSData().get() encoding:NSUTF8StringEncoding]);
         // FIXME: seems poor form to do this as a side effect of getting a document fragment
         if (DocumentLoader* loader = frame->loader().documentLoader())
             loader->addAllArchiveResources(coreArchive.get());
index 53a40aae7f1d1fbacc4c18e4037f59f40d044607..40b41e880ef4a54950e51fa63c0c72a2e1ddeaf9 100644 (file)
@@ -136,7 +136,7 @@ long PlatformPasteboard::setTypes(const Vector<String>& pasteboardTypes)
 
 long PlatformPasteboard::setBufferForType(PassRefPtr<SharedBuffer> buffer, const String& pasteboardType)
 {
-    BOOL didWriteData = [m_pasteboard.get() setData:buffer ? [buffer->createNSData() autorelease] : nil forType:pasteboardType];
+    BOOL didWriteData = [m_pasteboard setData:buffer ? buffer->createNSData().get() : nil forType:pasteboardType];
     if (!didWriteData)
         return 0;
     return changeCount();
index 47515b3b20b13dffd9c2d7df7014b77d08fbe1db..316cf6cd4d0b520f022ee5def34fab6bb8fb09cb 100644 (file)
@@ -96,9 +96,9 @@ PassRefPtr<SharedBuffer> SharedBuffer::wrapNSData(NSData *nsData)
     return adoptRef(new SharedBuffer((CFDataRef)nsData));
 }
 
-NSData *SharedBuffer::createNSData()
+SharedBuffer::NSDataRetainPtrWithoutImplicitConversionOperator SharedBuffer::createNSData()
 {    
-    return [[WebCoreSharedBufferData alloc] initWithSharedBuffer:this];
+    return adoptNS([[WebCoreSharedBufferData alloc] initWithSharedBuffer:this]);
 }
 
 RetainPtr<CFDataRef> SharedBuffer::createCFData()
index 52e076f727b6fc98b3e93d083318cdc2370fe78f..6b3feac19191a1fd81a9ba521945105a324eb5f6 100644 (file)
@@ -1,3 +1,22 @@
+2013-09-12  Anders Carlsson  <andersca@apple.com>
+
+        SharedBuffer::createNSData should return a RetainPtr<NSData>
+        https://bugs.webkit.org/show_bug.cgi?id=121237
+
+        Reviewed by Darin Adler.
+
+        Update for WebCore changes. This also fixes a leak where we'd create an NSFileWrapper from NSData but never release the data.
+
+        * WebView/WebDataSource.mm:
+        (-[WebDataSource data]):
+        * WebView/WebHTMLRepresentation.mm:
+        (-[WebHTMLRepresentation documentSource]):
+        * WebView/WebHTMLView.mm:
+        (-[WebHTMLView namesOfPromisedFilesDroppedAtDestination:]):
+        * WebView/WebResource.mm:
+        (-[WebResource encodeWithCoder:]):
+        (-[WebResource data]):
+
 2013-09-12  Mark Lam  <mark.lam@apple.com>
 
         Change debug hooks to pass sourceID and position info via the DebuggerCallFrame.
index ec05dd2e6388da4cdbc7662d273802ed699c8276..c5f2c27470d7d109023440b193466cb2ad54327f 100644 (file)
@@ -405,7 +405,7 @@ static inline void addTypesFromClass(NSMutableDictionary *allTypes, Class objCCl
     RefPtr<ResourceBuffer> mainResourceData = toPrivate(_private)->loader->mainResourceData();
     if (!mainResourceData)
         return nil;
-    return [mainResourceData->createNSData() autorelease];
+    return [mainResourceData->createNSData().leakRef() autorelease];
 }
 
 - (id <WebDocumentRepresentation>)representation
index 6b538b815f356b2a585bbf55894460ffa4c6cae5..9f09dd49bf408605782be85a128e6526870fb425 100644 (file)
@@ -237,9 +237,7 @@ static NSMutableArray *newArrayByConcatenatingArrays(NSArray *first, NSArray *se
 {
     if ([self _isDisplayingWebArchive]) {            
         SharedBuffer *parsedArchiveData = [_private->dataSource _documentLoader]->parsedArchiveData();
-        NSData *nsData = parsedArchiveData ? parsedArchiveData->createNSData() : nil;
-        NSString *result = [[NSString alloc] initWithData:nsData encoding:NSUTF8StringEncoding];
-        [nsData release];
+        NSString *result = [[NSString alloc] initWithData:parsedArchiveData ? parsedArchiveData->createNSData().get() : nil encoding:NSUTF8StringEncoding];
         return [result autorelease];
     }
 
index d4f2525ddcd67b38a08402f740f005d7e964f8bf..0b2a62d8199bff74e57c847b37f3c995abad3a52 100644 (file)
@@ -3652,10 +3652,9 @@ static bool matchesExtensionOrEquivalent(NSString *filename, NSString *extension
     
     if (WebCore::CachedImage* tiffResource = [self promisedDragTIFFDataSource]) {
         if (ResourceBuffer *buffer = static_cast<CachedResource*>(tiffResource)->resourceBuffer()) {
-            NSData *data = buffer->createNSData();
             NSURLResponse *response = tiffResource->response().nsURLResponse();
             draggingImageURL = [response URL];
-            wrapper = [[[NSFileWrapper alloc] initRegularFileWithContents:data] autorelease];
+            wrapper = [[[NSFileWrapper alloc] initRegularFileWithContents:buffer->createNSData().get()] autorelease];
             NSString* filename = [response suggestedFilename];
             NSString* trueExtension(tiffResource->image()->filenameExtension());
             if (!matchesExtensionOrEquivalent(filename, trueExtension))
index e42598a8ef284a8b3e56adfed68bfb62a9ec12a6..a4ec35315afd4bf45dc2c5de64720a31607c90cd 100644 (file)
@@ -175,7 +175,7 @@ static NSString * const WebResourceResponseKey =          @"WebResourceResponse"
     
     if (resource) {
         if (resource->data())
-            data = [resource->data()->createNSData() autorelease];
+            data = resource->data()->createNSData().get();
         url = resource->url();
         mimeType = resource->mimeType();
         textEncoding = resource->textEncoding();
@@ -209,7 +209,7 @@ static NSString * const WebResourceResponseKey =          @"WebResourceResponse"
         return nil;
     if (!_private->coreResource->data())
         return nil;
-    return [_private->coreResource->data()->createNSData() autorelease];
+    return [_private->coreResource->data()->createNSData().leakRef() autorelease];
 }
 
 - (NSURL *)URL
index 315b3d0accb9a47d5052aceff5db873937b8fa63..9719ae67e2a8b0e1e61528f7eb30d6868e48e5a7 100644 (file)
@@ -1,3 +1,17 @@
+2013-09-12  Anders Carlsson  <andersca@apple.com>
+
+        SharedBuffer::createNSData should return a RetainPtr<NSData>
+        https://bugs.webkit.org/show_bug.cgi?id=121237
+
+        Reviewed by Darin Adler.
+
+        Update for WebCore changes.
+
+        * UIProcess/API/mac/WKView.mm:
+        (-[WKView writeSelectionToPasteboard:types:]):
+        (-[WKView _setPromisedData:WebCore::withFileName:withExtension:withTitle:withURL:withVisibleURL:withArchive:WebCore::forPasteboard:]):
+        (-[WKView namesOfPromisedFilesDroppedAtDestination:]):
+
 2013-09-12  Anders Carlsson  <andersca@apple.com>
 
         SharedBuffer::createCFData should return RetainPtr<CFDataRef>
index 62095962cfa439cd8dbf6d244675486fbdb0b54d..77ed70c6f9cb9e97d45ce9b2f6a1a5da3375d9cf 100644 (file)
@@ -661,7 +661,7 @@ WEBCORE_COMMAND(yankAndSelect)
             [pasteboard setString:_data->_page->stringSelectionForPasteboard() forType:NSStringPboardType];
         else {
             RefPtr<SharedBuffer> buffer = _data->_page->dataSelectionForPasteboard([types objectAtIndex:i]);
-            [pasteboard setData:buffer ? [buffer->createNSData() autorelease] : nil forType:[types objectAtIndex:i]];
+            [pasteboard setData:buffer ? buffer->createNSData().get() : nil forType:[types objectAtIndex:i]];
        }
     }
     return YES;
@@ -2791,7 +2791,7 @@ static bool matchesExtensionOrEquivalent(NSString *filename, NSString *extension
     [pasteboard setPropertyList:[NSArray arrayWithObject:extension] forType:NSFilesPromisePboardType];
 
     if (archiveBuffer)
-        [pasteboard setData:[archiveBuffer->createNSData() autorelease] forType:PasteboardTypes::WebArchivePboardType];
+        [pasteboard setData:archiveBuffer->createNSData().get() forType:PasteboardTypes::WebArchivePboardType];
 
     _data->_promisedImage = image;
     _data->_promisedFilename = filename;
@@ -2859,9 +2859,9 @@ static NSString *pathWithUniqueFilenameForPath(NSString *path)
     RetainPtr<NSData> data;
     
     if (_data->_promisedImage) {
-        data = adoptNS(_data->_promisedImage->data()->createNSData());
+        data = _data->_promisedImage->data()->createNSData();
         wrapper = adoptNS([[NSFileWrapper alloc] initRegularFileWithContents:data.get()]);
-        [wrapper.get() setPreferredFilename:_data->_promisedFilename];
+        [wrapper setPreferredFilename:_data->_promisedFilename];
     }
     
     if (!wrapper) {