Bug 125868 - Crash when trying to copy image
Summary: Crash when trying to copy image
Status: RESOLVED DUPLICATE of bug 131576
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.9
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-17 12:38 PST by Zev Eisenberg
Modified: 2015-12-16 21:34 PST (History)
5 users (show)

See Also:


Attachments
Crash log (58.10 KB, application/octet-stream)
2013-12-17 12:38 PST, Zev Eisenberg
no flags Details
Patch to reduce memory usage and CPU time. (1.41 KB, patch)
2015-08-16 18:59 PDT, Jeremy Zerfas
no flags Details | Formatted Diff | Diff
Patch to reduce memory usage and CPU time with test. (13.08 KB, patch)
2015-08-30 20:20 PDT, Jeremy Zerfas
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zev Eisenberg 2013-12-17 12:38:31 PST
Created attachment 219441 [details]
Crash log

Safari Web Content crashes when I try to copy the image on this page: https://github.com/jverdi/JVFloatLabeledTextField

Steps to reproduce:
1. Open Safari 7.0.1 (9537.73.11)
2. Navigate to https://github.com/jverdi/JVFloatLabeledTextField
3. Right-click the animated gif showing the UI animation. The URL of the image is https://github-camo.global.ssl.fastly.net/be57d040ec0ce5d6467fb73564c6bcb6c76d5a7b/687474703a2f2f6472696262626c652e73332e616d617a6f6e6177732e636f6d2f75736572732f363431302f73637265656e73686f74732f313235343433392f666f726d2d616e696d6174696f6e2d5f6769665f2e676966
4. Choose "Copy Image"

Expected results:
No crash.

Actual results:
Safari Web Content crashes with EXC_BAD_ACCESS (crash log attached).

Reproduced in Webkit Nightly 7.0.1 (9537.73.11, 538+) and Safari 7.0.1 (9537.73.11).
Running on Mavericks 10.9.1 and confirmed on three other machines.
Comment 1 Zev Eisenberg 2013-12-17 12:39:09 PST
Oh, and that's the public Mavericks build 10.9.1 (13B3116).
Comment 2 Jeremy Zerfas 2015-08-16 18:58:56 PDT
I was able to reproduce this bug on a freshly installed Mavericks virtual machine that was set up with 2 GB of memory. It looks like the crashes were happening due to all the memory that is required when processing the animated GIF (800x600 resolution x 439 frames in the animation * 32 bits per pixel = ~804 MiB when uncompressed). Updating to Safari 7.1.8 seems to largely have resolved the issue by getting rid of the crashes. However during and after copying an image into the clipboard Safari still seems to be using up more memory than is necessary.

While looking at this bug I did notice that when copying an image to the clipboard, WebKit allocates an NSImage that isn't needed. The patch I'm adding causes it to to not allocate the NSImage. With the animation from this bug, the patch considerably reduces the amount of memory and CPU time used when copying an image to the clipboard. Originally when copying the animation on my Mac, the resident memory size would increase to ~2,750 MiB. With the patch applied the resident memory size only increases up to ~1,150 MiB and the process also finishes about 1.75 seconds faster.
Comment 3 Jeremy Zerfas 2015-08-16 18:59:06 PDT
Created attachment 259133 [details]
Patch to reduce memory usage and CPU time.

Patch to reduce memory usage and CPU time when copying images/animations to the clipboard.
Comment 4 Said Abou-Hallawa 2015-08-18 09:22:32 PDT
Comment on attachment 259133 [details]
Patch to reduce memory usage and CPU time.

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

> Source/WebCore/ChangeLog:12
> +        No new tests needed.

Why are not no new tests needed for this change?

> Source/WebCore/platform/mac/PasteboardMac.mm:258
> +    NSData *imageData = (NSData*)(pasteboardImage.image->getTIFFRepresentation());

