Bug 38646

Summary: Potential crash in EventHandler::handleTouchEvent
Product: WebKit Reporter: Ben Murdoch <benm>
Component: WebCore Misc.Assignee: Ben Murdoch <benm>
Status: RESOLVED FIXED    
Severity: Normal CC: android-webkit-unforking, commit-queue, gdk, hausmann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 32485    
Attachments:
Description Flags
Proposed Patch. none

Description Ben Murdoch 2010-05-06 08:17:11 PDT
We store the targets of a users touches inside a hashmap in WebCore::EventHandler. When we handle a TouchReleased event we call take() on the hashmap to get the target and remove it as in the case of a release, the user has lifted their finger so we don't need to store that target anymore. However, we store RefPtrs in the map, and call get() to retrieve it's raw ptr on the result of take(). This means that the RefPtr returned by take() is only in scope for the block where we call take() so in the case the refcount goes to 0, we delete the ptr wrapped in the RefPtr when we leave that block. But we saved a copy of that raw pointer and then go on to use it later. Bang. :(

Fix to follow.
Comment 1 Ben Murdoch 2010-05-06 10:45:48 PDT
Created attachment 55263 [details]
Proposed Patch.
Comment 2 Darin Adler 2010-05-06 11:23:49 PDT
Comment on attachment 55263 [details]
Proposed Patch.

> -        if (!touchTarget)
> +        if (!touchTarget.get())

This get() should not be required.

I'm sad that we can't make a test for this. In the bad old days we'd take changes like this without tests a lot. The obviousness of the "simple logic error" is not an important criterion.

r=me even without a test
Comment 3 Ben Murdoch 2010-05-06 11:29:39 PDT
(In reply to comment #2)
> (From update of attachment 55263 [details])
> > -        if (!touchTarget)
> > +        if (!touchTarget.get())
> 
> This get() should not be required.
> 
> I'm sad that we can't make a test for this. In the bad old days we'd take
> changes like this without tests a lot. The obviousness of the "simple logic
> error" is not an important criterion.
> 
> r=me even without a test

Totally agree about the tests. I really wanted to have a test for this and have been trying to get a concrete set of steps for a consistent reproduction over the past day or two but unfortunately didn't get there. In the end I decided it was more important to get the fix into the tree and move on. :)

Thanks for the review, setting cq+.
Comment 4 WebKit Commit Bot 2010-05-07 08:05:10 PDT
Comment on attachment 55263 [details]
Proposed Patch.

Clearing flags on attachment: 55263

Committed r58948: <http://trac.webkit.org/changeset/58948>
Comment 5 WebKit Commit Bot 2010-05-07 08:05:16 PDT
All reviewed patches have been landed.  Closing bug.