Bug 38646 – Potential crash in EventHandler::handleTouchEvent

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ben Murdoch]

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.

[Ben Murdoch]

Created attachment 55263 [details]
Proposed Patch.

[Darin Adler]

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

[Ben Murdoch]

(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+.

[WebKit Commit Bot]

Comment on attachment 55263 [details]
Proposed Patch.

Clearing flags on attachment: 55263

Committed r58948: <http://trac.webkit.org/changeset/58948>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.