Bug 210814 – Clean up QuickLookThumbnailLoader

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Clean up QuickLookThumbnailLoader.

The clang static analyzer noticed two new potential leaks in r260407 for Bug 208891:

'WKQLThumbnailQueueManager' lacks a 'dealloc' instance method but must release '_qlThumbnailGenerationQueue'
'WKQLThumbnailLoadOperation' lacks a 'dealloc' instance method but must release '_contentType'

Neither leak would occur in practice because WKQLThumbnailLoadOperation._contentType was completely unused(!), and WKQLThumbnailQueueManager uses a +sharedInstance pattern that would likely never deallocate the shared object instance that is created.

However, after investigating the leaks, I found some other things that could be cleaned up.

[David Kilzer (:ddkilzer)]

Created attachment 397126 [details]
Patch v1

[Darin Adler]

Comment on attachment 397126 [details]
Patch v1

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

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:33
>  #if PLATFORM(IOS_FAMILY)
>  @class UIImage;
> +using PlatformQLImage = UIImage;
> +#elif PLATFORM(MAC)
> +using PlatformQLImage = NSImage;
>  #endif

I’d like to see this somewhere in PAL or WTF where we can share it for other Cocoa-specific files. Along with other types that just need UIKit/AppKit typedefs. Not sure at all that I like the word "Platform" in these type names. PlatformQLImage does not seem like an improvement to me. I’m thinking it should be this:

    #if USE(APPKIT)
    OBJC_CLASS(NSImage);
    using CocoaImage = NSImage;
    #else
    OBJC_CLASS(UIImage);
    using CocoaImage = UIImage;
    #endif

In some appropriate header file.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:37
> +@property (nonatomic, readonly, retain) NSOperationQueue *qlThumbnailGenerationQueue;

This property name really doesn’t need the "ql" prefix. In fact, given the class it’s in, maybe it should be named "queue".

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:46
> +    [_qlThumbnailGenerationQueue release];

!!!

I guess this is why you’re here?

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:64
> +@interface WKQLThumbnailLoadOperation ()
> +@property (atomic, readwrite, getter=isExecuting) BOOL executing;
> +@property (atomic, readwrite, getter=isFinished) BOOL finished;
> +@property (nonatomic, readwrite, copy) NSString *identifier;
> +@property (nonatomic, readwrite) BOOL shouldWrite;
> +@property (nonatomic, readwrite, retain) PlatformQLImage *thumbnail;
> +@end

