42092 2010-07-12 10:23:53 -0700 [Chromium] Fix adoptRef usage violation in WebAccessibilityCacheImpl.cpp 2010-07-19 15:12:07 -0700 1 1 1 Unclassified WebKit Accessibility 528+ (Nightly build) PC All RESOLVED FIXED P2 Normal --- 0 ctguil webkit-unassigned commit-queue dglazkov levin oldest_to_newest 249898 0 ctguil 2010-07-12 10:23:53 -0700 Fix adoptRef usage violation in WebAccessibilityCacheImpl.cpp 249914 1 61246 ctguil 2010-07-12 11:04:08 -0700 Created attachment 61246 Patch for adoptRef. 249978 2 61246 darin 2010-07-12 12:58:04 -0700 Comment on attachment 61246 Patch for adoptRef. > + // We pass a reference to this object before it can be adopted. > + relaxAdoptionRequirement(); > m_object->setWrapper(this); This is not the correct way to fix this problem. Instead the call to setWrapper should be moved into the create function. 250031 3 61267 ctguil 2010-07-12 13:59:17 -0700 Created attachment 61267 Proposed patch. Moved adoptRef into create. Thanks for fast review. 250033 4 61267 darin 2010-07-12 14:03:07 -0700 Comment on attachment 61267 Proposed patch. > // FIXME: Remove resetting ref-count from AccessibilityObjectWrapper > - // and convert to use adoptRef. Should add the period on the line you are keeping. > - return new WebAccessibilityCacheImpl::WeakHandle(object); > + RefPtr<WebAccessibilityCacheImpl::WeakHandle> weakHandle = adoptRef(new WebAccessibilityCacheImpl::WeakHandle(object)); > + weakHandle->m_object->setWrapper(weakHandle.get()); > + > + return weakHandle; This should be "return weakHandle.release()" for best performance. Avoids one round of reference counting churn. It looks like this change actually fixes a storage leak that was happening here. Or maybe not because of whatever "resetting ref-count from AccessibilityObjectWrapper" is. I’ll say r=me but I think this is not completely right. 250155 5 61267 commit-queue 2010-07-12 16:57:13 -0700 Comment on attachment 61267 Proposed patch. Rejecting patch 61267 from commit-queue. Found no modified ChangeLogs, cannot create a commit message. All changes require a ChangeLog. See: http://webkit.org/coding/contributing.html 250285 6 61321 ctguil 2010-07-12 21:43:58 -0700 Created attachment 61321 Proposed patch. Added use of release(). Can someone commit this for me? The commit bot failed because there was no ChangeLog found. When I run prepareChangeLog it says "No changes found." 250302 7 levin 2010-07-12 22:14:40 -0700 (In reply to comment #6) > Created an attachment (id=61321) [details] > Proposed patch. > > Added use of release(). Can someone commit this for me? The commit bot failed because there was no ChangeLog found. When I run prepareChangeLog it says "No changes found." If using git, you need to specify the --git-commit and give it an appropriate change like head~1. If using svn, make sure to run prepareChangeLog from the root of your enlistment so it will see the change. Regarding Darin's coment about this not being right, I looked over the code, and it does appear to be correct, but I couldn't figure out what that FIXME comment was about. (This does appear to be a leak that is getting fixed.) 250334 8 61331 ctguil 2010-07-12 23:06:50 -0700 Created attachment 61331 Proposed patch. 250336 9 ctguil 2010-07-12 23:08:36 -0700 For some reason prepare-Changelog was having issues when I used --name to pass my name. It worked once I set the CHANGE_LOG_NAME and CHANGE_LOG_EMAIL_ADDRESS environment variables. 250799 10 61331 commit-queue 2010-07-13 16:20:53 -0700 Comment on attachment 61331 Proposed patch. Clearing flags on attachment: 61331 Committed r63258: <http://trac.webkit.org/changeset/63258> 250800 11 commit-queue 2010-07-13 16:20:58 -0700 All reviewed patches have been landed. Closing bug. 253417 12 ctguil 2010-07-19 15:12:07 -0700 The FIXME comment is obsolete. It was done a while back. http://trac.webkit.org/changeset/51402 61246 2010-07-12 11:04:08 -0700 2010-07-12 21:43:58 -0700 Patch for adoptRef. MyPatch.txt text/plain 974 ctguil SW5kZXg6IFdlYktpdC9jaHJvbWl1bS9zcmMvV2ViQWNjZXNzaWJpbGl0eUNhY2hlSW1wbC5jcHAK PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQotLS0gV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJ bXBsLmNwcAkocmV2aXNpb24gNjMwOTUpCisrKyBXZWJLaXQvY2hyb21pdW0vc3JjL1dlYkFjY2Vz c2liaWxpdHlDYWNoZUltcGwuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC02MywxNCArNjMsMTQgQEAg V2ViQWNjZXNzaWJpbGl0eUNhY2hlKiBXZWJBY2Nlc3NpYmlsaXR5QwogCiBQYXNzUmVmUHRyPFdl YkFjY2Vzc2liaWxpdHlDYWNoZUltcGw6OldlYWtIYW5kbGU+IFdlYkFjY2Vzc2liaWxpdHlDYWNo ZUltcGw6OldlYWtIYW5kbGU6OmNyZWF0ZShBY2Nlc3NpYmlsaXR5T2JqZWN0KiBvYmplY3QpCiB7 Ci0gICAgLy8gRklYTUU6IFJlbW92ZSByZXNldHRpbmcgcmVmLWNvdW50IGZyb20gQWNjZXNzaWJp bGl0eU9iamVjdFdyYXBwZXIKLSAgICAvLyBhbmQgY29udmVydCB0byB1c2UgYWRvcHRSZWYuCi0g ICAgcmV0dXJuIG5ldyBXZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJbXBsOjpXZWFrSGFuZGxlKG9iamVj dCk7CisgICAgcmV0dXJuIGFkb3B0UmVmKG5ldyBXZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJbXBsOjpX ZWFrSGFuZGxlKG9iamVjdCkpOwogfQogCiBXZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJbXBsOjpXZWFr SGFuZGxlOjpXZWFrSGFuZGxlKEFjY2Vzc2liaWxpdHlPYmplY3QqIG9iamVjdCkKICAgICA6IEFj Y2Vzc2liaWxpdHlPYmplY3RXcmFwcGVyKG9iamVjdCkKIHsKKyAgICAvLyBXZSBwYXNzIGEgcmVm ZXJlbmNlIHRvIHRoaXMgb2JqZWN0IGJlZm9yZSBpdCBjYW4gYmUgYWRvcHRlZC4KKyAgICByZWxh eEFkb3B0aW9uUmVxdWlyZW1lbnQoKTsKICAgICBtX29iamVjdC0+c2V0V3JhcHBlcih0aGlzKTsK IH0KIAo= 61267 2010-07-12 13:59:17 -0700 2010-07-12 21:43:58 -0700 Proposed patch. MyPatch.txt text/plain 1080 ctguil SW5kZXg6IFdlYktpdC9jaHJvbWl1bS9zcmMvV2ViQWNjZXNzaWJpbGl0eUNhY2hlSW1wbC5jcHAK PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQotLS0gV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJ bXBsLmNwcAkocmV2aXNpb24gNjMwOTUpCisrKyBXZWJLaXQvY2hyb21pdW0vc3JjL1dlYkFjY2Vz c2liaWxpdHlDYWNoZUltcGwuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC02NCwxNCArNjQsMTUgQEAg V2ViQWNjZXNzaWJpbGl0eUNhY2hlKiBXZWJBY2Nlc3NpYmlsaXR5QwogUGFzc1JlZlB0cjxXZWJB Y2Nlc3NpYmlsaXR5Q2FjaGVJbXBsOjpXZWFrSGFuZGxlPiBXZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJ bXBsOjpXZWFrSGFuZGxlOjpjcmVhdGUoQWNjZXNzaWJpbGl0eU9iamVjdCogb2JqZWN0KQogewog ICAgIC8vIEZJWE1FOiBSZW1vdmUgcmVzZXR0aW5nIHJlZi1jb3VudCBmcm9tIEFjY2Vzc2liaWxp dHlPYmplY3RXcmFwcGVyCi0gICAgLy8gYW5kIGNvbnZlcnQgdG8gdXNlIGFkb3B0UmVmLgotICAg IHJldHVybiBuZXcgV2ViQWNjZXNzaWJpbGl0eUNhY2hlSW1wbDo6V2Vha0hhbmRsZShvYmplY3Qp OworICAgIFJlZlB0cjxXZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJbXBsOjpXZWFrSGFuZGxlPiB3ZWFr SGFuZGxlID0gYWRvcHRSZWYobmV3IFdlYkFjY2Vzc2liaWxpdHlDYWNoZUltcGw6OldlYWtIYW5k bGUob2JqZWN0KSk7CisgICAgd2Vha0hhbmRsZS0+bV9vYmplY3QtPnNldFdyYXBwZXIod2Vha0hh bmRsZS5nZXQoKSk7CisgICAgCisgICAgcmV0dXJuIHdlYWtIYW5kbGU7CiB9CiAKIFdlYkFjY2Vz c2liaWxpdHlDYWNoZUltcGw6OldlYWtIYW5kbGU6OldlYWtIYW5kbGUoQWNjZXNzaWJpbGl0eU9i amVjdCogb2JqZWN0KQogICAgIDogQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXIob2JqZWN0KQog ewotICAgIG1fb2JqZWN0LT5zZXRXcmFwcGVyKHRoaXMpOwogfQogCiAvLyBXZWJBY2Nlc3NpYmls aXR5Q2FjaGVJbXBsIC0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0K 61321 2010-07-12 21:43:58 -0700 2010-07-12 23:06:50 -0700 Proposed patch. MyPatch.txt text/plain 1090 ctguil SW5kZXg6IFdlYktpdC9jaHJvbWl1bS9zcmMvV2ViQWNjZXNzaWJpbGl0eUNhY2hlSW1wbC5jcHAK PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQotLS0gV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJ bXBsLmNwcAkocmV2aXNpb24gNjMwOTUpCisrKyBXZWJLaXQvY2hyb21pdW0vc3JjL1dlYkFjY2Vz c2liaWxpdHlDYWNoZUltcGwuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC02NCwxNCArNjQsMTUgQEAg V2ViQWNjZXNzaWJpbGl0eUNhY2hlKiBXZWJBY2Nlc3NpYmlsaXR5QwogUGFzc1JlZlB0cjxXZWJB Y2Nlc3NpYmlsaXR5Q2FjaGVJbXBsOjpXZWFrSGFuZGxlPiBXZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJ bXBsOjpXZWFrSGFuZGxlOjpjcmVhdGUoQWNjZXNzaWJpbGl0eU9iamVjdCogb2JqZWN0KQogewog ICAgIC8vIEZJWE1FOiBSZW1vdmUgcmVzZXR0aW5nIHJlZi1jb3VudCBmcm9tIEFjY2Vzc2liaWxp dHlPYmplY3RXcmFwcGVyCi0gICAgLy8gYW5kIGNvbnZlcnQgdG8gdXNlIGFkb3B0UmVmLgotICAg IHJldHVybiBuZXcgV2ViQWNjZXNzaWJpbGl0eUNhY2hlSW1wbDo6V2Vha0hhbmRsZShvYmplY3Qp OworICAgIFJlZlB0cjxXZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJbXBsOjpXZWFrSGFuZGxlPiB3ZWFr SGFuZGxlID0gYWRvcHRSZWYobmV3IFdlYkFjY2Vzc2liaWxpdHlDYWNoZUltcGw6OldlYWtIYW5k bGUob2JqZWN0KSk7CisgICAgd2Vha0hhbmRsZS0+bV9vYmplY3QtPnNldFdyYXBwZXIod2Vha0hh bmRsZS5nZXQoKSk7CisgICAgCisgICAgcmV0dXJuIHdlYWtIYW5kbGUucmVsZWFzZSgpOwogfQog CiBXZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJbXBsOjpXZWFrSGFuZGxlOjpXZWFrSGFuZGxlKEFjY2Vz c2liaWxpdHlPYmplY3QqIG9iamVjdCkKICAgICA6IEFjY2Vzc2liaWxpdHlPYmplY3RXcmFwcGVy KG9iamVjdCkKIHsKLSAgICBtX29iamVjdC0+c2V0V3JhcHBlcih0aGlzKTsKIH0KIAogLy8gV2Vi QWNjZXNzaWJpbGl0eUNhY2hlSW1wbCAtLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tCg== 61331 2010-07-12 23:06:50 -0700 2010-07-13 16:20:53 -0700 Proposed patch. MyPatch.txt text/plain 1810 ctguil SW5kZXg6IFdlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZwkocmV2aXNpb24gNjMxNzQpCisrKyBXZWJLaXQvY2hyb21pdW0vQ2hh bmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTAtMDctMTIgIENocmlz IEd1aWxsb3J5ICA8Y2hyaXMuZ3VpbGxvcnlAZ29vZ2xlLmNvbT4KKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXggYWRvcHRSZWYgdXNhZ2UgdmlvbGF0 aW9uIGluIFdlYkFjY2Vzc2liaWxpdHlDYWNoZUltcGwuY3BwLgorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDIwOTIKKworICAgICAgICAqIHNyYy9XZWJB Y2Nlc3NpYmlsaXR5Q2FjaGVJbXBsLmNwcDoKKyAgICAgICAgKFdlYktpdDo6V2ViQWNjZXNzaWJp bGl0eUNhY2hlSW1wbDo6V2Vha0hhbmRsZTo6Y3JlYXRlKToKKyAgICAgICAgKFdlYktpdDo6V2Vi QWNjZXNzaWJpbGl0eUNhY2hlSW1wbDo6V2Vha0hhbmRsZTo6V2Vha0hhbmRsZSk6CisKIDIwMTAt MDctMTIgIFNoZXJpZmYgQm90ICA8d2Via2l0LnJldmlldy5ib3RAZ21haWwuY29tPgogCiAgICAg ICAgIFVucmV2aWV3ZWQsIHJvbGxpbmcgb3V0IHI2MzE3MS4KSW5kZXg6IFdlYktpdC9jaHJvbWl1 bS9zcmMvV2ViQWNjZXNzaWJpbGl0eUNhY2hlSW1wbC5jcHAKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViS2l0 L2Nocm9taXVtL3NyYy9XZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJbXBsLmNwcAkocmV2aXNpb24gNjMw OTUpCisrKyBXZWJLaXQvY2hyb21pdW0vc3JjL1dlYkFjY2Vzc2liaWxpdHlDYWNoZUltcGwuY3Bw CSh3b3JraW5nIGNvcHkpCkBAIC02NCwxNCArNjQsMTUgQEAgV2ViQWNjZXNzaWJpbGl0eUNhY2hl KiBXZWJBY2Nlc3NpYmlsaXR5QwogUGFzc1JlZlB0cjxXZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJbXBs OjpXZWFrSGFuZGxlPiBXZWJBY2Nlc3NpYmlsaXR5Q2FjaGVJbXBsOjpXZWFrSGFuZGxlOjpjcmVh dGUoQWNjZXNzaWJpbGl0eU9iamVjdCogb2JqZWN0KQogewogICAgIC8vIEZJWE1FOiBSZW1vdmUg cmVzZXR0aW5nIHJlZi1jb3VudCBmcm9tIEFjY2Vzc2liaWxpdHlPYmplY3RXcmFwcGVyCi0gICAg Ly8gYW5kIGNvbnZlcnQgdG8gdXNlIGFkb3B0UmVmLgotICAgIHJldHVybiBuZXcgV2ViQWNjZXNz aWJpbGl0eUNhY2hlSW1wbDo6V2Vha0hhbmRsZShvYmplY3QpOworICAgIFJlZlB0cjxXZWJBY2Nl c3NpYmlsaXR5Q2FjaGVJbXBsOjpXZWFrSGFuZGxlPiB3ZWFrSGFuZGxlID0gYWRvcHRSZWYobmV3 IFdlYkFjY2Vzc2liaWxpdHlDYWNoZUltcGw6OldlYWtIYW5kbGUob2JqZWN0KSk7CisgICAgd2Vh a0hhbmRsZS0+bV9vYmplY3QtPnNldFdyYXBwZXIod2Vha0hhbmRsZS5nZXQoKSk7CisgICAgCisg ICAgcmV0dXJuIHdlYWtIYW5kbGUucmVsZWFzZSgpOwogfQogCiBXZWJBY2Nlc3NpYmlsaXR5Q2Fj aGVJbXBsOjpXZWFrSGFuZGxlOjpXZWFrSGFuZGxlKEFjY2Vzc2liaWxpdHlPYmplY3QqIG9iamVj dCkKICAgICA6IEFjY2Vzc2liaWxpdHlPYmplY3RXcmFwcGVyKG9iamVjdCkKIHsKLSAgICBtX29i amVjdC0+c2V0V3JhcHBlcih0aGlzKTsKIH0KIAogLy8gV2ViQWNjZXNzaWJpbGl0eUNhY2hlSW1w bCAtLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tCg==