Bug 219752 – [GPU Process] Crash when loading drinktrade.com

• Component: Images
• Status: RESOLVED FIXED

[Jon Lee]

Loading drinktrade.com causes crashes, based on a ToT build on r270635.

From Wenson:
Detected over-release of a CFTypeRef 0x10c8cad80 (317 / CGImage)
Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0 Crashed:
0   CoreFoundation                	0x00000001813269b0 _CFRelease.cold.3 + 92
1   CoreFoundation                	0x00000001813269a8 _CFRelease.cold.3 + 84
2   CoreFoundation                	0x0000000181230a64 _CFRelease + 1444
3   WebCore                       	0x0000000108f12efc WebCore::NativeImage::~NativeImage() + 208
4   WebCore                       	0x0000000108f909c8 WebCore::DisplayList::DisplayList::clear() + 256
5   WebKit                        	0x000000010323f478 WebKit::RemoteImageBufferProxy<WebKit::ImageBufferShareableIOSurfaceBackend>::flushDrawingContextAndCommit() + 192

[Radar WebKit Bug Importer]

<rdar://problem/72190569>

[Wenson Hsieh]

It seems we're just missing a retain here, since cgImage isn't a newly created object we should be taking ownership of:

diff --git a/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp b/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp
index 8bc00142820e..6c01c41599a6 100644
--- a/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp
+++ b/Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp
@@ -352,7 +352,7 @@ void DrawGlyphsRecorder::recordDrawImage(CGRenderingStateRef, CGGStateRef gstate
     m_owner.translate(0, rect.size.height + 2 * rect.origin.y);
     m_owner.scale(FloatSize(1, -1));

-    auto image = NativeImage::create(adoptCF(cgImage));
+    auto image = NativeImage::create(cgImage);
     m_owner.drawNativeImage(*image, image->size(), FloatRect(rect), FloatRect {{ }, image->size()}, ImagePaintingOptions { ImageOrientation::OriginTopLeft });

     // Undo the above y-flip to restore the context.

I suspect this is probably covered by some existing layout test when GPU process is enabled...

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #2)
> It seems we're just missing a retain here, since cgImage isn't a newly
> created object we should be taking ownership of:
> 
> diff --git
> a/Source/WebCore/platform/graphics/displaylists/
> DisplayListDrawGlyphsRecorderCoreText.cpp
> b/Source/WebCore/platform/graphics/displaylists/
> DisplayListDrawGlyphsRecorderCoreText.cpp
> index 8bc00142820e..6c01c41599a6 100644
> ---
> a/Source/WebCore/platform/graphics/displaylists/
> DisplayListDrawGlyphsRecorderCoreText.cpp
> +++
> b/Source/WebCore/platform/graphics/displaylists/
> DisplayListDrawGlyphsRecorderCoreText.cpp
> @@ -352,7 +352,7 @@ void
> DrawGlyphsRecorder::recordDrawImage(CGRenderingStateRef, CGGStateRef gstate
>      m_owner.translate(0, rect.size.height + 2 * rect.origin.y);
>      m_owner.scale(FloatSize(1, -1));
> 
> -    auto image = NativeImage::create(adoptCF(cgImage));
> +    auto image = NativeImage::create(cgImage);
>      m_owner.drawNativeImage(*image, image->size(), FloatRect(rect),
> FloatRect {{ }, image->size()}, ImagePaintingOptions {
> ImageOrientation::OriginTopLeft });
> 
>      // Undo the above y-flip to restore the context.
> 
> I suspect this is probably covered by some existing layout test when GPU
> process is enabled...

Looks like fast/text/emoji.html should have us covered here.

[Wenson Hsieh]

Created attachment 415919 [details]
Patch

[EWS]

Committed r270653: <https://trac.webkit.org/changeset/270653>

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