Bug 185887 – Fix issues with -dealloc methods found by clang static analyzer

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

[David Kilzer (:ddkilzer)]

The clang static analyzer warns about missing -dealloc methods and missing calls to release instance variables in WebKit.

[Radar WebKit Bug Importer]

<rdar://problem/40463929>

[David Kilzer (:ddkilzer)]

Created attachment 341026 [details]
Patch v1

[Joseph Pecoraro]

Comment on attachment 341026 [details]
Patch v1

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

> Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.mm:69
> +@property (nonatomic, assign) UIVisualEffectView *secondaryMaterialOverlayView;

Everything looks good except this seems totally sketchy. Should we just make it a (retain) computed member variable, or a method?

[EWS Watchlist]

Comment on attachment 341026 [details]
Patch v1

Attachment 341026 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7770535

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
http/tests/preload/onload_event.html
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 341048 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[David Kilzer (:ddkilzer)]

Comment on attachment 341026 [details]
Patch v1

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

>> Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.mm:69
>> +@property (nonatomic, assign) UIVisualEffectView *secondaryMaterialOverlayView;
> 
> Everything looks good except this seems totally sketchy. Should we just make it a (retain) computed member variable, or a method?

I almost made it `retain` but wasn’t sure if it would create a retain cycle since this view gets added as a subview of this object in -updateConstraints.

Maybe make it `weak`?  They would require a little code restructuring in -updateConstraints the way it assigns the object to the property now.

[Anders Carlsson]

Wouldn’t it be better to use RetainPtr where possible instead of using explicit calls to retain and release?

[David Kilzer (:ddkilzer)]

(In reply to Anders Carlsson from comment #7)
> Wouldn’t it be better to use RetainPtr where possible instead of using
> explicit calls to retain and release?

Won’t we just remove RetainPtr use for Objective-C objects once we enable ARC?

Seems like more editing is required to switch from RetainPtr to ARC as well, especially for instance variables.

[David Kilzer (:ddkilzer)]

Created attachment 341109 [details]
Patch v2

[Joseph Pecoraro]

Comment on attachment 341109 [details]
Patch v2

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

> Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.mm:69
> +@property (nonatomic, retain) UIVisualEffectView *secondaryMaterialOverlayView;

Err, now this is weird because nothing assigns to _secondaryMaterialOverlayView. The getter always returns a new instance, and hopefully nobody uses the setter because the setter/getter would return different values.

The existing implementation doesn't seem suitable to a property since it creates a new instance each time, nor does it appear to want a setter.

[David Kilzer (:ddkilzer)]

Comment on attachment 341109 [details]
Patch v2

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

>> Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.mm:69
>> +@property (nonatomic, retain) UIVisualEffectView *secondaryMaterialOverlayView;
> 
> Err, now this is weird because nothing assigns to _secondaryMaterialOverlayView. The getter always returns a new instance, and hopefully nobody uses the setter because the setter/getter would return different values.
> 
> The existing implementation doesn't seem suitable to a property since it creates a new instance each time, nor does it appear to want a setter.

The assignment to _secondaryMaterialOverlayView happens in -updateConstraints via olde-fashioned setter invocation:

            [self setSecondaryMaterialOverlayView:[[self class] secondaryMaterialOverlayView]];

The "getter" you're looking at is a class method, not an instance method:

+ (UIVisualEffectView *)secondaryMaterialOverlayView

The -updateConstraints method also uses this value later:

        [_secondaryMaterialOverlayView removeFromSuperview];

So this seems like a valid use of an instance variable (with a questionable design pattern).  (I'm going to send email to Jer Noble to find out if the `else` clause that calls -removeFromSuperview should also clear out _secondaryMaterialOverlayView, but that seems like a separate concern.  Maybe.)

If it's easier, I can just drop the changes to the secondaryMaterialOverlayView & secondaryMaterialOverlayViewConstraints properties so we can fix the other leaks.

[David Kilzer (:ddkilzer)]

I spoke with Joe in person today, and he said it was okay to land this without the changes to the secondaryMaterialOverlayView & secondaryMaterialOverlayViewConstraints properties, and then follow-up with a separate bug about fixing those.

[Joseph Pecoraro]

(In reply to David Kilzer (:ddkilzer) from comment #12)
> I spoke with Joe in person today, and he said it was okay to land this
> without the changes to the secondaryMaterialOverlayView &
> secondaryMaterialOverlayViewConstraints properties, and then follow-up with
> a separate bug about fixing those.

Yep. If you want to re-open Patch v1 I'll r+ it.

[David Kilzer (:ddkilzer)]

(In reply to Joseph Pecoraro from comment #13)
> (In reply to David Kilzer (:ddkilzer) from comment #12)
> > I spoke with Joe in person today, and he said it was okay to land this
> > without the changes to the secondaryMaterialOverlayView &
> > secondaryMaterialOverlayViewConstraints properties, and then follow-up with
> > a separate bug about fixing those.
> 
> Yep. If you want to re-open Patch v1 I'll r+ it.

Done.

[Joseph Pecoraro]

Comment on attachment 341026 [details]
Patch v1

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

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:152
> +    [_location release];

I notice that `location` is `@property (retain, nonatomic) NSString *location;` normally NSStrings are (copy) not (retain). Seems we could be doing some cleanup / modernization around this code.

[David Kilzer (:ddkilzer)]

Committed r232187: <https://trac.webkit.org/changeset/232187>

[David Kilzer (:ddkilzer)]

(In reply to Joseph Pecoraro from comment #15)
> Comment on attachment 341026 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341026&action=review
> 
> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:152
> > +    [_location release];
> 
> I notice that `location` is `@property (retain, nonatomic) NSString
> *location;` normally NSStrings are (copy) not (retain). Seems we could be
> doing some cleanup / modernization around this code.

Made that change and also fixed _WKPreviewControllerDataSource.mimeType to be `copy`.