Bug 156409 – [WK1] Wheel event callback removing the window causes crash in WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Brent Fulgham]

This is a follow-up to Bug 150871, which dealt with this problem in a WK2 context.

If a window is removed while it is triggering a wheel event, it will crash with a bad memory access, because WebKit will attempt to use the destroyed view. While we corrected the WK2 version of this problem, WK1 takes a slightly different code path that needs to be fixed.

The crash is shown by fast/events/wheel-event-destroys-frame.html.

[Radar WebKit Bug Importer]

<rdar://problem/25631267>

[Brent Fulgham]

Created attachment 276027 [details]
Patch

[Brent Fulgham]

Comment on attachment 276027 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2679
> +                return platformCompletePlatformWidgetWheelEvent(event, *widget, scrollableContainer.get());

Most of this is whitespace change due to removing a level of indent.

[Simon Fraser (smfr)]

Comment on attachment 276027 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:-2654
> -            if (is<RenderWidget>(target)) {

I don't get the change because is<RenderWidget>(target) already null-checks.

[Brent Fulgham]

Comment on attachment 276027 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:-2654
>> -            if (is<RenderWidget>(target)) {
> 
> I don't get the change because is<RenderWidget>(target) already null-checks.

This isn't the relevant part of the change. This stuff only changed because I moved it into a helper function so I could call it twice.

> Source/WebCore/page/EventHandler.cpp:2670
> +                Widget* widget = widgetForElement(*element);

THIS is the change. I found that in WK1 the underlying RenderWidget gets destroyed when the iFrame is destroyed, so the "widget" pointer pointed to deallocated memory. This "widgetForElement" call re-gets it, and will return nullptr in the case of a destroyed iFrame.

[Simon Fraser (smfr)]

Comment on attachment 276027 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2670
>> +                Widget* widget = widgetForElement(*element);
> 
> THIS is the change. I found that in WK1 the underlying RenderWidget gets destroyed when the iFrame is destroyed, so the "widget" pointer pointed to deallocated memory. This "widgetForElement" call re-gets it, and will return nullptr in the case of a destroyed iFrame.

I see. I think the fact that passWheelEventToWidget() is in the condition makes it a little harder to see that it could make the widget invalid.

[Brent Fulgham]

Comment on attachment 276027 [details]
Patch

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

>>> Source/WebCore/page/EventHandler.cpp:2670
>>> +                Widget* widget = widgetForElement(*element);
>> 
>> THIS is the change. I found that in WK1 the underlying RenderWidget gets destroyed when the iFrame is destroyed, so the "widget" pointer pointed to deallocated memory. This "widgetForElement" call re-gets it, and will return nullptr in the case of a destroyed iFrame.
> 
> I see. I think the fact that passWheelEventToWidget() is in the condition makes it a little harder to see that it could make the widget invalid.

That is a little confusing. That's actually not new to this patch, but I agree it would be easier to understand without doing both tests on the same line. I think it was done that way originally to avoid using two levels of indent. Of course, I probably did the original code, too :-\

[Brent Fulgham]

Committed r199245: <http://trac.webkit.org/changeset/199245>