I think this is wrong. The clipboard has to manage deallocating its own data. The original data can be freed or even the whole application gets shut down but the clipboard should keep its data. In other words, I think a new memory buffer has to be allocated every time we want to write to the clipboard.
Comment 5 Jeremy Zerfas 2015-08-30 20:19:44 PDT
(In reply to comment #4)

> > Source/WebCore/ChangeLog:12
> > +        No new tests needed.
> 
> Why are not no new tests needed for this change?

I didn't think it was possible to get the memory statistics needed for testing but after doing a more thorough search I found the mallocStatistics() function which will be sufficient. I've created a new patch that now includes a test for this.

> > Source/WebCore/platform/mac/PasteboardMac.mm:258
> > +    NSData *imageData = (NSData*)(pasteboardImage.image->getTIFFRepresentation());
> 
> I think this is wrong. The clipboard has to manage deallocating its own
> data. The original data can be freed or even the whole application gets shut
> down but the clipboard should keep its data. In other words, I think a new
> memory buffer has to be allocated every time we want to write to the
> clipboard.

I'm pretty sure that assessment is wrong but I might have misled you because I was mistaken about the NSImage allocation causing much of the extra memory use. The difference in memory usage is primarily due to using getTIFFRepresentation() to get the TIFF data instead of using getNSImage() to allocate a NSImage (which is just a wrapper around the data returned from getTIFFRepresentation() anyway) and then calling TIFFRepresentation on the NSImage which requires more overhead. The clipboard doesn't need the NSImage, it only needs the TIFF data which is still being allocated. As a bit of a proof of this, pasting the animation from the clipboard into another app still works fine even after quitting the patched browser. Hope that clears things up.
Comment 6 Jeremy Zerfas 2015-08-30 20:20:37 PDT
Created attachment 260269 [details]
Patch to reduce memory usage and CPU time with test.
Comment 7 Darin Adler 2015-11-30 09:45:11 PST
Comment on attachment 260269 [details]
Patch to reduce memory usage and CPU time with test.

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

Looks good.

> Source/WebCore/platform/mac/PasteboardMac.mm:258
> +    NSData *imageData = (NSData*)(pasteboardImage.image->getTIFFRepresentation());

We should not use NSData here at all. Instead:

    CFDataRef imageData = pasteboardImage.image->getTIFFRepresentation();

And then below change SharedBuffer::wrapNSData to SharedBuffer::wrapCFData.
Comment 8 Darin Adler 2015-11-30 09:48:45 PST
Comment on attachment 260269 [details]
Patch to reduce memory usage and CPU time with test.

Wait a second, this bug says “Crash when trying to copy an image”; this doesn’t fix that crash. It looks like there was a null check missing before, which is why the crash happened.

Why are we trying to use the same bug report for this issue? Shouldn’t we also add the missing null check?
Comment 9 Jeremy Zerfas 2015-12-13 15:40:18 PST
I looked into this more and the crash was fixed by the null check that was added while creating a shared memory buffer in WebPlatformStrategies.cpp in revision 167180.

> Why are we trying to use the same bug report for this issue?

I added this patch to this bug because it is sort of related to this bug. My patch reduces the amount of memory that is used while copying an image which would make the crash occur less often without the null check or make the copy less likely to partially fail with the null check. I can create a new bug and add the patch to that if that would be preferable.
Comment 10 Darin Adler 2015-12-14 08:21:04 PST
(In reply to comment #9)
> I can create a new
> bug and add the patch to that if that would be preferable.

It would.
Comment 11 Jeremy Zerfas 2015-12-16 19:50:48 PST
I've created bug 152374 regarding reducing the resource usage when copying an image to the clipboard. While submitting that bug I found the related bug 131576 which is a duplicate of this bug. I would suggest closing this bug since this problem was resolved in revision 167180 which was created from the patch for bug 131576.
Comment 12 Darin Adler 2015-12-16 21:34:59 PST
OK

*** This bug has been marked as a duplicate of bug 131576 ***