[webkit-reviews] review granted: [Bug 120230] Fix issues found by the Clang Static Analyzer : [Attachment 209531] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 120230: Fix issues found by the Clang Static Analyzer
https://bugs.webkit.org/show_bug.cgi?id=120230

Attachment 209531: Patch
https://bugs.webkit.org/attachment.cgi?id=209531&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=209531&action=review


> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:610
> +    [invocation retainArguments];

This is clearly right for the target, but what about other arguments? Is
retainArguments OK for all of them?

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:611
> +    [invocation setTarget:[[target copy] autorelease]];

Why autorelease instead of plain old release? I suggest using a local variable
and release instead.

> Source/WTF/wtf/ObjcRuntimeExtras.h:32
>  RetType wtfObjcMsgSend(id target, SEL selector, ArgTypes... args)

I’m surprised this is not marked inline. Same for wtCallIMP.

> Source/WTF/wtf/ObjcRuntimeExtras.h:49
> +inline id wtfHardAutorelease(CFTypeRef object)

The wtf prefix on this is super-ugly. Do we really need it? Lets just not
prefix this.

> Source/WTF/wtf/ObjcRuntimeExtras.h:54
> +    if (object)
> +	   CFMakeCollectable(object);
> +    [(id)object autorelease];
> +    return (id)object;

We should wrap the call to CFMakeCollectable inside #ifndef OBJC_NO_GC.

Is this the correct implementation under ARC?

> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:698
>      UInt8* urlBytes;
>      UInt8 buffer[2048];
> +    OwnArrayPtr<UInt8> bufferOnHeap;
>      CFIndex numBytes = CFURLGetBytes((CFURLRef)URL, buffer, 2048);
>      if (numBytes == -1) {
>	   numBytes = CFURLGetBytes((CFURLRef)URL, NULL, 0);
> -	   urlBytes = static_cast<UInt8*>(malloc(numBytes));
> +	   bufferOnHeap = adoptArrayPtr(new UInt8[numBytes]);
> +	   urlBytes = bufferOnHeap.get();
>	   CFURLGetBytes((CFURLRef)URL, urlBytes, numBytes);
>      } else
>	   urlBytes = buffer;

We created the Vector class and its inline capacity specifically to avoid
having to write things like this. It should be more like this:

    Vector<UInt8, 2048> buffer(2048);
    CFIndex numBytes = CFURLGetBytes((CFURLRef)URL, buffer.get(), 2048);
    if (numBytes == -1) {
	numBytes = CFURLGetBytes((CFURLRef)URL, NULL, 0);
	buffer.grow(numBytes);
	CFURLGetBytes((CFURLRef)URL, buffer.get(), numBytes);
    }
    UInt8* urlBytes = buffer.get();

> Source/WebKit/mac/Plugins/Hosted/WebHostedNetscapePluginView.mm:168
> -	   _pluginLayer.get().backgroundColor = CGColorCreateGenericRGB(1, 0,
1, 1);
> +	   _pluginLayer.get().backgroundColor =
(CGColorRef)wtfHardAutorelease(CGColorCreateGenericRGB(1, 0, 1, 1));

I don’t think we want autorelease here. I think we want plain old release. If
you want to use RetainPtr, the way to write it is this:

    _pluginLayer.get().backgroundColor = adoptCF(CGColorCreateGenericRGB(1, 0,
1, 1)).get();

If not, then I suggest a local variable and an explicit call to CGColorRelease.
CF doesn’t have autorelease, and we should not use HardAutorelease for a case
like where we are not bridging to Objective-C.

Or we could stick this into a global and just fetch it the first time this
function runs.

> Source/WebKit/mac/WebView/WebView.mm:6341
> -	   WebCFAutorelease(self);
> +	   wtfHardAutorelease(self);

This seems wrong. Should just be:

    [self autorelease];

> Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.mm:59
> +    return (NSURL *)wtfHardAutorelease(WKURLCopyCFURL(kCFAllocatorDefault,
wkURL.get()));

The cast to (NSURL *) is no longer required and should be omitted.

> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm:211
> +	   return (NSString
*)wtfHardAutorelease(WKStringCopyCFString(kCFAllocatorDefault,
(WKStringRef)result.get()));

The cast to (NSString *) is no longer required and should be omitted.

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:33
> +#import <wtf/RetainPtr.h>

Can we just compile with with ARC instead?