Memory usage spike and UI process hangs for several seconds when attaching a large JPEG in a web form. To reproduce: 1. Go to <http://imgur.com> 2. Click “Upload” 3. Click “device” 4. Select a very large JPEG from your photo library, e.g. <http://imgsrc.hubblesite.org/hu/db/images/hs-2015-12-a-full_jpg.jpg> You do not have to try to upload the file; selecting it will exhibit the behavior as the UI process attempts to generate an icon from the image. <rdar://problem/21206699>
Created attachment 254101 [details] Patch
Patch doesn’t apply because it depends on the changes from <https://bugs.webkit.org/show_bug.cgi?id=145539>.
Comment on attachment 254101 [details] Patch Nice! Looks good to me!
Comment on attachment 254101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254101&action=review Patch doesn’t apply so you should upload a rebased one. > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:93 > +static const CGSize iconSize = { 100.0f, 100.0f }; Would be nice to have a comment explaining where the 100 comes from, why it’s a good value. The use of 100.0f here is a little strange. In 64-bit it’s a double, not a float so you’d want “100.0”. But why not just say "100" instead? > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:154 > + (id)kCGImageSourceThumbnailMaxPixelSize: @(std::max(iconSize.width, iconSize.height)), Seems a bit strange to call max on two compile time constants. Not sure I really understand why this is correct.
(In reply to comment #4) > Comment on attachment 254101 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254101&action=review > > Patch doesn’t apply so you should upload a rebased one. This patch is on top of the changes from <https://bugs.webkit.org/show_bug.cgi?id=145539> which hasn't landed yet. I can reupload after that change lands. > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:93 > > +static const CGSize iconSize = { 100.0f, 100.0f }; > > Would be nice to have a comment explaining where the 100 comes from, why > it’s a good value. I'm not sure where the 100 comes from. It was already in the code, I just moved it to a global so it could be shared. > > The use of 100.0f here is a little strange. In 64-bit it’s a double, not a > float so you’d want “100.0”. But why not just say "100" instead? I didn't know that CGFloat was a double on 64-bit! I fixed this to be 100. > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:154 > > + (id)kCGImageSourceThumbnailMaxPixelSize: @(std::max(iconSize.width, iconSize.height)), > > Seems a bit strange to call max on two compile time constants. Not sure I > really understand why this is correct. I want the larger of the width and the height, so that we can select the optimal size of the thumbnail to generate/retrieve, although in practice I don't expect the width and the height to differ. Would it be better to have a global CGFloat squareIconSideLength?
Created attachment 254348 [details] Patch
Comment on attachment 254348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254348&action=review > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:95 > static CGRect squareCropRectForSize(CGSize size) Not part of this patch, but reading this function, I see incorrect use of the rintf function. It’s wrong in two ways: 1) It always converts to a float, which is not good when CGFloat is a double in 64-bit. 2) It rounds based on “the prevailing rounding mode”; this is not what we want and is why we normally use functions from the round family of functions, not the rint family. Since this is C++ code, the best solution is to use the std::round function, which is overloaded for both float and double. > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:131 > + CGRect destRect = { CGPointZero, CGSizeMake(iconSideLength, iconSideLength) }; The idiom here is a bit strange. Instead we could write one of these more idiomatic forms: CGRect destRect { 0, 0, iconSideLength, iconSideLength }; CGRect destRect = { 0, 0, iconSideLength, iconSideLength }; CGRect destRect = CGRectMake(0, 0, iconSideLength, iconSideLength); > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:164 > + return squareImage([UIImage imageWithCGImage:thumbnail.get()]); Seems like this could create an image smaller than 100x100; is that OK? Why not use thumbnailSizedImageForImage here? Should do no harm if the thumbnail is the right size and if it’s not exactly right it will correctly scale up. Code in function below uses initWithCGImage: and adoptNS rather than imageWithCGImage: and relying on autorelease. It’s not elegant to use two different idioms in these adjacent functions. Also seems that these functions now share more. In both cases, once they create a CGImageRef the rest is the same. I also will note that thumbnailSizedImageForImage and squareImage unnecessarily take a UIImage. All they do is extract the CGImageRef from it, which is ironic since all we do to make the UIImage is wrap the CGImageRef in it! I believe this points to a little refactoring that would improve things.
(In reply to comment #7) > Comment on attachment 254348 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254348&action=review > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:95 > > static CGRect squareCropRectForSize(CGSize size) > > Not part of this patch, but reading this function, I see incorrect use of > the rintf function. It’s wrong in two ways: > > 1) It always converts to a float, which is not good when CGFloat is a double > in 64-bit. > 2) It rounds based on “the prevailing rounding mode”; this is not what we > want and is why we normally use functions from the round family of > functions, not the rint family. > > Since this is C++ code, the best solution is to use the std::round function, > which is overloaded for both float and double. > Fixed to use std::round. > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:131 > > + CGRect destRect = { CGPointZero, CGSizeMake(iconSideLength, iconSideLength) }; > > The idiom here is a bit strange. Instead we could write one of these more > idiomatic forms: > > CGRect destRect { 0, 0, iconSideLength, iconSideLength }; > CGRect destRect = { 0, 0, iconSideLength, iconSideLength }; > CGRect destRect = CGRectMake(0, 0, iconSideLength, iconSideLength); Fixed. > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:164 > > + return squareImage([UIImage imageWithCGImage:thumbnail.get()]); > > Seems like this could create an image smaller than 100x100; is that OK? Why > not use thumbnailSizedImageForImage here? Should do no harm if the thumbnail > is the right size and if it’s not exactly right it will correctly scale up. This should have called thumbnailSizedImageForImage. Fixed. > > Code in function below uses initWithCGImage: and adoptNS rather than > imageWithCGImage: and relying on autorelease. It’s not elegant to use two > different idioms in these adjacent functions. Also seems that these > functions now share more. In both cases, once they create a CGImageRef the > rest is the same. I also will note that thumbnailSizedImageForImage and > squareImage unnecessarily take a UIImage. All they do is extract the > CGImageRef from it, which is ironic since all we do to make the UIImage is > wrap the CGImageRef in it! I believe this points to a little refactoring > that would improve things. I did some minor refactoring here. Thanks for the review! Landed in <http://trac.webkit.org/changeset/185357>
Follow-up build fix in r185362. <http://trac.webkit.org/r185362>