Bug 35798

Summary: Crash due to infinite recursion when viewing composited video on Windows
Product: WebKit Reporter: Chris Marrin <cmarrin>
Component: Layout and RenderingAssignee: Chris Marrin <cmarrin>
Status: RESOLVED FIXED    
Severity: Normal CC: simon.fraser, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Patch simon.fraser: review+

Description Chris Marrin 2010-03-05 10:48:10 PST
Playing any video on Windows with HW compositing enabled will crash due to infinite recursion. Here's the top of the stack:

 	WebKit.dll!WebCore::MediaPlayerPrivate::notifySyncRequired(const WebCore::GraphicsLayer * __formal=0x0b955bd0)  Line 866 + 0xb bytes	C++
 	WebKit.dll!WebCore::GraphicsLayerCACF::notifySyncRequired()  Line 94 + 0x31 bytes	C++
 	WebKit.dll!WebCore::MediaPlayerPrivate::notifySyncRequired(const WebCore::GraphicsLayer * __formal=0x0b955bd0)  Line 869	C++
 	WebKit.dll!WebCore::GraphicsLayerCACF::notifySyncRequired()  Line 94 + 0x31 bytes	C++
 	WebKit.dll!WebCore::MediaPlayerPrivate::notifySyncRequired(const WebCore::GraphicsLayer * __formal=0x0b955bd0)  Line 869	C++

Proposed solution:
1. Make WKCACFLayer be a base class, with a virtual drawInContext() method
2. Subclass WKCACFLayer for GraphicsLayer (equivalent of WebLayer)
3. Subclass WKCACFLayer for the root, to allow the root layer to communicate with the WKCACFLayerRenderer.
4. Eliminate the notifySyncRequired callbacks.
Comment 1 Chris Marrin 2010-03-05 11:59:28 PST
Created attachment 50109 [details]
Patch
Comment 2 WebKit Review Bot 2010-03-05 12:01:28 PST
Attachment 50109 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/win/GraphicsLayerCACF.cpp:120:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Simon Fraser (smfr) 2010-03-05 12:10:30 PST
Comment on attachment 50109 [details]
Patch

> Index: WebCore/platform/graphics/win/GraphicsLayerCACF.cpp
> ===================================================================

> +class WebLayer : public WKCACFLayer {

> +private:
> +   GraphicsLayerCACF* m_owner;
> +};

Could WebLayer get away with having pointer to the base class (GraphicsLayer?). No point making it too incestuous with GraphicsLayerCACF if you can avoid it.

This this a great improvement to these classes!
Comment 4 Simon Fraser (smfr) 2010-03-05 12:35:38 PST
<rdar://problem/7706319>
Comment 5 Chris Marrin 2010-03-05 13:05:41 PST
Fixed in http://trac.webkit.org/changeset/55592