Bug 37954

Summary: Occasional crash in CoreGraphics when using accelerated compositing in Windows.
Product: WebKit Reporter: Andy Estes <aestes>
Component: Layout and RenderingAssignee: Andy Estes <aestes>
Status: RESOLVED FIXED    
Severity: Critical Keywords: InRadar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Bug Depends on: 37952    
Bug Blocks:    
Attachments:
Description Flags
patch mjs: review+

Description Andy Estes 2010-04-21 14:52:10 PDT
When viewing pages rendered in accelerated compositing mode using the Windows port, there is occasionally a crash in the underlying CoreGraphics library used by WebKit on Windows.  Methods to repeat the crash include resizing the browser window, repeatedly changing focus between the browser and another application, and dragging items into the WebView.  The crash appears to be intermittent.
Comment 1 Andy Estes 2010-04-21 14:52:33 PDT
<rdar://problem/7806666>
Comment 2 Andy Estes 2010-04-21 14:54:33 PDT
Diagnosis from Steve Falkenburg:

This CGDataProviderRef pulls its data from m_backingStoreBitmap. Note this is deleted on a timer after a period of inactivity. I'm thinking this may be the cause of the bug. If the m_backingStoreBitmap is freed and then the WKCACFLayerRenderer::renderTimerFired(Timer<WKCACFLayerRenderer>*) timer fires, we will crash. One possible easy fix would be to protect the call to deleteBackingStoreSoon() if isAcceleratedCompositing() returns true in WebView::paint(). I haven't tested this theory (or fix).
Comment 3 Andy Estes 2010-04-21 20:56:49 PDT
Created attachment 54020 [details]
patch
Comment 4 Andy Estes 2010-04-21 21:02:57 PDT
Here is a description of a solution from Adam Roben, which is what I implemented in the patch:

I mentioned to Steve that one way to fix the problem with deleteBackingStoreSoon() is to add a RefCounted wrapper around an HBITMAP, and use that to hold the WebView's backing store bitmap. Then we can ref that wrapper object in updateRootLayerContents and call CGDataProviderCreateWithData (not CreateWithCFData), passing the wrapper object as the info pointer and a function that just calls deref() on the wrapper as the release callback. That should allow the bitmap to live as long as the CGImageRef needs it.
Comment 5 Maciej Stachowiak 2010-04-21 23:51:30 PDT
Comment on attachment 54020 [details]
patch

r=me

Nice fix.
Comment 6 Andy Estes 2010-04-22 00:13:09 PDT
Committed revision 58067.  Thanks Maciej!
Comment 7 Adam Roben (:aroben) 2010-04-22 08:14:24 PDT
Comment on attachment 54020 [details]
patch

Nice fix!