Bug 34700 - WebCore crashes when element is removed from within 2-dimensional mouse wheel event handler.
Summary: WebCore crashes when element is removed from within 2-dimensional mouse wheel...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-02-08 02:16 PST by Pavel Feldman
Modified: 2010-03-09 11:56 PST (History)
9 users (show)

See Also:


Attachments
test case (750 bytes, text/html)
2010-02-14 16:59 PST, Adele Peterson
no flags Details
patch (1.07 KB, patch)
2010-02-14 17:28 PST, Adele Peterson
no flags Details | Formatted Diff | Diff
Patch with test case (10.51 KB, patch)
2010-03-05 16:24 PST, Andy Estes
adele: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch with test case (v2) (10.82 KB, patch)
2010-03-08 20:34 PST, Andy Estes
adele: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2010-02-08 02:16:59 PST
#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);
Comment 1 Alexey Proskuryakov 2010-02-08 11:30:46 PST
Sounds like use of freed memory, which could be exploitable for remote code execution.
Comment 2 Darin Adler 2010-02-08 11:54:29 PST
Also seems like using RefPtr instead of a raw pointer may be almost enough to fix it.
Comment 3 David Kilzer (:ddkilzer) 2010-02-08 12:54:25 PST
<rdar://problem/7624850>
Comment 4 Adele Peterson 2010-02-08 13:57:17 PST
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?
Comment 5 Pavel Feldman 2010-02-08 14:34:24 PST
(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).
Comment 6 Adele Peterson 2010-02-08 21:44:19 PST
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.
Comment 7 Adele Peterson 2010-02-08 22:46:46 PST
Adding that nil check in scrollAndAcceptEvent fixes the crash, so making a test is the hard part here.
Comment 9 Adele Peterson 2010-02-14 16:59:56 PST
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.
Comment 10 Adele Peterson 2010-02-14 17:28:26 PST
Created attachment 48736 [details]
patch

parking this here in case I don't get around to fix DRT soon.
Comment 11 Darin Adler 2010-02-16 13:51:41 PST
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.
Comment 12 Andy Estes 2010-02-22 16:04:00 PST
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?
Comment 13 Pavel Feldman 2010-02-23 02:05:39 PST
(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.
Comment 14 Adele Peterson 2010-02-23 08:14:20 PST
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?
Comment 15 Andy Estes 2010-03-05 16:24:07 PST
Created attachment 50139 [details]
Patch with test case
Comment 16 Adele Peterson 2010-03-05 16:28:31 PST
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...
Comment 17 Adele Peterson 2010-03-05 16:31:17 PST
Comment on attachment 50139 [details]
Patch with test case

Andy said "yes" to my previous question and will file those follow-up bugs
Comment 18 Eric Seidel (no email) 2010-03-05 16:31:45 PST
Sadly the commit-queue does not work with security patches as the commit-bot account does not have security rights.
Comment 19 Adele Peterson 2010-03-05 16:36:24 PST
This is not a security bug, so I'm moving the component.
Comment 20 Eric Seidel (no email) 2010-03-05 16:50:59 PST
 This bug contains security-related information of a sensitive nature. 
will need to be unchecked for the commit-queue to see it.
Comment 21 WebKit Commit Bot 2010-03-06 06:06:06 PST
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
Comment 22 Andy Estes 2010-03-08 20:34:32 PST
Created attachment 50275 [details]
Patch with test case (v2)

Resolved the conflicts in the Skipped files and re-baselined the patch against r55704.
Comment 23 WebKit Commit Bot 2010-03-09 11:19:52 PST
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
Comment 24 Adele Peterson 2010-03-09 11:20:59 PST
Comment on attachment 50275 [details]
Patch with test case (v2)

Clearing the commit-queue flag.  I'll just commit this myself
Comment 25 Eric Seidel (no email) 2010-03-09 11:23:26 PST
Sorry for the long delay in response.  The commit-queue had some trouble with bug 35905 yesterday.  It's running again fine now.
Comment 26 Adele Peterson 2010-03-09 11:32:45 PST
Committed revision 55737.
Comment 27 Eric Seidel (no email) 2010-03-09 11:45:28 PST
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. :(
Comment 28 Adele Peterson 2010-03-09 11:47:50 PST
ok will look into it
Comment 29 Adele Peterson 2010-03-09 11:56:45 PST
Missed checking in the DRT changes - Committed revision 55739.