In WebKit, if a window is removed while it is triggering a wheel event, it will crash on a bad memory access (example code and live demo below). The crash is triggered in the WebCore/page/FrameView.cpp, in FrameView::wheelEvent, on what I believe is the delegatesScrolling() code. However, the fault appears to lie with the function calling it. WebCore/page/mac/EventHandlerMac.mm: ... bool EventHandler::platformCompleteWheelEvent(const PlatformWheelEvent& wheelEvent, ContainerNode* scrollableContainer, ScrollableArea* scrollableArea) { // We do another check on the frame view because the event handler can run JS which results in the frame getting destroyed. ASSERT(m_frame.view()); ... The only check against it being null appears to be an ASSERT call, which is not graceful and removed in release builds. Compare this against a similar funtion: WebCore/page/EventHandler.cpp ... bool EventHandler::platformCompleteWheelEvent(const PlatformWheelEvent& event, ContainerNode*, ScrollableArea*) { // We do another check on the frame view because the event handler can run JS which results in the frame getting destroyed. FrameView* view = m_frame.view(); bool didHandleEvent = view ? view->wheelEvent(event) : false; m_isHandlingWheelEvent = false; return didHandleEvent; } ... This has a check against a null pointer, before calling the wheelEvent. Currently, this can be seen affecting both the latest Nightly WebKit browser and Apple's Safari, as well as anything that links against the same libraries, such as those using WebView. Additionally, I ran a test with pywebview, which I believe is using a legacy API for these libraries, and it crashes in WebCore/page/mac/EventHandlerMac.mm, on the EventHandler::platformCompletePlatformWidgetWheelEvent function. Here is a live demo for how to trigger the crash: https://cdn.rawgit.com/AlexanderOMara/49e10fcd48b0ab680c56/raw/5c131abd9a11a8f5b1a4534c8ceba2d23f515781/index.html And the code from the link: <!DOCTYPE html> <html> <head> <meta charset="utf-8" /> <title>WebKit Wheel Event Remove Window Crash</title> </head> <body> <script type="text/javascript"> /*<!--*/ (function(window) {'use strict'; var iframe = window.document.createElement('iframe'); iframe.style.position = 'fixed'; iframe.style.left = iframe.style.top = '5%'; iframe.style.width = iframe.style.height = '90%'; window.document.body.appendChild(iframe); function iframeReady() { iframe.contentWindow.document.body.innerHTML = '<h1>Wheel here to crash.</h1>'; iframe.contentWindow.addEventListener('wheel', function() { // Removing the window during event firing causes crash. window.document.body.removeChild(iframe); }); } var pollInterval; var pollReady = function() { if (iframe.contentWindow.document.body) { window.clearInterval(pollInterval); iframeReady(); } }; pollInterval = window.setInterval(pollReady, 100); })(window); /*-->*/ </script> </body> </html>
Thank you, I can reproduce (using a built-in trackpad). rdar://problem/23418283
We lost the view null check in r181879 or r182334 despite the comment saying we still null check, in EventHandler::platformCompleteWheelEvent().
Created attachment 271701 [details] Patch
Comment on attachment 271701 [details] Patch r=me. Thanks!
Comment on attachment 271701 [details] Patch Clearing flags on attachment: 271701 Committed r196790: <http://trac.webkit.org/changeset/196790>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 154439
It's very odd that this caused fast/events/wheelevent-basic.html to regress. That test is about overflow div scrolling, and doesn't even hit this code. There must be some state left from a previous test.
Looks like this is caused by latching.
Created attachment 271776 [details] Patch
Comment on attachment 271776 [details] Patch Attachment 271776 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/855669 Number of test failures exceeded the failure limit.
Created attachment 271782 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 271776 [details] Patch Attachment 271776 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/855679 Number of test failures exceeded the failure limit.
Created attachment 271783 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 271776 [details] Patch Attachment 271776 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/855735 Number of test failures exceeded the failure limit.
Created attachment 271790 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 271791 [details] Patch
Comment on attachment 271791 [details] Patch r=me.
Comment on attachment 271791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271791&action=review > Source/WebCore/page/EventHandler.cpp:2666 > + filter->endFilteringDeltas(); Good catch. That should have been null checked in my original version. :-(
Comment on attachment 271791 [details] Patch Clearing flags on attachment: 271791 Committed r196837: <http://trac.webkit.org/changeset/196837>
This has caused lots of test to crash for EFL and GTK. It seems we are using the MainFrame after it has been deleted now, see the bt: #0 0x00007ffd8dd2e5f0 in ?? () #1 0x00007f3b1172b387 in WebCore::EventHandler::clear() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #2 0x00007f3b1173c951 in WebCore::Frame::setView(WTF::RefPtr<WebCore::FrameView>&&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #3 0x00007f3b1173d5db in WebCore::Frame::~Frame() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #4 0x00007f3b11755db9 in WebCore::MainFrame::~MainFrame() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #5 0x00007f3b11761b8d in WebCore::Page::~Page() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #6 0x00007f3b10da200d in WebKit::WebPage::close() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #7 0x00007f3b10ebb8e6 in WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::MessageDecoder&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #8 0x00007f3b10be2879 in IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::MessageDecoder&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #9 0x00007f3b10cfc7b6 in WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #10 0x00007f3b10bded86 in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::MessageDecoder, std::default_delete<IPC::MessageDecoder> >) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #11 0x00007f3b10bdf753 in IPC::Connection::dispatchOneMessage() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #12 0x00007f3b0f51933f in WTF::RunLoop::performWork() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #13 0x00007f3b0f5480e9 in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #14 0x00007f3b0a814b4a in g_main_dispatch (context=0x17b0b00) at gmain.c:3154 #15 g_main_context_dispatch (context=context@entry=0x17b0b00) at gmain.c:3769 #16 0x00007f3b0a814ec8 in g_main_context_iterate (context=0x17b0b00, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3840 #17 0x00007f3b0a8151e2 in g_main_loop_run (loop=0x1e855c0) at gmain.c:4034 #18 0x00007f3b0f548910 in WTF::RunLoop::run() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #19 0x00007f3b10e77222 in int WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain>(int, char**) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #20 0x00007f3b05cd7870 in __libc_start_main (main=0x400af0 <main>, argc=2, argv=0x7ffd8dd2ee48, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffd8dd2ee38) at libc-start.c:291 #21 0x0000000000400b49 in _start () So, the main frame is deleted (and the destructor deletes the WheelEventDeltaFilter), then the Frame destructor is run that calls setView(nullptr) that calls EventHandler::clear(). And now that EventHandler::clear class clearLatchedState, we are using m_frame.mainFrame() that has already been deleted. What is happening at least for GTK+ in linux is that m_frame.mainFrame().wheelEventDeltaFilter() is still returning the pointer of the WheelEventDeltaFilter that was already deleted, and then it crashes when trying to call endFilteringDeltas. So, I think we should not access the mainFrame from EventHandler::clear() when it's called from the destructor, or even better not call EventHandler::clear() from the Frame destructor since it's going to be deleted.
Re-opened since this is blocked by bug 154495
I tried to prepare a quick fix for this instead of rolling out, but it was not that easy in the end :-( I tried to split the Frame::setView() method to only do the prepare for detach/destruction in the destructor and not call EventHandler::clear(), but then we do FrameLoader::cancelAndClear() that again calls EventHandler::clear() unconditionally. So, we need to figure out a proper way to not use the mainFrame from EventHandler::clear().
Re-landed in https://trac.webkit.org/changeset/196866 with a change in Frame::setView that I hope will avoid the crash.
(In reply to comment #25) > Re-landed in https://trac.webkit.org/changeset/196866 with a change in > Frame::setView that I hope will avoid the crash. No, it didn't work :-(. When the main frame is destroyed we have a valid view, then setView is called with nullptr which is != m_view. Also note that FrameLoader::cancelAndClear() calls EventHandler::clear() again unconditionally.
(In reply to comment #25) > Re-landed in https://trac.webkit.org/changeset/196866 with a change in > Frame::setView that I hope will avoid the crash. It made many tests crash on EFL and GTK bots, cc-ing port maintainers.
Re-opened since this is blocked by bug 154515
Rolled out again in https://trac.webkit.org/r196872
(In reply to comment #22) > This has caused lots of test to crash for EFL and GTK. It seems we are using > the MainFrame after it has been deleted now, see the bt: > > #0 0x00007ffd8dd2e5f0 in ?? () > #1 0x00007f3b1172b387 in WebCore::EventHandler::clear() () from > /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0. > so.37 Where can we see crash traces like these? Do I need to land a patch, watch for crashes, and get the traces somewhere? I don't believe you have tests in EWS, correct?
(In reply to comment #22) > So, the main frame is deleted (and the destructor deletes the > WheelEventDeltaFilter), then the Frame destructor is run that calls > setView(nullptr) that calls EventHandler::clear(). And now that > EventHandler::clear class clearLatchedState, we are using > m_frame.mainFrame() that has already been deleted. I can see how this could be an issue if the Frame being destructed is a MainFrame, and that its MainFrame member is a reference to itself. In that case, the MainFrame portion of the object could have been destroyed before 'setView(nullptr)' was called. It might work to call "setView(nullptr)" in the MainFrame destructor, and only call it in the Frame destructor for non-mainframes.
Created attachment 275684 [details] Patch
Can an EFL/GTK person try this patch and let me know if it still produces a crash for you? If so, can you please identify WHICH tests fail, and provide stack traces?
Comment on attachment 275684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275684&action=review > Source/WebCore/ChangeLog:1 > +2016-04-05 Simon Fraser <simon.fraser@apple.com> You should fix this with your name. > LayoutTests/ChangeLog:1 > +2016-04-05 Simon Fraser <simon.fraser@apple.com> Ditto.
Comment on attachment 275684 [details] Patch Attachment 275684 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1105271 New failing tests: fast/events/wheel-event-destroys-frame.html
Created attachment 275692 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Created attachment 275700 [details] Patch v2.
Comment on attachment 275700 [details] Patch v2. Revised to remove Simon's name. Also, skip wheel event test on iOS.
After the most recent patch I'm seeing 159 more crashing tests than before the patch. Let me know if a stack trace will help.
(In reply to comment #39) > After the most recent patch I'm seeing 159 more crashing tests than before > the patch. Let me know if a stack trace will help. Bummer! Can you give me the stack traces from a couple of the tests, and which test they are from? I'm guessing something in fast/events, but let me know if there are other places! Can you just zip up the "layout-test-results" and attack to this bug?
I finally managed to get a stack trace for this issue, though not in a debug build. 0x00007ffff655af96 in WebCore::EventHandler::clearLatchedState() () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 (gdb) where #0 0x00007ffff655af96 in WebCore::EventHandler::clearLatchedState() () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #1 0x00007ffff655b1c7 in WebCore::EventHandler::clear() () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #2 0x00007ffff6479a98 in WebCore::FrameLoader::clear(WebCore::Document*, bool, bool, bool) () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #3 0x00007ffff647a985 in WebCore::FrameLoader::cancelAndClear() () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #4 0x00007ffff656dbe3 in WebCore::Frame::~Frame() () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #5 0x00007ffff6587b7c in WebCore::MainFrame::~MainFrame() () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #6 0x00007ffff6587bc9 in WebCore::MainFrame::~MainFrame() () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #7 0x00007ffff6593ea0 in WebCore::Page::~Page() () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #8 0x00007ffff5b63fd9 in WebKit::WebPage::close() () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #9 0x00007ffff5c90df1 in WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::MessageDecoder&) () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #10 0x00007ffff5980bc9 in IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::MessageDecoder&) () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #11 0x00007ffff5ab4456 in WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #12 0x00007ffff597cd36 in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::MessageDecoder, std::default_delete<IPC::MessageDecoder> >) () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #13 0x00007ffff597d703 in IPC::Connection::dispatchOneMessage() () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #14 0x00007ffff20e8cdf in WTF::RunLoop::performWork() () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #15 0x00007ffff21194b9 in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #16 0x00007ffff254a166 in g_main_dispatch () from target:/home/martin/WebKit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0 #17 0x00007ffff254b029 in g_main_context_dispatch () from target:/home/martin/WebKit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0 #18 0x00007ffff254b21c in g_main_context_iterate () from target:/home/martin/WebKit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0 #19 0x00007ffff254b656 in g_main_loop_run () from target:/home/martin/WebKit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0 #20 0x00007ffff2119d40 in WTF::RunLoop::run() () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #21 0x00007ffff5c4441a in int WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain>(int, char**) () from target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #22 0x00007ffff461aa40 in __libc_start_main (main=0x400760 <main>, argc=2, argv=0x7fffffffdd48, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffdd38) at libc-start.c:289 #23 0x00000000004007b9 in _start ()
(In reply to comment #41) > I finally managed to get a stack trace for this issue, though not in a debug > build. > > 0x00007ffff655af96 in WebCore::EventHandler::clearLatchedState() () from > target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > (gdb) where > #0 0x00007ffff655af96 in WebCore::EventHandler::clearLatchedState() () from > target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #1 0x00007ffff655b1c7 in WebCore::EventHandler::clear() () from > target:/home/martin/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #2 0x00007ffff6479a98 in WebCore::FrameLoader::clear(WebCore::Document*, > bool, bool, bool) () from Looks like we are still trying to use a destroyed WheelEventFilter (just through a different code path).
A speculative bug fix based on Patch v2: Index: MainFrame.cpp =================================================================== --- MainFrame.cpp (revision 199064) +++ MainFrame.cpp (working copy) @@ -64,8 +64,10 @@ MainFrame::~MainFrame() { + setView(nullptr); if (m_diagnosticLoggingClient) m_diagnosticLoggingClient->mainFrameDestroyed(); + m_recentWheelEventDeltaFilter = nullptr; // Make sure this is null during destruction of the parent class. } Ref<MainFrame> MainFrame::create(Page& page, PageConfiguration& configuration)
(In reply to comment #43) > A speculative bug fix based on Patch v2: > > Index: MainFrame.cpp > =================================================================== > --- MainFrame.cpp (revision 199064) > +++ MainFrame.cpp (working copy) > @@ -64,8 +64,10 @@ > > MainFrame::~MainFrame() > { > + setView(nullptr); > if (m_diagnosticLoggingClient) > m_diagnosticLoggingClient->mainFrameDestroyed(); > + m_recentWheelEventDeltaFilter = nullptr; // Make sure this is null > during destruction of the parent class. > } > > Ref<MainFrame> MainFrame::create(Page& page, PageConfiguration& > configuration) I can confirm that this fix seemed to eliminate the crashes.
Created attachment 275863 [details] Patch
Created attachment 275868 [details] Patch v3
Comment on attachment 275868 [details] Patch v3 Should resolve GTK/EFL issues as well.
Comment on attachment 275868 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=275868&action=review > Source/WebCore/page/mac/EventHandlerMac.mm:1055 > + return false; Any reason why the normal "protect all the things" pattern does not work here? See EventHandler::handleWheelEvent RefPtr<FrameView> protector(m_frame.view());
Comment on attachment 275868 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=275868&action=review >> Source/WebCore/page/mac/EventHandlerMac.mm:1055 >> + return false; > > Any reason why the normal "protect all the things" pattern does not work here? > See EventHandler::handleWheelEvent > RefPtr<FrameView> protector(m_frame.view()); This might work if we protected somewhere higher up in the call stack for this wheel event handler.
Comment on attachment 275868 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=275868&action=review >>> Source/WebCore/page/mac/EventHandlerMac.mm:1055 >>> + return false; >> >> Any reason why the normal "protect all the things" pattern does not work here? >> See EventHandler::handleWheelEvent >> RefPtr<FrameView> protector(m_frame.view()); > > This might work if we protected somewhere higher up in the call stack for this wheel event handler. Looking at the code, we *do* use the protector (see EventHandler::handleWheelEvent line 2619. I think the issue is not that the view object has been destroyed (since that reference would keep it alive), but that the view() member of m_frame may have been cleared at this point.
Created attachment 275920 [details] Patch
Comment on attachment 275920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275920&action=review > Source/WebCore/page/Frame.cpp:255 > + if (m_eventHandler && m_view != view) Is the m_view != view check still needed? > Source/WebCore/page/Frame.cpp:271 > +void Frame::setMainFrameWasDestroyed() > +{ > + m_eventHandler = nullptr; > + m_mainFrameWasDestroyed = true; > +} > + I think you should make m_eventHandler protected, and have ~MainFrame clear it. > Source/WebCore/page/Frame.h:154 > EventHandler& eventHandler() const; > + EventHandler* nullOrEventHandler() const; Seems weird to have both of these. Maybe rename nullOrEventHandler to eventHandlerPtr()? > Source/WebCore/page/MainFrame.cpp:70 > + // Clear MainFrame state so Frame destructor doesn't encounter problems. I don't think this comment adds much.
Comment on attachment 275920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275920&action=review Thanks for the review! >> Source/WebCore/page/Frame.cpp:255 >> + if (m_eventHandler && m_view != view) > > Is the m_view != view check still needed? I think you are right. I thought we might avoid losing event handler state if the same view was set on the frame, but we do so much other clean-up in here that it's probably not good to be inconsistent. >> Source/WebCore/page/Frame.cpp:271 >> + > > I think you should make m_eventHandler protected, and have ~MainFrame clear it. Will do. >> Source/WebCore/page/Frame.h:154 >> + EventHandler* nullOrEventHandler() const; > > Seems weird to have both of these. Maybe rename nullOrEventHandler to eventHandlerPtr()? eventHandlerPtr() seems good. I'll use that. >> Source/WebCore/page/MainFrame.cpp:70 >> + // Clear MainFrame state so Frame destructor doesn't encounter problems. > > I don't think this comment adds much. I'll remove it.
Created attachment 275934 [details] Patch
Landed in r199181. <http://trac.webkit.org/changeset/199181>