Bug 210814

Summary: Clean up QuickLookThumbnailLoader
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebKit2Assignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, nikosm, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=211160
Bug Depends on: 208891, 210894    
Bug Blocks: 210974, 211660    
Attachments:
Description Flags
Patch v1
none
Patch for landing
none
TestWebKitAPI.WKAttachmentTests crash log
none
Patch for landing to retry EWS
none
Patch for landing to retry EWS without QLThumbnailGenerationRequest RetainPtr<>
none
Patch for landing v4 none

Description David Kilzer (:ddkilzer) 2020-04-21 13:34:08 PDT
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.
Comment 1 David Kilzer (:ddkilzer) 2020-04-21 14:50:10 PDT
Created attachment 397126 [details]
Patch v1
Comment 2 Darin Adler 2020-04-21 14:57:06 PDT
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?
Comment 3 Tim Horton 2020-04-21 15:02:58 PDT
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
Comment 4 David Kilzer (:ddkilzer) 2020-04-21 17:12:14 PDT
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.
Comment 5 David Kilzer (:ddkilzer) 2020-04-21 17:12:39 PDT
Created attachment 397144 [details]
Patch for landing
Comment 6 Tim Horton 2020-04-21 17:27:29 PDT
(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.
Comment 7 David Kilzer (:ddkilzer) 2020-04-21 17:45:44 PDT
(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.
Comment 8 EWS 2020-04-21 17:51:52 PDT
Committed r260478: <https://trac.webkit.org/changeset/260478>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397144 [details].
Comment 9 Radar WebKit Bug Importer 2020-04-21 17:52:18 PDT
<rdar://problem/62141879>
Comment 10 Ryan Haddad 2020-04-21 20:52:24 PDT
(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
Comment 11 Ryan Haddad 2020-04-21 21:05:54 PDT
Reverted r260478 for reason:

Caused TestWebKitAPI.WKAttachmentTests crashes on Catalina

Committed r260498: <https://trac.webkit.org/changeset/260498>
Comment 12 Ryan Haddad 2020-04-21 21:06:51 PDT
Created attachment 397162 [details]
TestWebKitAPI.WKAttachmentTests crash log
Comment 13 Ryan Haddad 2020-04-21 21:09:37 PDT
(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
Comment 14 David Kilzer (:ddkilzer) 2020-04-22 16:33:32 PDT
Created attachment 397285 [details]
Patch for landing to retry EWS
Comment 15 David Kilzer (:ddkilzer) 2020-04-22 16:35:40 PDT
(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.
Comment 16 David Kilzer (:ddkilzer) 2020-04-23 01:43:29 PDT
Created attachment 397329 [details]
Patch for landing to retry EWS without QLThumbnailGenerationRequest RetainPtr<>
Comment 17 David Kilzer (:ddkilzer) 2020-04-23 10:10:41 PDT
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.
Comment 18 Darin Adler 2020-04-23 10:12:57 PDT
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?
Comment 19 David Kilzer (:ddkilzer) 2020-04-23 10:54:06 PDT
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.
Comment 20 David Kilzer (:ddkilzer) 2020-04-23 10:58:54 PDT
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.
Comment 21 Darin Adler 2020-04-23 11:02:28 PDT
(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.
Comment 22 Darin Adler 2020-04-23 11:03:05 PDT
You were using the phrase "without using RetainPtr", but you meant "without fixing the leak". That’s what confused me.
Comment 23 David Kilzer (:ddkilzer) 2020-04-23 13:47:51 PDT
(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.
Comment 24 David Kilzer (:ddkilzer) 2020-04-23 13:52:19 PDT
Created attachment 397380 [details]
Patch for landing v4
Comment 25 EWS 2020-04-23 17:46:22 PDT
Committed r260611: <https://trac.webkit.org/changeset/260611>

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