Bug 219752

Summary: [GPU Process] Crash when loading drinktrade.com
Product: WebKit Reporter: Jon Lee <jonlee>
Component: ImagesAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, sabouhallawa, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Jon Lee 2020-12-10 11:46:47 PST 
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
Comment 1 Radar WebKit Bug Importer 2020-12-10 11:47:12 PST 
<rdar://problem/72190569>
Comment 2 Wenson Hsieh 2020-12-10 12:20:08 PST 
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...
Comment 3 Wenson Hsieh 2020-12-10 13:08:28 PST 
(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.
Comment 4 Wenson Hsieh 2020-12-10 13:19:46 PST 
Created attachment 415919 [details]
Patch
Comment 5 EWS 2020-12-10 14:02:02 PST 
Committed r270653: <https://trac.webkit.org/changeset/270653>

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