Bug 34700 – WebCore crashes when element is removed from within 2-dimensional mouse wheel event handler.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Pavel Feldman]

#0	0x044f953d in WebCore::RenderObject::enclosingBox at RenderObject.cpp:556
#1	0x03f03fce in WebCore::scrollAndAcceptEvent at EventHandler.cpp:116
#2	0x03f044ee in WebCore::EventHandler::handleWheelEvent at EventHandler.cpp:1869
#3	0x03f1166a in WebCore::EventHandler::wheelEvent at EventHandlerMac.mm:120
#4	0x0034aa5a in -[WebHTMLView scrollWheel:] at WebHTMLView.mm:3319
#5	0x902ac731 in -[NSWindow sendEvent:]

http://trac.webkit.org/browser/trunk/WebCore/page/EventHandler.cpp
Lines 1865-1869.

Looks like if node is removed in line 1868, subsequent 1869 will crash.

// Just break up into two scrolls if we need to.  Diagonal movement on
1866	            // a MacBook pro is an example of a 2-dimensional mouse wheel event (where both deltaX and deltaY can be set).
1867	            Node* stopNode = m_previousWheelScrolledNode.get();
1868	            scrollAndAcceptEvent(e.deltaX(), ScrollLeft, ScrollRight, e, node, &stopNode);
1869	            scrollAndAcceptEvent(e.deltaY(), ScrollUp, ScrollDown, e, node, &stopNode);

[Alexey Proskuryakov]

Sounds like use of freed memory, which could be exploitable for remote code execution.

[Darin Adler]

Also seems like using RefPtr instead of a raw pointer may be almost enough to fix it.

[David Kilzer (:ddkilzer)]

<rdar://problem/7624850>

[Adele Peterson]

Maybe I'm missing something, but it doesn't seem like scrollAndAcceptEvent actually dispatches an event.  I'm not sure how to remove the node at that point.  Do you have a test case?

[Pavel Feldman]

(In reply to comment #4)
> Maybe I'm missing something, but it doesn't seem like scrollAndAcceptEvent
> actually dispatches an event.  I'm not sure how to remove the node at that
> point.  Do you have a test case?

I did not manage to come up with reduction, so here is a larger app it crashes with:
http://repenaxa.com/viewer/crash.html (make sure both scrollers are visible, then make diagonal scroll on MBP).

Removing node in between of those lines was a wild guess. It is just that I know that I am removing nodes in the scroll event. I can see wheel event being dispatched before both invocations though (EventHandler:1855 though).

[Adele Peterson]

The renderer is nil in scrollAndAcceptEvent, but the node is still present.  I think scrollAndAcceptEvent just needs to check for that.  I don't think this is a security bug.

I'll send a fix out soon.

[Adele Peterson]

Adding that nil check in scrollAndAcceptEvent fixes the crash, so making a test is the hard part here.

[Adam Barth]

http://code.google.com/p/chromium/issues/detail?id=35150

[Adele Peterson]

Created attachment 48734 [details]
test case

I don't think there's any way to turn this into a layout test without enhancing DRT.  I think you need to send a wheel event, and initWheelEvent isn't exposed in JS.

[Adele Peterson]

Created attachment 48736 [details]
patch

parking this here in case I don't get around to fix DRT soon.

[Darin Adler]

It’s kind of strange that the wheel event is one kind you can't create from the DOM. The problem is that the DOM event-creating API we implemented is sort of a patchwork of things found in various versions of the DOM events standard and none included the wheel event.

[Andy Estes]

Adele pointed me to http://www.w3.org/TR/DOM-Level-3-Events/#events-wheelevents, which I'm going to attempt to implement.  This will help test both this and https://bugs.webkit.org/show_bug.cgi?id=29601.

By the way, I can no longer repro the crash.  Is it just me?  Did the page change?

[Pavel Feldman]

(In reply to comment #12)
> Adele pointed me to
> http://www.w3.org/TR/DOM-Level-3-Events/#events-wheelevents, which I'm going to
> attempt to implement.  This will help test both this and
> https://bugs.webkit.org/show_bug.cgi?id=29601.
> 
> By the way, I can no longer repro the crash.  Is it just me?  Did the page
> change?

Page did not change, crash is still there on ToT on my MBP.

[Adele Peterson]

you should be able to repro with the attached test case too.  It can be a little tricky sometimes to make sure you're scrolling diagonally.

(In reply to comment #12)
> Adele pointed me to
> http://www.w3.org/TR/DOM-Level-3-Events/#events-wheelevents, which I'm going to
> attempt to implement.  This will help test both this and
> https://bugs.webkit.org/show_bug.cgi?id=29601.
> 
> By the way, I can no longer repro the crash.  Is it just me?  Did the page
> change?

[Andy Estes]

Created attachment 50139 [details]
Patch with test case

[Adele Peterson]

Comment on attachment 50139 [details]
Patch with test case

You should file bugs for the disabled tests and the missing DRT implementations for the other platforms.  And did you confirm that this test will crash without the WebCore patch?  I'll cq+ this after that answer...

[Adele Peterson]

Comment on attachment 50139 [details]
Patch with test case

Andy said "yes" to my previous question and will file those follow-up bugs

[Eric Seidel (no email)]

Sadly the commit-queue does not work with security patches as the commit-bot account does not have security rights.

[Adele Peterson]

This is not a security bug, so I'm moving the component.

[Eric Seidel (no email)]

 This bug contains security-related information of a sensitive nature. 
will need to be unchecked for the commit-queue to see it.

[WebKit Commit Bot]

Comment on attachment 50139 [details]
Patch with test case

Rejecting patch 50139 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Adele Peterson', '--force']" exit_code: 1
Last 500 characters of output:
ests/platform/gtk/Skipped
Hunk #1 FAILED at 5803.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/gtk/Skipped.rej
patching file LayoutTests/platform/mac-tiger/Skipped
patching file LayoutTests/platform/qt/Skipped
Hunk #1 FAILED at 5095.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej
patching file LayoutTests/platform/win/Skipped
Hunk #1 FAILED at 774.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/win/Skipped.rej

Full output: http://webkit-commit-queue.appspot.com/results/342205

[Andy Estes]

Created attachment 50275 [details]
Patch with test case (v2)

Resolved the conflicts in the Skipped files and re-baselined the patch against r55704.

[WebKit Commit Bot]

Comment on attachment 50275 [details]
Patch with test case (v2)

Rejecting patch 50275 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Adele Peterson', '--force']" exit_code: 1
Last 500 characters of output:
e LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/events/remove-child-onscroll-expected.txt
patching file LayoutTests/fast/events/remove-child-onscroll.html
patching file LayoutTests/platform/gtk/Skipped
patching file LayoutTests/platform/mac-tiger/Skipped
patching file LayoutTests/platform/qt/Skipped
Hunk #1 FAILED at 5103.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej
patching file LayoutTests/platform/win/Skipped

Full output: http://webkit-commit-queue.appspot.com/results/475001

[Adele Peterson]

Comment on attachment 50275 [details]
Patch with test case (v2)

Clearing the commit-queue flag.  I'll just commit this myself

[Eric Seidel (no email)]

Sorry for the long delay in response.  The commit-queue had some trouble with bug 35905 yesterday.  It's running again fine now.

[Adele Peterson]

Committed revision 55737.

[Eric Seidel (no email)]

The commit-queue just failed it's "does the current trunk build work" check:
fast/events/remove-child-onscroll.html -> failed

I assume it's related to this patch and that the bots will roll red shortly. :(

[Adele Peterson]

ok will look into it

[Adele Peterson]

Missed checking in the DRT changes - Committed revision 55739.