|Summary:||Potential crash in EventHandler::handleTouchEvent|
|Product:||WebKit||Reporter:||Ben Murdoch <benm>|
|Component:||WebCore Misc.||Assignee:||Ben Murdoch <benm>|
|Severity:||Normal||CC:||android-webkit-unforking, commit-queue, gdk, hausmann|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
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 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.