Bug 125868 – Crash when trying to copy image

• Component: Images
• Status: RESOLVED DUPLICATE

[Zev Eisenberg]

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.

[Zev Eisenberg]

Oh, and that's the public Mavericks build 10.9.1 (13B3116).

[Jeremy Zerfas]

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.

[Jeremy Zerfas]

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.

[Said Abou-Hallawa]

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.

[Jeremy Zerfas]

(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.

[Jeremy Zerfas]

Created attachment 260269 [details]
Patch to reduce memory usage and CPU time with test.

[Darin Adler]

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.

[Darin Adler]

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?

[Jeremy Zerfas]

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.

[Darin Adler]

(In reply to comment #9)
> I can create a new
> bug and add the patch to that if that would be preferable.

It would.

[Jeremy Zerfas]

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.

[Darin Adler]

OK

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