Bug 38646 - Potential crash in EventHandler::handleTouchEvent
Summary: Potential crash in EventHandler::handleTouchEvent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ben Murdoch
URL:
Keywords:
Depends on:
Blocks: 32485
  Show dependency treegraph
 
Reported: 2010-05-06 08:17 PDT by Ben Murdoch
Modified: 2010-05-07 08:05 PDT (History)
4 users (show)

See Also:


Attachments
Proposed Patch. (2.86 KB, patch)
2010-05-06 10:45 PDT, Ben Murdoch
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.