Bug 156956 – 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

[Andy Estes]

Created attachment 277172 [details]
Patch

[WebKit Commit Bot]

Attachment 277172 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:463:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:463:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 277172 [details]
Patch

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

Fun.

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:466
>  - (NSURLResponse *)response
>  {
> -    return _response.get();
> +    @synchronized (self) {
> +        return _response.get();
> +    }
>  }

I don't understand what good this is. The member variable should be aligned, so reading it is always atomic, no synchronization needed.

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:509
> +- (void)dealloc
> +{
> +    [_originalRequest release];
> +    [_currentRequest release];
> +    [_error release];
> +    [_taskDescription release];
> +    [super dealloc];
> +}

I know that Alex ran NSURLSession code through leaks, surprised that this wasn't found.

[Andy Estes]

(In reply to comment #2)
> Attachment 277172 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:463: 
> Place brace on its own line for function definitions.  [whitespace/braces]
> [4]
> ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:463: 
> Extra space before ( in function call  [whitespace/parens] [4]
> Total errors found: 2 in 19 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Filed https://bugs.webkit.org/show_bug.cgi?id=156957

[Andy Estes]

(In reply to comment #3)
> Comment on attachment 277172 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277172&action=review
> 
> Fun.
> 
> > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:466
> >  - (NSURLResponse *)response
> >  {
> > -    return _response.get();
> > +    @synchronized (self) {
> > +        return _response.get();
> > +    }
> >  }
> 
> I don't understand what good this is. The member variable should be aligned,
> so reading it is always atomic, no synchronization needed.

Perhaps you're right. I didn't think too much about this. I'll remove it.

> 
> > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:509
> > +- (void)dealloc
> > +{
> > +    [_originalRequest release];
> > +    [_currentRequest release];
> > +    [_error release];
> > +    [_taskDescription release];
> > +    [super dealloc];
> > +}
> 
> I know that Alex ran NSURLSession code through leaks, surprised that this
> wasn't found.

I think this is code added by Jer for AVFoundation loading, perhaps written after Alex did his check.

Thanks for the review!

[Andy Estes]

Created attachment 277174 [details]
patch for landing

[Andy Estes]

Created attachment 277175 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 277175 [details]
patch for landing

Clearing flags on attachment: 277175

Committed r199960: <http://trac.webkit.org/changeset/199960>

[Joseph Pecoraro]

Nice changes!

Closing this bug as it landed a while ago.