225228 2021-04-30 05:55:53 -0700 CheckedLock::tryLock() use pattern should be more robust 2021-06-14 02:47:40 -0700 1 1 1 Unclassified WebKit Web Template Framework WebKit Local Build Unspecified Unspecified RESOLVED CONFIGURATION CHANGED https://bugs.webkit.org/show_bug.cgi?id=224970 InRadar P2 Normal --- 1 kkinnunen kkinnunen benjamin cdumez cmarcelo ews-watchlist webkit-bug-importer oldest_to_newest 1755658 0 kkinnunen 2021-04-30 05:55:53 -0700 CheckedLock::tryLock() use pattern should be more robust From bug 224970 discussions 1755661 1 kkinnunen 2021-04-30 06:21:27 -0700 Bug 224970 (In reply to Chris Dumez from comment #15) > > My preference would have been: > TryLocker locker(lock); > if (!locker.isLocked()) > return; > > This is also what Blink uses. > > I dislike any kind of adopted lock. I don't think we should be interacting > with the lock directly or adopting lock as this is a recipe for mistakes. Blink MutexTryLocker does not support thread safety analysis. As such it is equivalent of already implemented auto locker = tryHoldLock(). https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/threading_primitives.h I failed to write a TryLocker that would support current implementation of thread safety analysis. Suppose we set aside the preferences for syntactical constructs for some time. The decision of what syntax to use wrt tryLock is based on which problem are you solving: A) The problem to solve is that caller forgets to adopt the lock after tryLock(). B) The problem to solve is that caller accesses the locked variables without lock. My (relatively small) experience leads to think that B is the more important problem to solve. In patch below I'm trying to communicate that the thread safety analysis already covers all of B and most of A. 1755662 2 427412 kkinnunen 2021-04-30 06:23:17 -0700 Created attachment 427412 Patch 1757852 3 webkit-bug-importer 2021-05-07 05:56:13 -0700 <rdar://problem/77653579> 427412 2021-04-30 06:23:17 -0700 2021-06-14 02:47:40 -0700 Patch bug-225228-20210430162315.patch text/plain 5086 kkinnunen U3VidmVyc2lvbiBSZXZpc2lvbjogMjc2ODM0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDFhMzdlYTJhZGJlNjFmNGI2ZGE0Njc2 MGFhNTA0ZGRiMWIyZTQ3Y2QuLmFkOTM0YTllMjE4MDQ1MTU5OGJhMWY4YzBkYWY4N2RmZTQ0NGRl MTYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMjEtMDQtMzAgIEtpbW1vIEtpbm51bmVuICA8a2tp bm51bmVuQGFwcGxlLmNvbT4KKworICAgICAgICBDaGVja2VkTG9jazo6dHJ5TG9jaygpIHVzZSBw YXR0ZXJuIHNob3VsZCBiZSBtb3JlIHJvYnVzdAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjI1MjI4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgKiB3dGYvQ2hlY2tlZExvY2suaDoKKyAgICAgICAgQWRkIFdB Uk5fVU5VU0VEX1JFVFVSTiB0byBDaGVja2VkTG9jazo6dHJ5TG9jaygpLgorICAgICAgICBNYWtl IHRoZSBMb2NrZXI8Q2hlY2tlZExvY2s+IGFkb3B0aW5nIGNvbnN0cnVjdG9yIGVhc2llciB0byB1 c2UKKyAgICAgICAgYnkgcHJvdmlkaW5nIGEgcmVhZHktbWFkZSB0YWcgdmFsdWUuCisKIDIwMjEt MDQtMjkgIEJlbiBOaGFtICA8bmhhbUBhcHBsZS5jb20+CiAKICAgICAgICAgUmVkdWNlIG1lbW9y eSBmb290cHJpbnQgZm9yIGJhY2tncm91bmQgdGFicwpkaWZmIC0tZ2l0IGEvU291cmNlL1dURi93 dGYvQ2hlY2tlZExvY2suaCBiL1NvdXJjZS9XVEYvd3RmL0NoZWNrZWRMb2NrLmgKaW5kZXggNDRi ODk5OTY5MTIzNDUyMDAxOThmZDg2NGE5OTg5NjY3NzYzOTk0Mi4uZDZhYzI4ZWU2ODUzZGZmZTlm MWUxNDNhZTYxODc1ZTcyMWNhNjY1OCAxMDA2NDQKLS0tIGEvU291cmNlL1dURi93dGYvQ2hlY2tl ZExvY2suaAorKysgYi9Tb3VyY2UvV1RGL3d0Zi9DaGVja2VkTG9jay5oCkBAIC00NSw3ICs0NSw3 IEBAIG5hbWVzcGFjZSBXVEYgewogLy8gICAgICAgewogLy8gICAgICAgICAgIGlmICghbV9sb2Nr LnRyeUxvY2soKSkKIC8vICAgICAgICAgICAgICByZXR1cm47Ci0vLyAgICAgICAgICAgTG9ja2Vy IGxvY2tlciB7IEFkb3B0TG9ja1RhZyB7IH0sIG1fb3RoZXJMb2NrIH07CisvLyAgICAgICAgICAg TG9ja2VyIGxvY2tlciB7IGFkb3B0TG9jaywgbV9vdGhlckxvY2sgfTsKIC8vICAgICAgICAgICBt X290aGVyVmFsdWUgPSB2YWx1ZTsKIC8vICAgICAgIH0KIC8vICAgcHJpdmF0ZToKQEAgLTYwLDgg KzYwLDggQEAgY2xhc3MgV1RGX0NBUEFCSUxJVFlfTE9DSyBDaGVja2VkTG9jayA6IExvY2sgewog cHVibGljOgogICAgIGNvbnN0ZXhwciBDaGVja2VkTG9jaygpID0gZGVmYXVsdDsKICAgICB2b2lk IGxvY2soKSBXVEZfQUNRVUlSRVNfTE9DSygpIHsgTG9jazo6bG9jaygpOyB9Ci0gICAgYm9vbCB0 cnlMb2NrKCkgV1RGX0FDUVVJUkVTX0xPQ0tfSUYodHJ1ZSkgeyByZXR1cm4gTG9jazo6dHJ5TG9j aygpOyB9Ci0gICAgYm9vbCB0cnlfbG9jaygpIFdURl9BQ1FVSVJFU19MT0NLX0lGKHRydWUpIHsg cmV0dXJuIExvY2s6OnRyeV9sb2NrKCk7IH0gLy8gTk9MSU5UOiBJbnRlbnRpb25hbCBkZXZpYXRp b24gdG8gc3VwcG9ydCBzdGQ6OnNjb3BlZF9sb2NrLgorICAgIGJvb2wgdHJ5TG9jaygpIFdBUk5f VU5VU0VEX1JFVFVSTiBXVEZfQUNRVUlSRVNfTE9DS19JRih0cnVlKSB7IHJldHVybiBMb2NrOjp0 cnlMb2NrKCk7IH0KKyAgICBib29sIHRyeV9sb2NrKCkgV0FSTl9VTlVTRURfUkVUVVJOIFdURl9B Q1FVSVJFU19MT0NLX0lGKHRydWUpIHsgcmV0dXJuIExvY2s6OnRyeV9sb2NrKCk7IH0gLy8gTk9M SU5UOiBJbnRlbnRpb25hbCBkZXZpYXRpb24gdG8gc3VwcG9ydCBzdGQ6OnNjb3BlZF9sb2NrLgog ICAgIHZvaWQgdW5sb2NrKCkgV1RGX1JFTEVBU0VTX0xPQ0soKSB7IExvY2s6OnVubG9jaygpOyB9 CiAgICAgdm9pZCB1bmxvY2tGYWlybHkoKSBXVEZfUkVMRUFTRVNfTE9DSygpIHsgTG9jazo6dW5s b2NrRmFpcmx5KCk7IH0KICAgICB2b2lkIHNhZmVwb2ludCgpIHsgTG9jazo6c2FmZXBvaW50KCk7 IH0KQEAgLTc3LDYgKzc3LDkgQEAgaW5saW5lIHZvaWQgYXNzZXJ0SXNIZWxkKGNvbnN0IENoZWNr ZWRMb2NrJiBsb2NrKSBXVEZfQVNTRVJUU19BQ1FVSVJFRF9MT0NLKGxvY2sKIAogdXNpbmcgQWRv cHRMb2NrVGFnID0gc3RkOjphZG9wdF9sb2NrX3Q7CiAKKy8vIFZhbHVlIHRvIHVzZSB0byBzZWxl Y3QgTG9ja2VyIGNvbnN0cnVjdG9yIHRoYXQgYWRvcHRzIGhlbGQgbG9jay4KK2lubGluZSBjb25z dGV4cHIgQWRvcHRMb2NrVGFnIGFkb3B0TG9jayA9IEFkb3B0TG9ja1RhZyB7IH07CisKIC8vIExv Y2tlciBzcGVjaWFsaXphdGlvbiB0byB1c2Ugd2l0aCBDaGVja2VkTG9jay4KIC8vIE5vbi1tb3Zh YmxlIHNpbXBsZSBzY29wZWQgbG9jayBob2xkZXIuCiAvLyBFeGFtcGxlOiBMb2NrZXIgbG9ja2Vy IHsgbV9sb2NrIH07CkBAIC0xMDksNCArMTEyLDQgQEAgTG9ja2VyKEFkb3B0TG9ja1RhZywgQ2hl Y2tlZExvY2smKSAtPiBMb2NrZXI8Q2hlY2tlZExvY2s+OwogCiB1c2luZyBXVEY6OmFzc2VydElz SGVsZDsKIHVzaW5nIFdURjo6Q2hlY2tlZExvY2s7Ci11c2luZyBXVEY6OkFkb3B0TG9ja1RhZzsK K3VzaW5nIFdURjo6YWRvcHRMb2NrOwpkaWZmIC0tZ2l0IGEvVG9vbHMvQ2hhbmdlTG9nIGIvVG9v bHMvQ2hhbmdlTG9nCmluZGV4IGUxM2FiNzkzYTU5ZTc1MzlmMWVmN2E3ZDY5ZTczOTNhYTQ4NzA5 OGQuLjY1NzgyOTBlZDIyMjUxODA5ZDg4NjllZTEzYTQyZGI1OWU4ZmU0OGYgMTAwNjQ0Ci0tLSBh L1Rvb2xzL0NoYW5nZUxvZworKysgYi9Ub29scy9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNSBAQAor MjAyMS0wNC0zMCAgS2ltbW8gS2lubnVuZW4gIDxra2lubnVuZW5AYXBwbGUuY29tPgorCisgICAg ICAgIENoZWNrZWRMb2NrOjp0cnlMb2NrKCkgdXNlIHBhdHRlcm4gc2hvdWxkIGJlIG1vcmUgcm9i dXN0CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMjUy MjgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIFRl c3RXZWJLaXRBUEkvVGVzdHMvV1RGL0NoZWNrZWRMb2NrVGVzdC5jcHA6CisgICAgICAgIChUZXN0 V2ViS2l0QVBJOjpURVNUKToKKyAgICAgICAgQWRkIHRlc3QtY2FzZXMgZGVzY3JpYmluZyB3aGlj aCBjb21tb24gYWRvcHRpbmcgZXJyb3JzCisgICAgICAgIGFyZSBjYXVnaHQgY29tcGlsZSB0aW1l IGF0IHRoZSBtb21lbnQuCisKIDIwMjEtMDQtMjkgIERhcmluIEFkbGVyICA8ZGFyaW5AYXBwbGUu Y29tPgogCiAgICAgICAgIEV4dGVuZCBTb3J0ZWRBcnJheU1hcCBmdXJ0aGVyIHRvIHdvcmsgb24g Y2FzZS1mb2xkZWQgc3RyaW5ncywgdXNlIGluIE1JTUVUeXBlUmVnaXN0cnkKZGlmZiAtLWdpdCBh L1Rvb2xzL1Rlc3RXZWJLaXRBUEkvVGVzdHMvV1RGL0NoZWNrZWRMb2NrVGVzdC5jcHAgYi9Ub29s cy9UZXN0V2ViS2l0QVBJL1Rlc3RzL1dURi9DaGVja2VkTG9ja1Rlc3QuY3BwCmluZGV4IGQ4MmVl MWEyZTViZGUwZGNhYmYzNWNlOWRjNzMxMTIxMTAyMjYyNGEuLmI4OGJlOTNjNWIwYWQyYTNkMmQ4 MDNjZTQxM2VjM2I2MjljN2JmYzIgMTAwNjQ0Ci0tLSBhL1Rvb2xzL1Rlc3RXZWJLaXRBUEkvVGVz dHMvV1RGL0NoZWNrZWRMb2NrVGVzdC5jcHAKKysrIGIvVG9vbHMvVGVzdFdlYktpdEFQSS9UZXN0 cy9XVEYvQ2hlY2tlZExvY2tUZXN0LmNwcApAQCAtNDAsNyArNDAsNyBAQCBwdWJsaWM6CiAgICAg ewogICAgICAgICBpZiAoIW1fb3RoZXJMb2NrLnRyeUxvY2soKSkKICAgICAgICAgICAgIHJldHVy bjsKLSAgICAgICAgTG9ja2VyIGhvbGRMb2NrIHsgQWRvcHRMb2NrVGFnIHsgfSwgbV9vdGhlckxv Y2sgfTsKKyAgICAgICAgTG9ja2VyIGhvbGRMb2NrIHsgYWRvcHRMb2NrLCBtX290aGVyTG9jayB9 OwogICAgICAgICBtX290aGVyVmFsdWUgPSB2YWx1ZTsKICAgICB9CiAgICAgLy8gVGhpcyBmdW5j dGlvbiBjYW4gYmUgdXNlZCB0byBtYW51YWxseSBjaGVjayB0aGF0IGNvbXBpbGUgZmFpbHMuCkBA IC02NCw0ICs2NCwzOCBAQCBURVNUKFdURl9DaGVja2VkTG9jaywgQ2hlY2tlZExvY2tDb21waWxl cykKICAgICB2Lm1heWJlU2V0T3RoZXJWYWx1ZSgzNCk7CiB9CiAKK1RFU1QoV1RGX0NoZWNrZWRM b2NrLCBDaGVja2VkTG9ja0ZvcmdvdEFkb3B0Q29tcGlsZUZhaWx1cmUxKQoreworICAgIENoZWNr ZWRMb2NrIGxvY2s7CisgICAgVU5VU0VEX1ZBUklBQkxFKGxvY2spOworICAgIC8vIFVuY29tbWVu dCB0byBjYXVzZSBjb21waWxlIGZhaWx1cmUuCisgICAgLy8gbG9jay50cnlMb2NrKCk7Cit9CisK K1RFU1QoV1RGX0NoZWNrZWRMb2NrLCBDaGVja2VkTG9ja0ZvcmdvdEFkb3B0Q29tcGlsZUZhaWx1 cmUyKQoreworICAgIENoZWNrZWRMb2NrIGxvY2s7CisgICAgaWYgKGxvY2sudHJ5TG9jaygpKSB7 CisgICAgICAgIExvY2tlciBsb2NrZXIgeyBhZG9wdExvY2ssIGxvY2sgfTsgLy8gQ29tbWVudCB0 byBjYXVzZSBjb21waWxlIGZhaWx1cmUuCisgICAgICAgIHJldHVybjsKKyAgICB9Cit9CisKK1RF U1QoV1RGX0NoZWNrZWRMb2NrLCBDaGVja2VkTG9ja0ZvcmdvdEFkb3B0Q29tcGlsZUZhaWx1cmUz KQoreworICAgIENoZWNrZWRMb2NrIGxvY2s7CisgICAgaWYgKCFsb2NrLnRyeUxvY2soKSkKKyAg ICAgICAgcmV0dXJuOworICAgIExvY2tlciBsb2NrZXIgeyBhZG9wdExvY2ssIGxvY2sgfTsgLy8g Q29tbWVudCB0byBjYXVzZSBjb21waWxlIGZhaWx1cmUuCit9CisKK1RFU1QoV1RGX0NoZWNrZWRM b2NrLCBDaGVja2VkTG9ja0ZvcmdvdEFkb3B0U2hvdWxkRmFpbENvbXBpbGVOb3RJbXBsZW1lbnRl ZDEpCit7CisgICAgLy8gVGhpcyBzaG91bGQgY2F1c2UgY29tcGlsZSBmYWlsdXJlIGJ1dCBkb2Vz IG5vdC4KKyAgICBDaGVja2VkTG9jayBsb2NrOworICAgIGJvb2wgdW51c2VkID0gbG9jay50cnlM b2NrKCk7CisgICAgRVhQRUNUX1RSVUUodW51c2VkKTsKK30KKworCiB9IC8vIG5hbWVzcGFjZSBU ZXN0V2ViS2l0QVBJCg==