Bug 48278 – Convert DumpRenderTree webarchive code to CoreFoundation

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[David Kilzer (:ddkilzer)]

The current webarchive code in DumpRenderTree is written in Objective-C[++], which makes it impossible to share with the Windows port.  Rewriting it to use CoreFoundation (CF) methods instead would make it cross-platform and reusable on Windows.

Patches to be attached soon.

[David Kilzer (:ddkilzer)]

Created attachment 71817 [details]
Part 1: Extract call to -[WebHTMLRepresentation supportedNonImageMIMETypes] from WebArchiveDumpSupport.mm

[David Kilzer (:ddkilzer)]

Created attachment 71818 [details]
Part 2: Extract use of NSKeyedUnarchiver from WebArchiveDumpSupport.mm

[David Kilzer (:ddkilzer)]

Created attachment 71819 [details]
Part 3: Convert WebArchiveDumpSupport.mm from NS objects to CF types

[David Kilzer (:ddkilzer)]

Created attachment 71820 [details]
Part 4: Rename WebArchiveDumpSupport.mm to WebArchiveDumpSupport.cpp

[Adam Roben (:aroben)]

Comment on attachment 71819 [details]
Part 3: Convert WebArchiveDumpSupport.mm from NS objects to CF types

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

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.h:33
> +extern "C" {
> +typedef struct _CFURLResponse* CFURLResponseRef;
> +}

I don't think extern "C" is needed for CF-style typedefs. At least, we don't normally use it for them.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.h:40
> -NSString *serializeWebArchiveToXML(WebArchive *webArchive);
> +CFStringRef createXMLStringFromWebArchiveData(CFDataRef webArchiveData);
>  
>  #pragma mark -
>  #pragma mark Platform-specific methods
>  
> -NSURLResponse *unarchiveNSURLResponseFromResponseData(NSData *responseData);
> +CFURLResponseRef createCFURLResponseFromResponseData(CFDataRef responseData);

Maybe these new functions should return RetainPtrs?

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:30
> +#import <JavaScriptCore/RetainPtr.h>

Does <wtf/RetainPtr.h> not work in DRT?

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:48
> +    if (CFStringCompare(mimeType, CFSTR("text/xml"), kCFCompareAnchored|kCFCompareCaseInsensitive) == kCFCompareEqualTo)

I think you should mention the change to using a case-insensitive comparison in your ChangeLog. You also need spaces around the | operator.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:52
> +    if (CFStringCompare(mimeType, CFSTR("application/x-javascript"), kCFCompareAnchored|kCFCompareCaseInsensitive) == kCFCompareEqualTo)

Ditto.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:60
> +    CFStringLowercase(mimeType, CFLocaleGetSystem());
>      convertMIMEType(mimeType);

Seems like you don't need to lowercase here, given that convertMIMEType is now case-insensitive.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:72
> +        RetainPtr<CFStringRef> dataAsString(AdoptCF, CFStringCreateFromExternalRepresentation(NULL, data, stringEncoding));

Since this is C++ code, you should use 0 instead of NULL. (Though I think we may be moving toward always using kCFAllocatorDefault in this case.)

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:94
> -    if ([fields objectForKey:@"Connection"])
> -        [fields removeObjectForKey:@"Connection"];
> -    if ([fields objectForKey:@"Keep-Alive"])
> -        [fields removeObjectForKey:@"Keep-Alive"];
> +    if (CFDictionaryContainsKey(fields, CFSTR("Connection")))
> +        CFDictionaryRemoveValue(fields, CFSTR("Connection"));
> +    if (CFDictionaryContainsKey(fields, CFSTR("Keep-Alive")))
> +        CFDictionaryRemoveValue(fields, CFSTR("Keep-Alive"));

