Bug 142905 - [WinCairo] Crash when plugin window is destroyed.
Summary: [WinCairo] Crash when plugin window is destroyed.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-20 04:50 PDT by peavo
Modified: 2015-03-25 10:56 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.10 KB, patch)
2015-03-20 05:01 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (3.05 KB, patch)
2015-03-25 09:34 PDT, peavo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.