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.
<rdar://problem/25631267>
Created attachment 276027 [details] Patch
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.
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.
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.
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.
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 :-\
Committed r199245: <http://trac.webkit.org/changeset/199245>