Bug 120230 – Fix issues found by the Clang Static Analyzer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

Fix issues found by the Clang Static Analyzer

[Radar WebKit Bug Importer]

<rdar://problem/14822738>

[Andy Estes]

Created attachment 209512 [details]
Patch

[Build Bot]

Comment on attachment 209512 [details]
Patch

Attachment 209512 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1546419

[Andy Estes]

Created attachment 209531 [details]
Patch

[Darin Adler]

Comment on attachment 209531 [details]
Patch

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?

[Andy Estes]

(In reply to comment #5)

Thanks for the thorough review!

> (From update of attachment 209531 [details])
> 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?

Yes, I think it's okay. Previously we weren't retaining the arguments, relying instead on invocation happening immediately after setting arguments. I think letting the NSInvocation retain and release the object arguments makes things more explicit.

> > 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.

No good reason. I'll implement your suggestion.

> > 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.

I changed these to be inline.

> > 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.

Sure, I'll remove it.

> > 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.

Ok.

> Is this the correct implementation under ARC?

Good point. It isn't correct if the function's inlined into a ARC-enabled translation unit. I changed the function to this:

inline id HardAutorelease(CFTypeRef object)
{
#ifndef OBJC_NO_GC
    if (object)
        CFMakeCollectable(object);
#elif !__has_feature(objc_arc)
    [(id)object autorelease];
#endif
    return (id)object;
}

> > 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();

Ok, I'll do this.

> 
> > 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.

I like using RetainPtr<>. I'll do that.

> > Source/WebKit/mac/WebView/WebView.mm:6341
> > -        WebCFAutorelease(self);
> > +        wtfHardAutorelease(self);
> 
> This seems wrong. Should just be:
> 
>     [self autorelease];

Done.

> > 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.

Done.

> > 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.

Done.

> > Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:33
> > +#import <wtf/RetainPtr.h>
> 
> Can we just compile with with ARC instead?

I could compile this single test with ARC, although I think I'd rather convert most/all the tests to ARC in a separate patch.

[Andy Estes]

Committed r154647: <http://trac.webkit.org/changeset/154647>

[Darin Adler]

Comment on attachment 209531 [details]
Patch

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

>>> Source/WTF/wtf/ObjcRuntimeExtras.h:54
>>> +    return (id)object;
>> 
>> We should wrap the call to CFMakeCollectable inside #ifndef OBJC_NO_GC.
>> 
>> Is this the correct implementation under ARC?
> 
> Ok.

The version of this you landed seems like it needs an UNUSED_PARAM in the non-GC, non-ARC case.

[Darin Adler]

Comment on attachment 209531 [details]
Patch

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

>>>> Source/WTF/wtf/ObjcRuntimeExtras.h:54
>>>> +    return (id)object;
>>> 
>>> We should wrap the call to CFMakeCollectable inside #ifndef OBJC_NO_GC.
>>> 
>>> Is this the correct implementation under ARC?
>> 
>> Ok.
> 
> The version of this you landed seems like it needs an UNUSED_PARAM in the non-GC, non-ARC case.

No, my mistake, it’s fine.