157864 2016-05-18 15:18:15 -0700 Code that null checks the VM pointer before any use should ref the VM. 2016-05-19 14:04:16 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Local Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mark.lam mark.lam benjamin commit-queue fpizlo ggaren keith_miller msaboff saam webkit-bug-importer oldest_to_newest 1194448 0 mark.lam 2016-05-18 15:18:15 -0700 Specifically, in JSLock::willReleaseLock() and HeapTimer::timerDidFire(). Otherwise, there's no guarantee that the VM won't be deleted after the null check. Patch coming. 1194451 1 mark.lam 2016-05-18 15:21:28 -0700 <rdar://problem/26129156> 1194466 2 279304 mark.lam 2016-05-18 15:45:45 -0700 Created attachment 279304 proposed patch. Still need to run tests. 1194467 3 commit-queue 2016-05-18 15:48:00 -0700 Attachment 279304 did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSLock.cpp:180: 'vm' is incorrectly named. It should be named 'protector' or 'protectedVm'. [readability/naming/protected] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style. 1194469 4 279304 fpizlo 2016-05-18 15:49:04 -0700 Comment on attachment 279304 proposed patch. Nice! 1194475 5 279304 keith_miller 2016-05-18 15:54:31 -0700 Comment on attachment 279304 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=279304&action=review > Source/JavaScriptCore/ChangeLog:3 > + Code that need to null check the VM pointer before use should ref the VM. I think this would be less confusing as "Code that null checks the VM pointer before any use should ref the VM." 1194476 6 279304 keith_miller 2016-05-18 15:55:00 -0700 Comment on attachment 279304 proposed patch. r=me too. 1194502 7 mark.lam 2016-05-18 16:27:10 -0700 (In reply to comment #5) > Comment on attachment 279304 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=279304&action=review > > > Source/JavaScriptCore/ChangeLog:3 > > + Code that need to null check the VM pointer before use should ref the VM. > > I think this would be less confusing as "Code that null checks the VM > pointer before any use should ref the VM." I'll make the change. 1194933 8 mark.lam 2016-05-19 14:03:35 -0700 The patch has passed the layout tests and JSC tests on x86_64. I also did an ad hoc smoke test by running a few apps with it on ARM64. 1194934 9 mark.lam 2016-05-19 14:04:16 -0700 Landed in r201180: <http://trac.webkit.org/r201180>. 279304 2016-05-18 15:45:45 -0700 2016-05-18 15:55:00 -0700 proposed patch. bug-157864.patch text/plain 3736 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjAxMTAxKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI4IEBA CisyMDE2LTA1LTE4ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBD b2RlIHRoYXQgbmVlZCB0byBudWxsIGNoZWNrIHRoZSBWTSBwb2ludGVyIGJlZm9yZSB1c2Ugc2hv dWxkIHJlZiB0aGUgVk0uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0xNTc4NjQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICBKU0xvY2s6OndpbGxSZWxlYXNlTG9jaygpIGFuZCBIZWFwVGltZXI6OnRpbWVyRGlk RmlyZSgpIG5lZWQgdG8gcmVmZXJlbmNlIHRoZSBWTQorICAgICAgICB0aHJvdWdoIGEgUmVmUHRy LiAgT3RoZXJ3aXNlLCB0aGVyZSdzIG5vIGd1YXJhbnRlZSB0aGF0IHRoZSBWTSB3b24ndCBiZSBk ZWxldGVkCisgICAgICAgIGFmdGVyIHRoZWlyIG51bGwgY2hlY2tzLgorCisgICAgICAgICogYnl0 ZWNvZGUvQ29kZUJsb2NrLmg6CisgICAgICAgIChKU0M6OkNvZGVCbG9jazo6dm0pOgorICAgICAg ICAoSlNDOjpDb2RlQmxvY2s6OnNldFZNKTogRGVsZXRlZC4KKyAgICAgICAgLSBOb3QgdXNlZCwg YW5kIHN1Z2dlc3RzIHRoYXQgaXQgY2FuIGJlIGNoYW5nZWQgZHVyaW5nIHRoZSBsaWZldGltZSBv ZiB0aGUKKyAgICAgICAgICBDb2RlQmxvY2sgKHdoaWNoIHNob3VsZCBub3QgYmUpLgorCisgICAg ICAgICogaGVhcC9IZWFwVGltZXIuY3BwOgorICAgICAgICAoSlNDOjpIZWFwVGltZXI6OnRpbWVy RGlkRmlyZSk6CisgICAgICAgICogcnVudGltZS9KU0xvY2suY3BwOgorICAgICAgICAoSlNDOjpK U0xvY2s6OndpbGxSZWxlYXNlTG9jayk6CisgICAgICAgIC0gU3RvcmUgdGhlIFZNIHBvaW50ZXIg aW4gYSBSZWZQdHIgZmlyc3QsIGFuZCBudWxsIGNoZWNrIHRoZSBSZWZQdHIgaW5zdGVhZCBvZgor ICAgICAgICAgIHRoZSByYXcgVk0gcG9pbnRlci4gIFRoaXMgbWFrZXMgdGhlIG51bGwgY2hlY2sg YSBzdHJvbmcgZ3VhcmFudGVlIHRoYXQgdGhlCisgICAgICAgICAgVk0gcG9pbnRlciBpcyB2YWxp ZCB3aGlsZSB0aGVzZSBmdW5jdGlvbnMgYXJlIHVzaW5nIGl0LgorCiAyMDE2LTA1LTE4ICBZdXN1 a2UgU3V6dWtpICA8dXRhdGFuZS50ZWFAZ21haWwuY29tPgogCiAgICAgICAgIFtFUzZdIE5hbWVz cGFjZSBvYmplY3QgcmUtZXhwb3J0IHNob3VsZCBiZSBoYW5kbGVkIGFzIGxvY2FsIGV4cG9ydApJ bmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2J5dGVjb2RlL0NvZGVCbG9jay5oCj09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29kZS9Db2RlQmxvY2suaAkocmV2aXNp b24gMjAxMTAxKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2J5dGVjb2RlL0NvZGVCbG9jay5o CSh3b3JraW5nIGNvcHkpCkBAIC0zNDAsOCArMzQwLDcgQEAgcHVibGljOgogICAgIEV4ZWN1dGFi bGVCYXNlKiBvd25lckV4ZWN1dGFibGUoKSBjb25zdCB7IHJldHVybiBtX293bmVyRXhlY3V0YWJs ZS5nZXQoKTsgfQogICAgIFNjcmlwdEV4ZWN1dGFibGUqIG93bmVyU2NyaXB0RXhlY3V0YWJsZSgp IGNvbnN0IHsgcmV0dXJuIGpzQ2FzdDxTY3JpcHRFeGVjdXRhYmxlKj4obV9vd25lckV4ZWN1dGFi bGUuZ2V0KCkpOyB9CiAKLSAgICB2b2lkIHNldFZNKFZNKiB2bSkgeyBtX3ZtID0gdm07IH0KLSAg ICBWTSogdm0oKSB7IHJldHVybiBtX3ZtOyB9CisgICAgVk0qIHZtKCkgY29uc3QgeyByZXR1cm4g bV92bTsgfQogCiAgICAgdm9pZCBzZXRUaGlzUmVnaXN0ZXIoVmlydHVhbFJlZ2lzdGVyIHRoaXNS ZWdpc3RlcikgeyBtX3RoaXNSZWdpc3RlciA9IHRoaXNSZWdpc3RlcjsgfQogICAgIFZpcnR1YWxS ZWdpc3RlciB0aGlzUmVnaXN0ZXIoKSBjb25zdCB7IHJldHVybiBtX3RoaXNSZWdpc3RlcjsgfQpJ bmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvSGVhcFRpbWVyLmNwcAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9IZWFwVGltZXIuY3BwCShyZXZpc2lvbiAy MDExMDEpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9IZWFwVGltZXIuY3BwCSh3b3Jr aW5nIGNvcHkpCkBAIC04MCw5ICs4MCw5IEBAIHZvaWQgSGVhcFRpbWVyOjp0aW1lckRpZEZpcmUo Q0ZSdW5Mb29wVGkKICAgICBKU0xvY2sqIGFwaUxvY2sgPSBzdGF0aWNfY2FzdDxKU0xvY2sqPihj b250ZXh0KTsKICAgICBhcGlMb2NrLT5sb2NrKCk7CiAKLSAgICBWTSogdm0gPSBhcGlMb2NrLT52 bSgpOwotICAgIC8vIFRoZSBWTSBoYXMgYmVlbiBkZXN0cm95ZWQsIHNvIHdlIHNob3VsZCBqdXN0 IGdpdmUgdXAuCisgICAgUmVmUHRyPFZNPiB2bSA9IGFwaUxvY2stPnZtKCk7CiAgICAgaWYgKCF2 bSkgeworICAgICAgICAvLyBUaGUgVk0gaGFzIGJlZW4gZGVzdHJveWVkLCBzbyB3ZSBzaG91bGQg anVzdCBnaXZlIHVwLgogICAgICAgICBhcGlMb2NrLT51bmxvY2soKTsKICAgICAgICAgcmV0dXJu OwogICAgIH0KQEAgLTk4LDcgKzk4LDcgQEAgdm9pZCBIZWFwVGltZXI6OnRpbWVyRGlkRmlyZShD RlJ1bkxvb3BUaQogICAgICAgICBSRUxFQVNFX0FTU0VSVF9OT1RfUkVBQ0hFRCgpOwogCiAgICAg ewotICAgICAgICBKU0xvY2tIb2xkZXIgbG9ja2VyKHZtKTsKKyAgICAgICAgSlNMb2NrSG9sZGVy IGxvY2tlcih2bS5nZXQoKSk7CiAgICAgICAgIGhlYXBUaW1lci0+ZG9Xb3JrKCk7CiAgICAgfQog CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU0xvY2suY3BwCj09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTTG9jay5jcHAJKHJldmlzaW9u IDIwMTEwMSkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTTG9jay5jcHAJKHdv cmtpbmcgY29weSkKQEAgLTE3NywxMSArMTc3LDEyIEBAIHZvaWQgSlNMb2NrOjp1bmxvY2soaW50 cHRyX3QgdW5sb2NrQ291bnQKIAogdm9pZCBKU0xvY2s6OndpbGxSZWxlYXNlTG9jaygpCiB7Ci0g ICAgaWYgKG1fdm0pIHsKLSAgICAgICAgbV92bS0+ZHJhaW5NaWNyb3Rhc2tzKCk7CisgICAgUmVm UHRyPFZNPiB2bSA9IG1fdm07CisgICAgaWYgKHZtKSB7CisgICAgICAgIHZtLT5kcmFpbk1pY3Jv dGFza3MoKTsKIAotICAgICAgICBtX3ZtLT5oZWFwLnJlbGVhc2VEZWxheWVkUmVsZWFzZWRPYmpl Y3RzKCk7Ci0gICAgICAgIG1fdm0tPnNldFN0YWNrUG9pbnRlckF0Vk1FbnRyeShudWxscHRyKTsK KyAgICAgICAgdm0tPmhlYXAucmVsZWFzZURlbGF5ZWRSZWxlYXNlZE9iamVjdHMoKTsKKyAgICAg ICAgdm0tPnNldFN0YWNrUG9pbnRlckF0Vk1FbnRyeShudWxscHRyKTsKICAgICB9CiAKICAgICBp ZiAobV9lbnRyeUF0b21pY1N0cmluZ1RhYmxlKSB7Cg==