It's weird that we check for the presence of the keys before removing them. That shouldn't be necessary, right?

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:117
> +    RetainPtr<CFMutableDictionaryRef> responseDictionary(AdoptCF, CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

Same comment about NULL here (and elsewhere in this patch).

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:119
> -    NSMutableString *urlString = [[[response URL] description] mutableCopy];
> -    normalizeWebResourceURL(urlString);
> -    [responseDictionary setObject:urlString forKey:@"URL"];
> -    [urlString release];
> +    RetainPtr<CFMutableStringRef> urlString(AdoptCF, CFStringCreateMutableCopy(NULL, 0, CFURLGetString(CFURLResponseGetURL(response.get()))));

What does -[NSURL description] return? Is that really the same as CFURLGetString?

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:132
> +    SInt64 expectedContentLength = CFURLResponseGetExpectedContentLength(response.get());
> +    RetainPtr<CFNumberRef> expectedContentLengthNumber(AdoptCF, CFNumberCreate(NULL, kCFNumberLongLongType, &expectedContentLength));

Seems like you should either be using "long long" or kCFNumberSInt64Type.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:142
> +        CFIndex statusCode = CFHTTPMessageGetResponseStatusCode(httpMessage);
> +        RetainPtr<CFNumberRef> statusCodeNumber(AdoptCF, CFNumberCreate(NULL, kCFNumberLongType, &statusCode));

Seems like you should either be using "long" or kCFNumberCFIndexType.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:152
> +    CFStringRef url1 = (CFStringRef)CFDictionaryGetValue((CFDictionaryRef)val1, CFSTR("WebResourceURL"));
> +    CFStringRef url2 = (CFStringRef)CFDictionaryGetValue((CFDictionaryRef)val2, CFSTR("WebResourceURL"));

You could use static_cast here.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:166
> +            return CFErrorCopyDescription(error);
> +        return CFSTR("An unknown error occurred converting data to property list.");

Presumably you need to pass the result of CFSTR() through CFRetain before returning it to maintain the "create" semantics of this function.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:189
> +        for (CFIndex i = 0; i < CFArrayGetCount(subresources); ++i) {

It might be better to store the result of CFArrayGetCount in a local variable.

> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:206
> +            return CFErrorCopyDescription(error);
> +        return CFSTR("An unknown error occurred converting property list to data.");

Same comment here about CFRetain.

[Adam Roben (:aroben)]

Comment on attachment 71820 [details]
Part 4: Rename WebArchiveDumpSupport.mm to WebArchiveDumpSupport.cpp

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

> WebKitTools/ChangeLog:18
> +        * DumpRenderTree/cf/WebArchiveDumpSupport.cpp: Renamed from DumpRenderTree/mac/WebArchiveDumpSupport.mm.
> +        (convertMIMEType):
> +        (convertWebResourceDataToString):
> +        (normalizeHTTPResponseHeaderFields):
> +        (normalizeWebResourceURL):
> +        (convertWebResourceResponseToDictionary):
> +        (compareResourceURLs):
> +        (createXMLStringFromWebArchiveData):

I usually leave out function names when doing a file rename.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> (From update of attachment 71819 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71819&action=review
> 
> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.h:33
> > +extern "C" {
> > +typedef struct _CFURLResponse* CFURLResponseRef;
> > +}
> 
> I don't think extern "C" is needed for CF-style typedefs. At least, we don't normally use it for them.

Will try removing it.

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.h:40
> > -NSString *serializeWebArchiveToXML(WebArchive *webArchive);
> > +CFStringRef createXMLStringFromWebArchiveData(CFDataRef webArchiveData);
> >  
> >  #pragma mark -
> >  #pragma mark Platform-specific methods
> >  
> > -NSURLResponse *unarchiveNSURLResponseFromResponseData(NSData *responseData);
> > +CFURLResponseRef createCFURLResponseFromResponseData(CFDataRef responseData);
> 
> Maybe these new functions should return RetainPtrs?

They currently follow the "create" rule for CoreFoundation memory management.

I don't think PassRetainPtr<> exists.  Won't returning a RetainPtr<> cause ref churn?  (I realize it probably doesn't matter for DRT, but I would expect to use PassRetainPtr<> to pass an owning reference to a returning method.)

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:30
> > +#import <JavaScriptCore/RetainPtr.h>
> 
> Does <wtf/RetainPtr.h> not work in DRT?

Probably.  I just got caught up in using the framework name in the #import statement.  :)

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:48
> > +    if (CFStringCompare(mimeType, CFSTR("text/xml"), kCFCompareAnchored|kCFCompareCaseInsensitive) == kCFCompareEqualTo)
> 
> I think you should mention the change to using a case-insensitive comparison in your ChangeLog. You also need spaces around the | operator.

Will fix.

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:52
> > +    if (CFStringCompare(mimeType, CFSTR("application/x-javascript"), kCFCompareAnchored|kCFCompareCaseInsensitive) == kCFCompareEqualTo)
> 
> Ditto.

Will fix.

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:60
> > +    CFStringLowercase(mimeType, CFLocaleGetSystem());
> >      convertMIMEType(mimeType);
> 
> Seems like you don't need to lowercase here, given that convertMIMEType is now case-insensitive.

Will fix.

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:72
> > +        RetainPtr<CFStringRef> dataAsString(AdoptCF, CFStringCreateFromExternalRepresentation(NULL, data, stringEncoding));
> 
> Since this is C++ code, you should use 0 instead of NULL. (Though I think we may be moving toward always using kCFAllocatorDefault in this case.)

Will fix.

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:94
> > -    if ([fields objectForKey:@"Connection"])
> > -        [fields removeObjectForKey:@"Connection"];
> > -    if ([fields objectForKey:@"Keep-Alive"])
> > -        [fields removeObjectForKey:@"Keep-Alive"];
> > +    if (CFDictionaryContainsKey(fields, CFSTR("Connection")))
> > +        CFDictionaryRemoveValue(fields, CFSTR("Connection"));
> > +    if (CFDictionaryContainsKey(fields, CFSTR("Keep-Alive")))
> > +        CFDictionaryRemoveValue(fields, CFSTR("Keep-Alive"));
> 
> It's weird that we check for the presence of the keys before removing them. That shouldn't be necessary, right?

I was trying to prevent DRT from doing unnecessary work, but it may be more work to check first.  Will fix.

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:117
> > +    RetainPtr<CFMutableDictionaryRef> responseDictionary(AdoptCF, CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> 
> Same comment about NULL here (and elsewhere in this patch).

Will fix.

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:119
> > -    NSMutableString *urlString = [[[response URL] description] mutableCopy];
> > -    normalizeWebResourceURL(urlString);
> > -    [responseDictionary setObject:urlString forKey:@"URL"];
> > -    [urlString release];
> > +    RetainPtr<CFMutableStringRef> urlString(AdoptCF, CFStringCreateMutableCopy(NULL, 0, CFURLGetString(CFURLResponseGetURL(response.get()))));
> 
> What does -[NSURL description] return? Is that really the same as CFURLGetString?

-[NSURL description] returns an NSString of the URL.  I probably should have used -absolueString instead.  But CFURLGetString() returns an equivalent string.  The webarchive tests continue to pass after this change.

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:132
> > +    SInt64 expectedContentLength = CFURLResponseGetExpectedContentLength(response.get());
> > +    RetainPtr<CFNumberRef> expectedContentLengthNumber(AdoptCF, CFNumberCreate(NULL, kCFNumberLongLongType, &expectedContentLength));
> 
> Seems like you should either be using "long long" or kCFNumberSInt64Type.

Will fix.

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:142
> > +        CFIndex statusCode = CFHTTPMessageGetResponseStatusCode(httpMessage);
> > +        RetainPtr<CFNumberRef> statusCodeNumber(AdoptCF, CFNumberCreate(NULL, kCFNumberLongType, &statusCode));
> 
> Seems like you should either be using "long" or kCFNumberCFIndexType.

Will fix.

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:152
> > +    CFStringRef url1 = (CFStringRef)CFDictionaryGetValue((CFDictionaryRef)val1, CFSTR("WebResourceURL"));
> > +    CFStringRef url2 = (CFStringRef)CFDictionaryGetValue((CFDictionaryRef)val2, CFSTR("WebResourceURL"));
> 
> You could use static_cast here.

Will fix.

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:166
> > +            return CFErrorCopyDescription(error);
> > +        return CFSTR("An unknown error occurred converting data to property list.");
> 
> Presumably you need to pass the result of CFSTR() through CFRetain before returning it to maintain the "create" semantics of this function.

I believe CFSTR("") in CF is like an @"" string in Cocoa.  They are "constants" that never have a retain count of zero.  I will add the CFRetain().

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:189
> > +        for (CFIndex i = 0; i < CFArrayGetCount(subresources); ++i) {
> 
> It might be better to store the result of CFArrayGetCount in a local variable.

Will fix.

> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:206
> > +            return CFErrorCopyDescription(error);
> > +        return CFSTR("An unknown error occurred converting property list to data.");
> 
> Same comment here about CFRetain.

Will fix.

Thanks!

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> (From update of attachment 71820 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71820&action=review
> 
> > WebKitTools/ChangeLog:18
> > +        * DumpRenderTree/cf/WebArchiveDumpSupport.cpp: Renamed from DumpRenderTree/mac/WebArchiveDumpSupport.mm.
> > +        (convertMIMEType):
> > +        (convertWebResourceDataToString):
> > +        (normalizeHTTPResponseHeaderFields):
> > +        (normalizeWebResourceURL):
> > +        (convertWebResourceResponseToDictionary):
> > +        (compareResourceURLs):
> > +        (createXMLStringFromWebArchiveData):
> 
> I usually leave out function names when doing a file rename.

Will fix.  Thanks for the thorough review, Adam!

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:60
> > +    CFStringLowercase(mimeType, CFLocaleGetSystem());
> >      convertMIMEType(mimeType);
> 
> Seems like you don't need to lowercase here, given that convertMIMEType is now case-insensitive.

Later in the method I check if mimeType begins with "text/", so I do want CFStringLowercase() here.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:72
> > +        RetainPtr<CFStringRef> dataAsString(AdoptCF, CFStringCreateFromExternalRepresentation(NULL, data, stringEncoding));
> 
> Since this is C++ code, you should use 0 instead of NULL. (Though I think we may be moving toward always using kCFAllocatorDefault in this case.)

BTW, why is that?  Passing 0 or NULL gets you kCFAllocatorDefault regardless (per the documentation).

[David Kilzer (:ddkilzer)]

Part 1: Committed r70611: <http://trac.webkit.org/changeset/70611>
Part 2: Committed r70612: <http://trac.webkit.org/changeset/70612>
Part 3: Committed r70613: <http://trac.webkit.org/changeset/70613>
Part 4: Committed r70614: <http://trac.webkit.org/changeset/70614>

[Nikolas Zimmermann]

(In reply to comment #11)
> Part 1: Committed r70611: <http://trac.webkit.org/changeset/70611>
> Part 2: Committed r70612: <http://trac.webkit.org/changeset/70612>
> Part 3: Committed r70613: <http://trac.webkit.org/changeset/70613>
> Part 4: Committed r70614: <http://trac.webkit.org/changeset/70614>

Hm, this broke Leopard. CFPropertyListCreateWithData is only "Available in Mac OS X v10.6 and later."

[Nikolas Zimmermann]

Created attachment 71997 [details]
Fix Leopard build

Verified this change works, by running run-webkit-tests webarchive on Leopard.

[Jeremy Orlow]

Comment on attachment 71997 [details]
Fix Leopard build

build fix, so rubber stamp...would be nice if someone who knew this stuff better could look tho!

[David Kilzer (:ddkilzer)]

Comment on attachment 71997 [details]
Fix Leopard build

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

Thanks for catching and fixing this, Nikolas!  A couple nits below.  r=me

> WebKitTools/DumpRenderTree/cf/WebArchiveDumpSupport.cpp:159
> +#ifdef BUILDING_ON_LEOPARD

Technically, this should be:

#if defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD)

> WebKitTools/DumpRenderTree/cf/WebArchiveDumpSupport.cpp:161
> +    CFReadStreamRef readStream = CFReadStreamCreateWithBytesNoCopy(kCFAllocatorDefault, CFDataGetBytePtr(webArchiveData), bytesCount, kCFAllocatorNull);

Could use RetainPtr<> here:

RetainPtr<CFReadStreamRef> readStream(AdoptCF, CFReadStreamCreateWithBytesNoCopy(...));

> WebKitTools/DumpRenderTree/cf/WebArchiveDumpSupport.cpp:162
> +    CFReadStreamOpen(readStream);

Then all references to readStream become readStream.get().

> WebKitTools/DumpRenderTree/cf/WebArchiveDumpSupport.cpp:165
> +    CFRelease(readStream);

This can be removed if RetainPtr<> is used.

> WebKitTools/DumpRenderTree/cf/WebArchiveDumpSupport.cpp:209
> +#ifdef BUILDING_ON_LEOPARD

Technically, this should be:

#if defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD)

[Nikolas Zimmermann]

(In reply to comment #15)
> (From update of attachment 71997 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71997&action=review
> 
> Thanks for catching and fixing this, Nikolas!  A couple nits below.  r=me
Thanks David for your prompt response, I thought you were asleep, and it was easy to fix, so I wanted to avoid a rollout :-)

> 
> > WebKitTools/DumpRenderTree/cf/WebArchiveDumpSupport.cpp:159
> > +#ifdef BUILDING_ON_LEOPARD
> 
> Technically, this should be:
> 
> #if defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD)
Fixed.

> 
> > WebKitTools/DumpRenderTree/cf/WebArchiveDumpSupport.cpp:161
> > +    CFReadStreamRef readStream = CFReadStreamCreateWithBytesNoCopy(kCFAllocatorDefault, CFDataGetBytePtr(webArchiveData), bytesCount, kCFAllocatorNull);
> 
> Could use RetainPtr<> here:
> 
> RetainPtr<CFReadStreamRef> readStream(AdoptCF, CFReadStreamCreateWithBytesNoCopy(...));
Ah, that's the trick to avoid the manual release.

> > WebKitTools/DumpRenderTree/cf/WebArchiveDumpSupport.cpp:162
> > +    CFReadStreamOpen(readStream);
> 
> Then all references to readStream become readStream.get().
Fixed.

> 
> > WebKitTools/DumpRenderTree/cf/WebArchiveDumpSupport.cpp:165
> > +    CFRelease(readStream);
> 
> This can be removed if RetainPtr<> is used.
Fixed.

> 
> > WebKitTools/DumpRenderTree/cf/WebArchiveDumpSupport.cpp:209
> > +#ifdef BUILDING_ON_LEOPARD
> 
> Technically, this should be:
> 
> #if defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD)

Fixed, thanks for the review - landing soon!

[David Kilzer (:ddkilzer)]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 71997 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=71997&action=review
> > 
> > Thanks for catching and fixing this, Nikolas!  A couple nits below.  r=me
> Thanks David for your prompt response, I thought you were asleep, and it was easy to fix, so I wanted to avoid a rollout :-)

I was, but I sensed a disturbance in the Force.  ;)

[Nikolas Zimmermann]

(In reply to comment #15)
> (From update of attachment 71997 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71997&action=review
> 
> Thanks for catching and fixing this, Nikolas!  A couple nits below.  r=me

I did another leaks run before landing, and I'm seeing leaks, not sure wheter they are also present on SnowLeopard using the other APIs. Going to post a log and wait for your comment.

[Nikolas Zimmermann]

(In reply to comment #18)
> (In reply to comment #15)
> > (From update of attachment 71997 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=71997&action=review
> > 
> > Thanks for catching and fixing this, Nikolas!  A couple nits below.  r=me
> 
> I did another leaks run before landing, and I'm seeing leaks, not sure wheter they are also present on SnowLeopard using the other APIs. Going to post a log and wait for your comment.

Testing 26 test cases.
webarchive ......................
webarchive/loading ....
 ? checking for leaks in DumpRenderTree
 + 1340 leaks (68576 bytes) were found, details in /tmp/layout-test-results/DumpRenderTree-leaks.txt


    Call stack: [thread 0xa0855720]: | 0x2 | start | main | dumpRenderTree(int, char const**) | __ZL20runTestingServerLoopv | __ZL7runTestRKSs | -[NSRunLoop(NSRunLoop) runMode:beforeDate:] | CFRunLoopRunInMode | CFRunLoopRunSpecific | MultiplexerSource::perform() | URLConnectionClient::processEvents() | URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload(XConnectionEventInfo<XClientEvent, XClientEventParams>*, long) | URLConnectionClient::_clientDidFinishLoading(URLConnectionClient::ClientConnectionEventQueue*) | _NSURLConnectionDidFinishLoading | -[NSURLConnection(NSURLConnectionReallyInternal) sendDidFinishLoading] | -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] | WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) | WebCore::MainResourceLoader::didFinishLoading(double) | WebCore::FrameLoader::finishedLoading() | WebCore::FrameLoader::checkLoadComplete() | WebCore::FrameLoader::recursiveCheckLoadComplete() | WebCore::FrameLoader::checkLoadCompleteForThisFrame() | WebFrameLoaderClient::dispatchDidFinishLoad() | CallFrameLoadDelegate(objc_object* (*)(objc_object*, objc_selector*, ...), WebView*, objc_selector*, objc_object*) | __ZL12CallDelegatePFP11objc_objectS0_P13objc_selectorzEP7WebViewS0_S2_S0_ | -[FrameLoadDelegate webView:didFinishLoadForFrame:] | -[FrameLoadDelegate webView:locationChangeDone:forDataSource:] | dump() | createXMLStringFromWebArchiveData(__CFData const*) | CFPropertyListCreateFromStream | _CFPropertyListCreateFromXMLData | __CFTryParseBinaryPlist | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | CFDataAppendBytes | CFDataReplaceBytes | __CFDataGrow | malloc_zone_realloc
Leak: 0x90cc000  size=2048  string 'GIF89a4'
    Call stack: [thread 0xa0855720]: | 0x2 | start | main | dumpRenderTree(int, char const**) | __ZL20runTestingServerLoopv | __ZL7runTestRKSs | -[NSRunLoop(NSRunLoop) runMode:beforeDate:] | CFRunLoopRunInMode | CFRunLoopRunSpecific | __ZN7WebCoreL10timerFiredEP16__CFRunLoopTimerPv | WebCore::ThreadTimers::sharedTimerFired() | WebCore::ThreadTimers::sharedTimerFiredInternal() | WebCore::DOMTimer::fired() | WebCore::ScheduledAction::execute(WebCore::ScriptExecutionContext*) | WebCore::ScheduledAction::execute(WebCore::Document*) | WebCore::ScriptController::executeScriptInWorld(WebCore::DOMWrapperWorld*, WTF::String const&, bool, WebCore::ShouldAllowXSS) | WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*, WebCore::ShouldAllowXSS) | WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue) | JSC::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue) | JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::ScopeChainNode*, JSC::JSObject*, JSC::JSValue*) | JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*, JSC::JSValue*) | jscGeneratedNativeCode | cti_op_call_NotJSFunction | JSC::JSCallbackFunction::call(JSC::ExecState*) | __ZL18notifyDoneCallbackPK15OpaqueJSContextP13OpaqueJSValueS3_mPKPKS2_PS5_ | LayoutTestController::notifyDone() | dump() | createXMLStringFromWebArchiveData(__CFData const*) | CFPropertyListCreateFromStream | _CFPropertyListCreateFromXMLData | __CFTryParseBinaryPlist | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | CFDataAppendBytes | CFDataReplaceBytes | __CFDataGrow | malloc_zone_realloc

It's always these two leaks. Does that ring a bell? I'm not familiar with CF at all unfortunately.

[Adam Roben (:aroben)]

Comment on attachment 71819 [details]
Part 3: Convert WebArchiveDumpSupport.mm from NS objects to CF types

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

>>> WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.h:40
>>> +CFURLResponseRef createCFURLResponseFromResponseData(CFDataRef responseData);
>> 
>> Maybe these new functions should return RetainPtrs?
> 
> They currently follow the "create" rule for CoreFoundation memory management.
> 
> I don't think PassRetainPtr<> exists.  Won't returning a RetainPtr<> cause ref churn?  (I realize it probably doesn't matter for DRT, but I would expect to use PassRetainPtr<> to pass an owning reference to a returning method.)

Yeah, if we had PassRetainPtr that would be the best thing to use here. But, since we don't, RetainPtr is an OK alternative that we use in other places in WebKit like this.

[Adam Roben (:aroben)]

(In reply to comment #10)
> (In reply to comment #5)
> > > WebKitTools/DumpRenderTree/mac/WebArchiveDumpSupport.mm:72
> > > +        RetainPtr<CFStringRef> dataAsString(AdoptCF, CFStringCreateFromExternalRepresentation(NULL, data, stringEncoding));
> > 
> > Since this is C++ code, you should use 0 instead of NULL. (Though I think we may be moving toward always using kCFAllocatorDefault in this case.)
> 
> BTW, why is that?  Passing 0 or NULL gets you kCFAllocatorDefault regardless (per the documentation).

I believe kCFAllocatorDefault is preferred because it is more explicit than passing 0, and thus hopefully clearer to readers.

[Nikolas Zimmermann]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #15)
> > > (From update of attachment 71997 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=71997&action=review
> > > 
> > > Thanks for catching and fixing this, Nikolas!  A couple nits below.  r=me
> > 
> > I did another leaks run before landing, and I'm seeing leaks, not sure wheter they are also present on SnowLeopard using the other APIs. Going to post a log and wait for your comment.
> 
> Testing 26 test cases.
> webarchive ......................
> webarchive/loading ....
>  ? checking for leaks in DumpRenderTree
>  + 1340 leaks (68576 bytes) were found, details in /tmp/layout-test-results/DumpRenderTree-leaks.txt
> 
> 
>     Call stack: [thread 0xa0855720]: | 0x2 | start | main | dumpRenderTree(int, char const**) | __ZL20runTestingServerLoopv | __ZL7runTestRKSs | -[NSRunLoop(NSRunLoop) runMode:beforeDate:] | CFRunLoopRunInMode | CFRunLoopRunSpecific | MultiplexerSource::perform() | URLConnectionClient::processEvents() | URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload(XConnectionEventInfo<XClientEvent, XClientEventParams>*, long) | URLConnectionClient::_clientDidFinishLoading(URLConnectionClient::ClientConnectionEventQueue*) | _NSURLConnectionDidFinishLoading | -[NSURLConnection(NSURLConnectionReallyInternal) sendDidFinishLoading] | -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] | WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) | WebCore::MainResourceLoader::didFinishLoading(double) | WebCore::FrameLoader::finishedLoading() | WebCore::FrameLoader::checkLoadComplete() | WebCore::FrameLoader::recursiveCheckLoadComplete() | WebCore::FrameLoader::checkLoadCompleteForThisFrame() | WebFrameLoaderClient::dispatchDidFinishLoad() | CallFrameLoadDelegate(objc_object* (*)(objc_object*, objc_selector*, ...), WebView*, objc_selector*, objc_object*) | __ZL12CallDelegatePFP11objc_objectS0_P13objc_selectorzEP7WebViewS0_S2_S0_ | -[FrameLoadDelegate webView:didFinishLoadForFrame:] | -[FrameLoadDelegate webView:locationChangeDone:forDataSource:] | dump() | createXMLStringFromWebArchiveData(__CFData const*) | CFPropertyListCreateFromStream | _CFPropertyListCreateFromXMLData | __CFTryParseBinaryPlist | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | CFDataAppendBytes | CFDataReplaceBytes | __CFDataGrow | malloc_zone_realloc
> Leak: 0x90cc000  size=2048  string 'GIF89a4'
>     Call stack: [thread 0xa0855720]: | 0x2 | start | main | dumpRenderTree(int, char const**) | __ZL20runTestingServerLoopv | __ZL7runTestRKSs | -[NSRunLoop(NSRunLoop) runMode:beforeDate:] | CFRunLoopRunInMode | CFRunLoopRunSpecific | __ZN7WebCoreL10timerFiredEP16__CFRunLoopTimerPv | WebCore::ThreadTimers::sharedTimerFired() | WebCore::ThreadTimers::sharedTimerFiredInternal() | WebCore::DOMTimer::fired() | WebCore::ScheduledAction::execute(WebCore::ScriptExecutionContext*) | WebCore::ScheduledAction::execute(WebCore::Document*) | WebCore::ScriptController::executeScriptInWorld(WebCore::DOMWrapperWorld*, WTF::String const&, bool, WebCore::ShouldAllowXSS) | WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*, WebCore::ShouldAllowXSS) | WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue) | JSC::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue) | JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::ScopeChainNode*, JSC::JSObject*, JSC::JSValue*) | JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*, JSC::JSValue*) | jscGeneratedNativeCode | cti_op_call_NotJSFunction | JSC::JSCallbackFunction::call(JSC::ExecState*) | __ZL18notifyDoneCallbackPK15OpaqueJSContextP13OpaqueJSValueS3_mPKPKS2_PS5_ | LayoutTestController::notifyDone() | dump() | createXMLStringFromWebArchiveData(__CFData const*) | CFPropertyListCreateFromStream | _CFPropertyListCreateFromXMLData | __CFTryParseBinaryPlist | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | CFDataAppendBytes | CFDataReplaceBytes | __CFDataGrow | malloc_zone_realloc
> 
> It's always these two leaks. Does that ring a bell? I'm not familiar with CF at all unfortunately.

Forgot to ask, shall I still land this patch? I think the leak is not my fault...

[Nikolas Zimmermann]

Hah, same leaks on Snow Leopard: http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r70618%20(12096)/DumpRenderTree7-leaks.txt

Going to land my patch, but it needs to be investigated.
I think the CFValuesAppend is the problem, leaving this to Adam/David.

[Nikolas Zimmermann]

(In reply to comment #23)
> Hah, same leaks on Snow Leopard: http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r70618%20(12096)/DumpRenderTree7-leaks.txt
> 
> Going to land my patch, but it needs to be investigated.
> I think the CFValuesAppend is the problem, leaving this to Adam/David.

Landed in r70629. Reopining bug, as the new code leaks.

[Adam Roben (:aroben)]

(In reply to comment #19)
>     Call stack: [thread 0xa0855720]: | 0x2 | start | main | dumpRenderTree(int, char const**) | __ZL20runTestingServerLoopv | __ZL7runTestRKSs | -[NSRunLoop(NSRunLoop) runMode:beforeDate:] | CFRunLoopRunInMode | CFRunLoopRunSpecific | MultiplexerSource::perform() | URLConnectionClient::processEvents() | URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload(XConnectionEventInfo<XClientEvent, XClientEventParams>*, long) | URLConnectionClient::_clientDidFinishLoading(URLConnectionClient::ClientConnectionEventQueue*) | _NSURLConnectionDidFinishLoading | -[NSURLConnection(NSURLConnectionReallyInternal) sendDidFinishLoading] | -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] | WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) | WebCore::MainResourceLoader::didFinishLoading(double) | WebCore::FrameLoader::finishedLoading() | WebCore::FrameLoader::checkLoadComplete() | WebCore::FrameLoader::recursiveCheckLoadComplete() | WebCore::FrameLoader::checkLoadCompleteForThisFrame() | WebFrameLoaderClient::dispatchDidFinishLoad() | CallFrameLoadDelegate(objc_object* (*)(objc_object*, objc_selector*, ...), WebView*, objc_selector*, objc_object*) | __ZL12CallDelegatePFP11objc_objectS0_P13objc_selectorzEP7WebViewS0_S2_S0_ | -[FrameLoadDelegate webView:didFinishLoadForFrame:] | -[FrameLoadDelegate webView:locationChangeDone:forDataSource:] | dump() | createXMLStringFromWebArchiveData(__CFData const*) | CFPropertyListCreateFromStream | _CFPropertyListCreateFromXMLData | __CFTryParseBinaryPlist | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | __CFBinaryPlistCreateObject2 | CFDataAppendBytes | CFDataReplaceBytes | __CFDataGrow | malloc_zone_realloc

Looks like we're leaking the whole property list. RetainPtr should fix this pretty easily.

[David Kilzer (:ddkilzer)]

(In reply to comment #24)
> (In reply to comment #23)
> > Going to land my patch, but it needs to be investigated.
> > I think the CFValuesAppend is the problem, leaving this to Adam/David.
> 
> Landed in r70629. Reopining bug, as the new code leaks.

Thanks!  I'll look into the leak.

(In reply to comment #25)
> Looks like we're leaking the whole property list. RetainPtr should fix this pretty easily.

Yikes.  I wonder why I didn't use RetainPtr<> there?!  Testing a patch now.

[David Kilzer (:ddkilzer)]

Created attachment 72032 [details]
Fix leak of CFMutableDictionaryRef in createXMLStringFromWebArchiveData()

[WebKit Commit Bot]

Comment on attachment 72032 [details]
Fix leak of CFMutableDictionaryRef in createXMLStringFromWebArchiveData()

Clearing flags on attachment: 72032

Committed r70647: <http://trac.webkit.org/changeset/70647>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.