Bug 150871

Summary: Wheel event callback removing the window causes crash in WebCore.
Product: WebKit Reporter: Alexander O'Mara <alexanderomara>
Component: UI EventsAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, cdumez, cgarcia, clopez, commit-queue, esprehn+autocc, glenn, gyuyoung.kim, japhet, jh718.park, kondapallykalyan, mrobinson, ossy, rniwa, simon.fraser, tonikitoo
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Mac   
OS: OS X 10.10   
URL: https://cdn.rawgit.com/AlexanderOMara/49e10fcd48b0ab680c56/raw/5c131abd9a11a8f5b1a4534c8ceba2d23f515781/index.html
Bug Depends on: 154439, 154495, 154515    
Bug Blocks: 156409, 156420    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews115 for mac-yosemite
none
Patch
none
Patch
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch v2.
none
Patch
none
Patch v3
none
Patch
none
Patch none

Alexander O'Mara
Reported 2015-11-03 16:50:44 PST
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>
Attachments
Patch (4.94 KB, patch)
2016-02-18 15:30 PST, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (7.42 KB, patch)
2016-02-19 12:21 PST, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (932.09 KB, application/zip)
2016-02-19 12:42 PST, Build Bot 		no flags
Details
Archive of layout-test-results from ews103 for mac-yosemite (1.07 MB, application/zip)
2016-02-19 12:44 PST, Build Bot 		no flags
Details
Archive of layout-test-results from ews115 for mac-yosemite (687.14 KB, application/zip)
2016-02-19 13:02 PST, Build Bot 		no flags
Details
Patch (7.83 KB, patch)
2016-02-19 13:03 PST, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (10.84 KB, patch)
2016-04-05 12:56 PDT, Brent Fulgham 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (595.50 KB, application/zip)
2016-04-05 13:59 PDT, Build Bot 		no flags
Details
Patch v2. (11.48 KB, patch)
2016-04-05 15:13 PDT, Brent Fulgham 		no flags
DetailsFormatted DiffDiff
Patch (16.57 KB, patch)
2016-04-06 21:49 PDT, Brent Fulgham 		no flags
DetailsFormatted DiffDiff
Patch v3 (14.95 KB, patch)
2016-04-06 23:08 PDT, Brent Fulgham 		no flags
DetailsFormatted DiffDiff
Patch (15.97 KB, patch)
2016-04-07 12:54 PDT, Brent Fulgham 		no flags
DetailsFormatted DiffDiff
Patch (16.53 KB, text/plain)
2016-04-07 14:12 PDT, Brent Fulgham 		no flags
Details
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2015-11-05 13:33:49 PST
Thank you, I can reproduce (using a built-in trackpad). rdar://problem/23418283
Simon Fraser (smfr)
Comment 2 2016-02-18 14:57:02 PST
We lost the view null check in r181879 or r182334 despite the comment saying we still null check, in EventHandler::platformCompleteWheelEvent().
Simon Fraser (smfr)
Comment 3 2016-02-18 15:30:47 PST
Created attachment 271701 [details] Patch
Brent Fulgham
Comment 4 2016-02-18 15:54:46 PST
Comment on attachment 271701 [details] Patch r=me. Thanks!
WebKit Commit Bot
Comment 5 2016-02-18 17:13:13 PST
Comment on attachment 271701 [details] Patch Clearing flags on attachment: 271701 Committed r196790: <http://trac.webkit.org/changeset/196790>
WebKit Commit Bot
Comment 6 2016-02-18 17:13:16 PST
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 7 2016-02-18 22:53:39 PST
Re-opened since this is blocked by bug 154439
Simon Fraser (smfr)
Comment 8 2016-02-19 11:11:52 PST
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.
Simon Fraser (smfr)
Comment 9 2016-02-19 11:37:53 PST
Looks like this is caused by latching.
Simon Fraser (smfr)
Comment 10 2016-02-19 12:21:09 PST
Created attachment 271776 [details] Patch
Build Bot
Comment 11 2016-02-19 12:42:18 PST
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.
Build Bot
Comment 12 2016-02-19 12:42:20 PST
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
Build Bot
Comment 13 2016-02-19 12:44:14 PST
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.
Build Bot
Comment 14 2016-02-19 12:44:17 PST
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
Build Bot
Comment 15 2016-02-19 13:02:34 PST
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.
Build Bot
Comment 16 2016-02-19 13:02:37 PST
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
Simon Fraser (smfr)
Comment 17 2016-02-19 13:03:08 PST
Created attachment 271791 [details] Patch
Brent Fulgham
Comment 18 2016-02-19 14:08:52 PST
Comment on attachment 271791 [details] Patch r=me.
Brent Fulgham
Comment 19 2016-02-19 14:09:58 PST
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. :-(
WebKit Commit Bot
Comment 20 2016-02-19 14:59:23 PST
Comment on attachment 271791 [details] Patch Clearing flags on attachment: 271791 Committed r196837: <http://trac.webkit.org/changeset/196837>
WebKit Commit Bot
Comment 21 2016-02-19 14:59:30 PST
All reviewed patches have been landed. Closing bug.
Carlos Garcia Campos
Comment 22 2016-02-20 02:17:33 PST
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.
WebKit Commit Bot
Comment 23 2016-02-20 02:58:08 PST
Re-opened since this is blocked by bug 154495
Carlos Garcia Campos
Comment 24 2016-02-20 03:03:02 PST
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().
Simon Fraser (smfr)
Comment 25 2016-02-20 13:22:13 PST
Re-landed in https://trac.webkit.org/changeset/196866 with a change in Frame::setView that I hope will avoid the crash.
Carlos Garcia Campos
Comment 26 2016-02-20 23:54:26 PST
(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.
Csaba Osztrogonác
Comment 27 2016-02-21 00:09:50 PST
(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.
WebKit Commit Bot
Comment 28 2016-02-21 10:16:20 PST
Re-opened since this is blocked by bug 154515
Simon Fraser (smfr)
Comment 29 2016-02-21 10:18:49 PST
Rolled out again in https://trac.webkit.org/r196872
Brent Fulgham
Comment 30 2016-04-05 11:07:27 PDT
(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?
Brent Fulgham
Comment 31 2016-04-05 12:05:00 PDT
(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.
Brent Fulgham
Comment 32 2016-04-05 12:56:11 PDT
Created attachment 275684 [details] Patch
Brent Fulgham
Comment 33 2016-04-05 13:11:08 PDT
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?
Simon Fraser (smfr)
Comment 34 2016-04-05 13:27:09 PDT
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.
Build Bot
Comment 35 2016-04-05 13:59:21 PDT
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
Build Bot
Comment 36 2016-04-05 13:59:27 PDT
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
Brent Fulgham
Comment 37 2016-04-05 15:13:25 PDT
Created attachment 275700 [details] Patch v2.
Brent Fulgham
Comment 38 2016-04-05 15:17:40 PDT
Comment on attachment 275700 [details] Patch v2. Revised to remove Simon's name. Also, skip wheel event test on iOS.
Martin Robinson
Comment 39 2016-04-05 22:43:44 PDT
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.
Brent Fulgham
Comment 40 2016-04-05 22:48:50 PDT
(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?
Martin Robinson
Comment 41 2016-04-06 17:03:34 PDT
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 ()
Brent Fulgham
Comment 42 2016-04-06 17:43:00 PDT
(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).
Brent Fulgham
Comment 43 2016-04-06 17:43:34 PDT
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)
Martin Robinson
Comment 44 2016-04-06 20:13:00 PDT
(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.
Brent Fulgham
Comment 45 2016-04-06 21:49:24 PDT
Created attachment 275863 [details] Patch
Brent Fulgham
Comment 46 2016-04-06 23:08:52 PDT
Created attachment 275868 [details] Patch v3
Brent Fulgham
Comment 47 2016-04-06 23:09:29 PDT
Comment on attachment 275868 [details] Patch v3 Should resolve GTK/EFL issues as well.
alan
Comment 48 2016-04-07 09:02:00 PDT
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());
Brent Fulgham
Comment 49 2016-04-07 09:48:37 PDT
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.
Brent Fulgham
Comment 50 2016-04-07 10:02:32 PDT
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.
Brent Fulgham
Comment 51 2016-04-07 12:54:36 PDT
Created attachment 275920 [details] Patch
Simon Fraser (smfr)
Comment 52 2016-04-07 13:37:58 PDT
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.
Brent Fulgham
Comment 53 2016-04-07 13:44:10 PDT
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.
Brent Fulgham
Comment 54 2016-04-07 14:12:55 PDT
Created attachment 275934 [details] Patch
Brent Fulgham
Comment 55 2016-04-07 14:16:10 PDT
Landed in r199181. <http://trac.webkit.org/changeset/199181>
Note You need to log in before you can comment on or make changes to this bug.