Bug 175064 – CFString leak dragging an image - allocation under PlatformPasteboard::writeObjectRepresentations

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Joseph Pecoraro]

Leak seen in an iOS application dragging an image.

Leak: 0x1393de630  size=64  zone: WebKit Using System Malloc_0x10315c000
	0x39372260 0x00000001 0xc0192550 0x00000001 	`"79....P%......
	0x00000000 0x00000000 0xb58c2f31 0x000001a1 	........1/......
	0x00000740 0x00000001 0x39372274 0x00000001 	@.......t"79....
	0x0000009a 0x00000000 0x00000000 0x00000000 	................
	Call stack: [thread 0x1b58beb40]: 
        | 0x0 
        | start 
        | 0x102772080 
        | UIApplicationMain 
        ...
        | -[WebView(WebPrivate) _requestStartDataInteraction:globalPosition:] 
        | WebCore::EventHandler::tryToBeginDataInteractionAtPoint(WebCore::IntPoint const&, WebCore::IntPoint const&) 
        | WebCore::EventHandler::handleMouseDraggedEvent(WebCore::MouseEventWithHitTestResults const&, WebCore::CheckDragHysteresis) 
        | WebCore::EventHandler::handleDrag(WebCore::MouseEventWithHitTestResults const&, WebCore::CheckDragHysteresis) 
        | WebCore::DragController::startDrag(WebCore::Frame&, WebCore::DragState const&, WebCore::DragOperation, WebCore::PlatformMouseEvent const&, WebCore::IntPoint const&) 
        | WebDragClient::declareAndWriteDragImage(WTF::String const&, WebCore::Element&, WebCore::URL const&, WTF::String const&, WebCore::Frame*) 
        | WebCore::Editor::writeImageToPasteboard(WebCore::Pasteboard&, WebCore::Element&, WebCore::URL const&, WTF::String const&) 
        | non-virtual thunk to WebPlatformStrategies::writeToPasteboard(WebCore::PasteboardImage const&, WTF::String const&) 
        | WebCore::PlatformPasteboard::writeObjectRepresentations(WebCore::PasteboardImage const&) 
        | WTF::StringImpl::operator NSString*() 
        | WTF::StringImpl::createCFString() 
        | CFStringCreateWithBytesNoCopy 
        | __CFStringCreateImmutableFunnel3 
        | _CFRuntimeCreateInstance 
        | WTF::StringWrapperCFAllocator::allocate(long, unsigned long, void*) 
        | WTF::fastMalloc(unsigned long) 
        | bmalloc::DebugHeap::malloc(unsigned long) 
        | malloc_zone_malloc 

There are only a few WTF::String -> NSString conversions in this function. Notably:

>       auto utiOrMIMEType = pasteboardImage.resourceMIMEType.createCFString();
>       if (!UTTypeIsDeclared(utiOrMIMEType.get()))
>           utiOrMIMEType = UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType, utiOrMIMEType.get(), nil);

In the first assignment the type is RetainPtr and should be fine.
In the second assignment it looks like an extra +1. We should adopt a Created value into a RetainPtr.

[Joseph Pecoraro]

I am able to reproduce, I'll try just adding an adoptCF() here.

[Joseph Pecoraro]

Created attachment 316931 [details]
[PATCH] Proposed Fix

[Tim Horton]

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

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

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:296
>          auto utiOrMIMEType = pasteboardImage.resourceMIMEType.createCFString();

This is one of those times where I really don't like this auto

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:121
> +    [_suggestedName release];

Alternatively, we could stop with the autosynthesis and back this with a RetainPtr.

[WebKit Commit Bot]

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

Clearing flags on attachment: 316931

Committed r220124: <http://trac.webkit.org/changeset/220124>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33671119>

[Joseph Pecoraro]

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

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

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:121
>> +    [_suggestedName release];
> 
> Alternatively, we could stop with the autosynthesis and back this with a RetainPtr.

Yeah, I normally would do that but I was confused by what RetainPtr would do given we specified the property as -copy. Would it just work, or would it actually be doing -retains under the hood unless we implemented our own setter that explicitly called -copy. Do you happen to know what it would do?

[Tim Horton]

You would need to copy yourself.

[Darin Adler]

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

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

>>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:121
>>> +    [_suggestedName release];
>> 
>> Alternatively, we could stop with the autosynthesis and back this with a RetainPtr.
> 
> Yeah, I normally would do that but I was confused by what RetainPtr would do given we specified the property as -copy. Would it just work, or would it actually be doing -retains under the hood unless we implemented our own setter that explicitly called -copy. Do you happen to know what it would do?

RetainPtr itself definitely does not know how to copy rather than retain. But you would write code like this:

    _suggestedName = adoptNS([suggestedName copy]);

And that’s how you would get it to copy.

[Joseph Pecoraro]

Okay, so you'd have to write out the setter. Thats what I figured.