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.
Created attachment 397126 [details] Patch v1
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 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 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.
Created attachment 397144 [details] Patch for landing
(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.
(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.
Committed r260478: <https://trac.webkit.org/changeset/260478> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397144 [details].
<rdar://problem/62141879>
(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
Reverted r260478 for reason: Caused TestWebKitAPI.WKAttachmentTests crashes on Catalina Committed r260498: <https://trac.webkit.org/changeset/260498>
Created attachment 397162 [details] TestWebKitAPI.WKAttachmentTests crash log
(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
Created attachment 397285 [details] Patch for landing to retry EWS
(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.
Created attachment 397329 [details] Patch for landing to retry EWS without QLThumbnailGenerationRequest RetainPtr<>
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 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 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 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.
(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.
You were using the phrase "without using RetainPtr", but you meant "without fixing the leak". That’s what confused me.
(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.
Created attachment 397380 [details] Patch for landing v4
Committed r260611: <https://trac.webkit.org/changeset/260611> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397380 [details].