I don’t understand this. Why does this all needed to be repeated?

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:98
> -    if (_shouldWrite) {
> +
> +    if (self.shouldWrite) {

This seems to make things worse, not better. Why would a class use a property to access its own instance variables? Maybe if it’s an object that needs to be retained/released, but outside of that it does not seem great.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:125
>  #if PLATFORM(IOS_FAMILY)
> -        _thumbnail = thumbnail.UIImage;
> +        self.thumbnail = thumbnail.UIImage;
> +#elif PLATFORM(MAC)
> +        self.thumbnail = thumbnail.NSImage;
>  #endif

I’d suggest USE(APPKIT) rather than PLATFORM(MAC) and PLATFORM(IOS_FAMILY).

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:141
> +    if (![_thumbnail isEqual:thumbnail])

This seems like a surprising thing to do. Is it really normal to compare the content of images when setting them?

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:152
> +    if (![_identifier isEqualToString:identifier])

This seems OK, but is it necessary?

[Tim Horton]

Comment on attachment 397126 [details]
Patch v1

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

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:30
> +using PlatformQLImage = UIImage;

I think this can just be PlatformImage. Or if you must include the QuickLook, spell it out, instead of QL.

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:46
> +    [_qlThumbnailGenerationQueue release];

Why not use RetainPtr instead?

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:113
> +    QLThumbnailGenerationRequest *request = [[QLThumbnailGenerationRequest alloc] initWithFileAtURL:_filePath.get() size:CGSizeMake(400, 400) scale:1 representationTypes:QLThumbnailGenerationRequestRepresentationTypeAll];

What frees this? Should this be RetainPtr'd?

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:141
> +    if (![_thumbnail isEqual:thumbnail])

Early return might be more WebKit-style

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:152
> +    if (![_identifier isEqualToString:identifier])

Ditto

[David Kilzer (:ddkilzer)]

Comment on attachment 397126 [details]
Patch v1

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

>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:30
>> +using PlatformQLImage = UIImage;
> 
> I think this can just be PlatformImage. Or if you must include the QuickLook, spell it out, instead of QL.

Okay.  I thought PlatformImage was too generic (and reminded me of old class/file names in WebCore).  Will go with CocoaImage per Darin's suggestion below.

>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:33
>>  #endif
> 
> I’d like to see this somewhere in PAL or WTF where we can share it for other Cocoa-specific files. Along with other types that just need UIKit/AppKit typedefs. Not sure at all that I like the word "Platform" in these type names. PlatformQLImage does not seem like an improvement to me. I’m thinking it should be this:
> 
>     #if USE(APPKIT)
>     OBJC_CLASS(NSImage);
>     using CocoaImage = NSImage;
>     #else
>     OBJC_CLASS(UIImage);
>     using CocoaImage = UIImage;
>     #endif
> 
> In some appropriate header file.

Will change from PlatformQLImage to CocoaImage, but will move to another shared header in a different patch.

A quick recursive grep shows there aren't as many places as you might think that would benefit from this.

>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.h:37
>> +@property (nonatomic, readonly, retain) NSOperationQueue *qlThumbnailGenerationQueue;
> 
> This property name really doesn’t need the "ql" prefix. In fact, given the class it’s in, maybe it should be named "queue".

Will change to `queue`.

>>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:46
>>> +    [_qlThumbnailGenerationQueue release];
>> 
>> !!!
>> 
>> I guess this is why you’re here?
> 
> Why not use RetainPtr instead?

Yes, the whole reason this patch exists is due to clang static analyzer warnings.

--

Using RetainPtr<> seems more complex than a simple dealloc method because I have to declare the instance variable, manually implement the setter, then use adoptNS() in the -init method vs. just implementing a simple -dealloc method.

When we switch to ARC, the -dealloc method can be wholly deleted with no other edits, while RetainPtr<> becomes additional unnecessary complexity.

I'm trying to skate to where the puck will be, although the ARC puck hasn't been getting any closer recently.  :(

>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:64
>> +@end
> 
> I don’t understand this. Why does this all needed to be repeated?

These make the properties read-write within the source file, while being read-only for another classes.

There are two benefits:
1. For atomic properties, you do not get atomic behavior if you set the property directly.
2. For `_identifier`, you get consistent -copy behavior (as declared in the @property) rather than having to remember to call -copy each time it's assigned internally.

I included the other properties for consistency with the rest of the read-only external properties.

I'll just get rid of the non-atomic ones.

>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:98
>> +    if (self.shouldWrite) {
> 
> This seems to make things worse, not better. Why would a class use a property to access its own instance variables? Maybe if it’s an object that needs to be retained/released, but outside of that it does not seem great.

Okay, I'll change it back.

>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:113
>> +    QLThumbnailGenerationRequest *request = [[QLThumbnailGenerationRequest alloc] initWithFileAtURL:_filePath.get() size:CGSizeMake(400, 400) scale:1 representationTypes:QLThumbnailGenerationRequestRepresentationTypeAll];
> 
> What frees this? Should this be RetainPtr'd?

Yes, nice catch.

>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:125
>>  #endif
> 
> I’d suggest USE(APPKIT) rather than PLATFORM(MAC) and PLATFORM(IOS_FAMILY).

Will fix.

>>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:141
>>> +    if (![_thumbnail isEqual:thumbnail])
>> 
>> This seems like a surprising thing to do. Is it really normal to compare the content of images when setting them?
> 
> Early return might be more WebKit-style

Using the class' -isEqual: (or -isEqualTo:) is generally a best-practice for implementing setters, but you're right, this is probably overkill for a UIImage.

I'll remove this.

>>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:152
>>> +    if (![_identifier isEqualToString:identifier])
>> 
>> This seems OK, but is it necessary?
> 
> Ditto

I'll remove this.

[David Kilzer (:ddkilzer)]

Created attachment 397144 [details]
Patch for landing

[Tim Horton]

(In reply to David Kilzer (:ddkilzer) from comment #4)
> Comment on attachment 397126 [details]
> 
> >>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:141
> >>> +    if (![_thumbnail isEqual:thumbnail])
> >> 
> >> This seems like a surprising thing to do. Is it really normal to compare the content of images when setting them?
> > 
> > Early return might be more WebKit-style
> 
> Using the class' -isEqual: (or -isEqualTo:) is generally a best-practice for
> implementing setters, but you're right, this is probably overkill for a
> UIImage.
> 
> I'll remove this.

FWIW while I agree it's probably unnecessary, I'm pretty sure both UIImage and NSImage isEqual just do pointer comparison, not actual image data comparison, so it's probably not extremely overkill.

[David Kilzer (:ddkilzer)]

(In reply to Tim Horton from comment #6)
> (In reply to David Kilzer (:ddkilzer) from comment #4)
> > Comment on attachment 397126 [details]
> > 
> > >>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:141
> > >>> +    if (![_thumbnail isEqual:thumbnail])
> > >> 
> > >> This seems like a surprising thing to do. Is it really normal to compare the content of images when setting them?
> > > 
> > > Early return might be more WebKit-style
> > 
> > Using the class' -isEqual: (or -isEqualTo:) is generally a best-practice for
> > implementing setters, but you're right, this is probably overkill for a
> > UIImage.
> > 
> > I'll remove this.
> 
> FWIW while I agree it's probably unnecessary, I'm pretty sure both UIImage
> and NSImage isEqual just do pointer comparison, not actual image data
> comparison, so it's probably not extremely overkill.

I looked.  UIImage is much more complex than just a pointer comparison.  I couldn't find an NSImage implementation, so it probably defaults to NSObject's pointer comparison.

[EWS]

Committed r260478: <https://trac.webkit.org/changeset/260478>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397144 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62141879>

[Ryan Haddad]

(In reply to EWS from comment #8)
> Committed r260478: <https://trac.webkit.org/changeset/260478>
This changed caused 15 TestWebKitAPI.WKAttachmentTests to crash (Catalina only, so EWS didn't flag this): https://build.webkit.org/builders/Apple-Catalina-Release-WK1-Tests/builds/5096

[Ryan Haddad]

Reverted r260478 for reason:

Caused TestWebKitAPI.WKAttachmentTests crashes on Catalina

Committed r260498: <https://trac.webkit.org/changeset/260498>

[Ryan Haddad]

Created attachment 397162 [details]
TestWebKitAPI.WKAttachmentTests crash log

[Ryan Haddad]

(In reply to Ryan Haddad from comment #10)
> (In reply to EWS from comment #8)
> > Committed r260478: <https://trac.webkit.org/changeset/260478>
> This changed caused 15 TestWebKitAPI.WKAttachmentTests to crash (Catalina
> only, so EWS didn't flag this):
Correction: iOS Simulator too, but that EWS bot didn't finish running before this landed.
https://build.webkit.org/builders/Apple%20iOS%2013%20Simulator%20Release%20WK2%20%28Tests%29/builds/3896

[David Kilzer (:ddkilzer)]

Created attachment 397285 [details]
Patch for landing to retry EWS

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #14)
> Created attachment 397285 [details]
> Patch for landing to retry EWS

Could not reproduce crashes running an iOS Simulator release build (with or without --iterations 5) with the patch reapplied on r260531:

$ ./Tools/Scripts/run-api-tests --release --no-build --iterations 5 TestWebKitAPI.WKAttachmentTests 

Trying EWS again to see what it says.

[David Kilzer (:ddkilzer)]

Created attachment 397329 [details]
Patch for landing to retry EWS without QLThumbnailGenerationRequest RetainPtr<>

[David Kilzer (:ddkilzer)]

Comment on attachment 397329 [details]
Patch for landing to retry EWS without QLThumbnailGenerationRequest RetainPtr<>

I'm considering landing this patch without the RetainPtr<> for KQLThumbnailLoadOperation, and file a follow-up bug for that issue.

I wonder if we need to keep it alive until the block is deallocated.

[Darin Adler]

Comment on attachment 397329 [details]
Patch for landing to retry EWS without QLThumbnailGenerationRequest RetainPtr<>

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

> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:109
> +    QLThumbnailGenerationRequest *request = [[QLThumbnailGenerationRequest alloc] initWithFileAtURL:_filePath.get() size:CGSizeMake(400, 400) scale:1 representationTypes:QLThumbnailGenerationRequestRepresentationTypeAll];

Where is this object released?

[David Kilzer (:ddkilzer)]

Comment on attachment 397329 [details]
Patch for landing to retry EWS without QLThumbnailGenerationRequest RetainPtr<>

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

>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:109
>> +    QLThumbnailGenerationRequest *request = [[QLThumbnailGenerationRequest alloc] initWithFileAtURL:_filePath.get() size:CGSizeMake(400, 400) scale:1 representationTypes:QLThumbnailGenerationRequestRepresentationTypeAll];
> 
> Where is this object released?

That's the point--it's not released.  It's leaked.  But when I made it a RetainPtr<>, it caused 15 crashes on API tests for TestWebKitAPI.WKAttachmentTests (which I could not reproduce locally).

So I was planning to commit this fix without the RetainPtr<> change, and then fix that in a follow-up.

[David Kilzer (:ddkilzer)]

Comment on attachment 397329 [details]
Patch for landing to retry EWS without QLThumbnailGenerationRequest RetainPtr<>

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

>>> Source/WebKit/UIProcess/QuickLookThumbnailLoader.mm:109
>>> +    QLThumbnailGenerationRequest *request = [[QLThumbnailGenerationRequest alloc] initWithFileAtURL:_filePath.get() size:CGSizeMake(400, 400) scale:1 representationTypes:QLThumbnailGenerationRequestRepresentationTypeAll];
>> 
>> Where is this object released?
> 
> That's the point--it's not released.  It's leaked.  But when I made it a RetainPtr<>, it caused 15 crashes on API tests for TestWebKitAPI.WKAttachmentTests (which I could not reproduce locally).
> 
> So I was planning to commit this fix without the RetainPtr<> change, and then fix that in a follow-up.

> So I was planning to commit this fix without the RetainPtr<> change, and then fix that in a follow-up.

Because this leaked before this patch, so this patch doesn't make that scenario any worse.

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #20)
> Because this leaked before this patch, so this patch doesn't make that
> scenario any worse.

Got it.

One way to make that clear is to file a new bug saying the object leaks. Can do that now. Doesn’t matter when we plan on fixing it. No need to wait to make the bug report.

[Darin Adler]

You were using the phrase "without using RetainPtr", but you meant "without fixing the leak". That’s what confused me.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #22)
> You were using the phrase "without using RetainPtr", but you meant "without
> fixing the leak". That’s what confused me.

Sorry.  It's been a long week.

In good news, I think Nikos may have fixed the over-release in r260581 for Bug 210894, which was this change:

-        auto fileURLPath = adoptNS([NSURL fileURLWithPath:filePath]);
+        auto fileURLPath = [NSURL fileURLWithPath:filePath];

He also fixed the QLThumbnailGenerationRequest leak:

-    QLThumbnailGenerationRequest *req = [[QLThumbnailGenerationRequest alloc] initWithFileAtURL:_filePath.get() size:CGSizeMake(400, 400) scale:1 represe
ntationTypes:QLThumbnailGenerationRequestRepresentationTypeAll];
+    auto req = adoptNS([WebKit::allocQLThumbnailGenerationRequestInstance() initWithFileAtURL:_filePath.get() size:CGSizeMake(400, 400) scale:1 represent
ationTypes:QLThumbnailGenerationRequestRepresentationTypeAll]);

So I will post a rebased patch (with just the req -> request rename plus the other unrelated changes here) to check that the API tests don't crash, then commit that.

[David Kilzer (:ddkilzer)]

Created attachment 397380 [details]
Patch for landing v4

[EWS]

Committed r260611: <https://trac.webkit.org/changeset/260611>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397380 [details].