38646 2010-05-06 08:17:11 -0700 Potential crash in EventHandler::handleTouchEvent 2010-05-07 08:05:16 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 32485 1 benm benm android-webkit-unforking commit-queue gdk hausmann oldest_to_newest 221691 0 benm 2010-05-06 08:17:11 -0700 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. 221808 1 55263 benm 2010-05-06 10:45:48 -0700 Created attachment 55263 Proposed Patch. 221834 2 55263 darin 2010-05-06 11:23:49 -0700 Comment on attachment 55263 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 221838 3 benm 2010-05-06 11:29:39 -0700 (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+. 222372 4 55263 commit-queue 2010-05-07 08:05:10 -0700 Comment on attachment 55263 Proposed Patch. Clearing flags on attachment: 55263 Committed r58948: <http://trac.webkit.org/changeset/58948> 222373 5 commit-queue 2010-05-07 08:05:16 -0700 All reviewed patches have been landed. Closing bug. 55263 2010-05-06 10:45:48 -0700 2010-05-07 08:05:10 -0700 Proposed Patch. 38464.txt text/plain 2928 benm SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1ODg5NCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjIgQEAKKzIwMTAtMDUtMDYgIEJlbiBNdXJkb2NoICA8YmVubUBnb29nbGUuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFBvdGVu dGlhbCBjcmFzaCBpbiBFdmVudEhhbmRsZXI6OmhhbmRsZVRvdWNoRXZlbnQKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTM4NjQ2CisKKyAgICAgICAgRml4 IGEgcmVmIGNvdW50aW5nIGJ1ZyB0aGF0IGNhbiBjYXVzZSBhIGNyYXNoIGlmIHRoZSBtX29yaWdp bmF0aW5nb3VjaFBvaW50VGFyZ2V0cworICAgICAgICBoYXNobWFwIGhvbGRzIHRoZSBsYXN0IHJl ZiB0byBhbiBFdmVudFRhcmdldCB3aGVuIHRoZSB1c2VyIGxpZnRzIHRoZWlyIGZpbmdlci4KKwor ICAgICAgICBUaGlzIGlzIHZlcnkgaGFyZCB0byByZXByb2R1Y2UgaW4gYSBjb25zaXN0ZW50IHdh eSBhbmQgY2xlYXJseSBhCisgICAgICAgIHNpbXBsZSBsb2dpYyBlcnJvciBpbiB0aGUgY29kZSwg dGhlcmVmb3JlIG5vIG5ldyB0ZXN0cy4KKworICAgICAgICAqIHBhZ2UvRXZlbnRIYW5kbGVyLmNw cDoKKyAgICAgICAgKFdlYkNvcmU6OkV2ZW50SGFuZGxlcjo6aGFuZGxlVG91Y2hFdmVudCk6IERv bid0IGxldCB0aGUgUmVmUHRyIHdlIGdldCBiYWNrIGZyb20KKyAgICAgICAgICAgIHRoZSBoYXNt YXAgZ28gb3V0IG9mIHNjb3BlIHNvIHNvb24gYXMgaXQgY291bGQgZGVsZXRlIHRoZSB3cmFwcGVk IHB0ciBpZiB0aGUKKyAgICAgICAgICAgIGhhc2htYXAgaGVsZCB0aGUgbGFzdCByZWYgKGFuZCB3 ZSB1c2UgdGhlIHJhdyBwdHIgdGhhdCB0aGUgUmVmUHRyCisgICAgICAgICAgICB3cmFwcyBsYXRl ciBpbiB0aGUgV2ViQ29yZTo6VG91Y2ggY29uc3RydWN0b3IpLgorCiAyMDEwLTA1LTA2ICBEYXJp biBBZGxlciAgPGRhcmluQGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBCZXRoIERh a2luLgpJbmRleDogV2ViQ29yZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g V2ViQ29yZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAJKHJldmlzaW9uIDU4ODgwKQorKysgV2ViQ29y ZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTI3NjgsMjEgKzI3Njgs MjEgQEAgYm9vbCBFdmVudEhhbmRsZXI6OmhhbmRsZVRvdWNoRXZlbnQoY29ucwogCiAgICAgICAg IC8vIEluY3JlbWVudCB0aGUgcGxhdGZvcm0gdG91Y2ggaWQgYnkgMSB0byBhdm9pZCBzdG9yaW5n IGEga2V5IG9mIDAgaW4gdGhlIGhhc2htYXAuCiAgICAgICAgIHVuc2lnbmVkIHRvdWNoUG9pbnRU YXJnZXRLZXkgPSBwb2ludC5pZCgpICsgMTsKLSAgICAgICAgRXZlbnRUYXJnZXQqIHRvdWNoVGFy Z2V0ID0gMDsKKyAgICAgICAgUmVmUHRyPEV2ZW50VGFyZ2V0PiB0b3VjaFRhcmdldDsKICAgICAg ICAgaWYgKHBvaW50LnN0YXRlKCkgPT0gUGxhdGZvcm1Ub3VjaFBvaW50OjpUb3VjaFByZXNzZWQp IHsKICAgICAgICAgICAgIG1fb3JpZ2luYXRpbmdUb3VjaFBvaW50VGFyZ2V0cy5zZXQodG91Y2hQ b2ludFRhcmdldEtleSwgdGFyZ2V0KTsKICAgICAgICAgICAgIHRvdWNoVGFyZ2V0ID0gdGFyZ2V0 OwogICAgICAgICB9IGVsc2UgaWYgKHBvaW50LnN0YXRlKCkgPT0gUGxhdGZvcm1Ub3VjaFBvaW50 OjpUb3VjaFJlbGVhc2VkIHx8IHBvaW50LnN0YXRlKCkgPT0gUGxhdGZvcm1Ub3VjaFBvaW50OjpU b3VjaENhbmNlbGxlZCkgewogICAgICAgICAgICAgLy8gVGhlIHRhcmdldCBzaG91bGQgYmUgdGhl IG9yaWdpbmFsIHRhcmdldCBmb3IgdGhpcyB0b3VjaCwgc28gZ2V0IGl0IGZyb20gdGhlIGhhc2ht YXAuIEFzIGl0J3MgYSByZWxlYXNlIG9yIGNhbmNlbAogICAgICAgICAgICAgLy8gd2UgYWxzbyBy ZW1vdmUgaXQgZnJvbSB0aGUgbWFwLgotICAgICAgICAgICAgdG91Y2hUYXJnZXQgPSBtX29yaWdp bmF0aW5nVG91Y2hQb2ludFRhcmdldHMudGFrZSh0b3VjaFBvaW50VGFyZ2V0S2V5KS5nZXQoKTsK KyAgICAgICAgICAgIHRvdWNoVGFyZ2V0ID0gbV9vcmlnaW5hdGluZ1RvdWNoUG9pbnRUYXJnZXRz LnRha2UodG91Y2hQb2ludFRhcmdldEtleSk7CiAgICAgICAgIH0gZWxzZQotICAgICAgICAgICAg dG91Y2hUYXJnZXQgPSBtX29yaWdpbmF0aW5nVG91Y2hQb2ludFRhcmdldHMuZ2V0KHRvdWNoUG9p bnRUYXJnZXRLZXkpLmdldCgpOworICAgICAgICAgICAgdG91Y2hUYXJnZXQgPSBtX29yaWdpbmF0 aW5nVG91Y2hQb2ludFRhcmdldHMuZ2V0KHRvdWNoUG9pbnRUYXJnZXRLZXkpOwogCi0gICAgICAg IGlmICghdG91Y2hUYXJnZXQpCisgICAgICAgIGlmICghdG91Y2hUYXJnZXQuZ2V0KCkpCiAgICAg ICAgICAgICBjb250aW51ZTsKIAotICAgICAgICBSZWZQdHI8VG91Y2g+IHRvdWNoID0gVG91Y2g6 OmNyZWF0ZShkb2MtPmZyYW1lKCksIHRvdWNoVGFyZ2V0LCBwb2ludC5pZCgpLAorICAgICAgICBS ZWZQdHI8VG91Y2g+IHRvdWNoID0gVG91Y2g6OmNyZWF0ZShkb2MtPmZyYW1lKCksIHRvdWNoVGFy Z2V0LmdldCgpLCBwb2ludC5pZCgpLAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICBwb2ludC5zY3JlZW5Qb3MoKS54KCksIHBvaW50LnNjcmVlblBvcygpLnkoKSwK ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgYWRqdXN0ZWRQYWdl WCwgYWRqdXN0ZWRQYWdlWSk7CiAK