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

Description Alexander O'Mara 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>
Comment 1 Alexey Proskuryakov 2015-11-05 13:33:49 PST
Thank you, I can reproduce (using a built-in trackpad).

rdar://problem/23418283
Comment 2 Simon Fraser (smfr) 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().
Comment 3 Simon Fraser (smfr) 2016-02-18 15:30:47 PST
Created attachment 271701 [details]
Patch
Comment 4 Brent Fulgham 2016-02-18 15:54:46 PST
Comment on attachment 271701 [details]
Patch

r=me. Thanks!
Comment 5 WebKit Commit Bot 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>
Comment 6 WebKit Commit Bot 2016-02-18 17:13:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 WebKit Commit Bot 2016-02-18 22:53:39 PST
Re-opened since this is blocked by bug 154439
Comment 8 Simon Fraser (smfr) 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.
Comment 9 Simon Fraser (smfr) 2016-02-19 11:37:53 PST
Looks like this is caused by latching.
Comment 10 Simon Fraser (smfr) 2016-02-19 12:21:09 PST
Created attachment 271776 [details]
Patch
Comment 11 Build Bot 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.
Comment 12 Build Bot 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
Comment 13 Build Bot 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.
Comment 14 Build Bot 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
Comment 15 Build Bot 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.
Comment 16 Build Bot 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
Comment 17 Simon Fraser (smfr) 2016-02-19 13:03:08 PST
Created attachment 271791 [details]
Patch
Comment 18 Brent Fulgham 2016-02-19 14:08:52 PST
Comment on attachment 271791 [details]
Patch

r=me.
Comment 19 Brent Fulgham 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. :-(
Comment 20 WebKit Commit Bot 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>
Comment 21 WebKit Commit Bot 2016-02-19 14:59:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Carlos Garcia Campos 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.
Comment 23 WebKit Commit Bot 2016-02-20 02:58:08 PST
Re-opened since this is blocked by bug 154495
Comment 24 Carlos Garcia Campos 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().
Comment 25 Simon Fraser (smfr) 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.
Comment 26 Carlos Garcia Campos 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.
Comment 27 Csaba Osztrogonác 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.
Comment 28 WebKit Commit Bot 2016-02-21 10:16:20 PST
Re-opened since this is blocked by bug 154515
Comment 29 Simon Fraser (smfr) 2016-02-21 10:18:49 PST
Rolled out again in https://trac.webkit.org/r196872
Comment 30 Brent Fulgham 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?
Comment 31 Brent Fulgham 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.
Comment 32 Brent Fulgham 2016-04-05 12:56:11 PDT
Created attachment 275684 [details]
Patch
Comment 33 Brent Fulgham 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?
Comment 34 Simon Fraser (smfr) 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.
Comment 35 Build Bot 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
Comment 36 Build Bot 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
Comment 37 Brent Fulgham 2016-04-05 15:13:25 PDT
Created attachment 275700 [details]
Patch v2.
Comment 38 Brent Fulgham 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.
Comment 39 Martin Robinson 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.
Comment 40 Brent Fulgham 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?
Comment 41 Martin Robinson 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 ()
Comment 42 Brent Fulgham 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).
Comment 43 Brent Fulgham 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)
Comment 44 Martin Robinson 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.
Comment 45 Brent Fulgham 2016-04-06 21:49:24 PDT
Created attachment 275863 [details]
Patch
Comment 46 Brent Fulgham 2016-04-06 23:08:52 PDT
Created attachment 275868 [details]
Patch v3
Comment 47 Brent Fulgham 2016-04-06 23:09:29 PDT
Comment on attachment 275868 [details]
Patch v3

Should resolve GTK/EFL issues as well.
Comment 48 zalan 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());
Comment 49 Brent Fulgham 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.
Comment 50 Brent Fulgham 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.
Comment 51 Brent Fulgham 2016-04-07 12:54:36 PDT
Created attachment 275920 [details]
Patch
Comment 52 Simon Fraser (smfr) 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.
Comment 53 Brent Fulgham 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.
Comment 54 Brent Fulgham 2016-04-07 14:12:55 PDT
Created attachment 275934 [details]
Patch
Comment 55 Brent Fulgham 2016-04-07 14:16:10 PDT
Landed in r199181.
<http://trac.webkit.org/changeset/199181>