Bug 142905

Summary: [WinCairo] Crash when plugin window is destroyed.
Product: WebKit Reporter: peavo
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description peavo 2015-03-20 04:50:59 PDT
I'm getting a reproducible crash when leaving a page with a windowed plugin. Leaving the page causes the plugin window to be destroyed with the Win32 api function DestroyWindow. This will send a syncrounous WM_PARENTNOTIFY message to the parent, in this case the WebView, see https://msdn.microsoft.com/en-us/library/windows/desktop/ms632682(v=vs.85).aspx. The WebView window procedure will, when processing the WM_PARENTNOTIFY message, call UpdateWindow to paint synchronously. This will cause reentrancy problems, since we're already called from WebCore code, and then reenter WebCore painting code. In this particular case, we crash because we try to paint a deleted RenderLayer.
Comment 1 peavo 2015-03-20 05:01:47 PDT
Created attachment 249108 [details]
Patch
Comment 2 peavo 2015-03-20 10:21:01 PDT
An alternative would be to stop UpdateWindow only for the WM_PARENTNOTIFY message, I'm a little worried about drawing performance in that case, though.
Comment 3 Alex Christensen 2015-03-20 10:47:36 PDT
Comment on attachment 249108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249108&action=review

> Source/WebKit/win/WebView.cpp:2526
> +    case WM_MOUSEWHEEL:
> +    case WM_MOUSEMOVE:
> +    case WM_KEYDOWN:
> +    case WM_GESTURE:

Where did this set of event types come from? Why not more, or fewer?
Comment 4 peavo 2015-03-20 11:07:35 PDT
(In reply to comment #3)
> Comment on attachment 249108 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249108&action=review
> 
> > Source/WebKit/win/WebView.cpp:2526
> > +    case WM_MOUSEWHEEL:
> > +    case WM_MOUSEMOVE:
> > +    case WM_KEYDOWN:
> > +    case WM_GESTURE:
> 
> Where did this set of event types come from? Why not more, or fewer?

I had some repaint artifacts when scrolling, that's why I added these. Maybe more are needed, and maybe it would be safest to only avoid UpdateWindow when receiving the WM_PARENTNOTIFY message. In some cases that might lead to unneeded painting, I think.
Comment 5 peavo 2015-03-25 09:34:11 PDT
Created attachment 249409 [details]
Patch
Comment 6 WebKit Commit Bot 2015-03-25 10:56:10 PDT
Comment on attachment 249409 [details]
Patch

Clearing flags on attachment: 249409

Committed r181966: <http://trac.webkit.org/changeset/181966>
Comment 7 WebKit Commit Bot 2015-03-25 10:56:15 PDT
All reviewed patches have been landed.  Closing bug.