198319 2019-05-28 18:01:36 -0700 Add write barrier to JSValueInWrappedObject for garbage collection 2021-03-23 17:01:04 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 sihui_liu sihui_liu cdumez darin fpizlo ggaren mark.lam rniwa sam webkit-bug-importer oldest_to_newest 1539841 0 sihui_liu 2019-05-28 18:01:36 -0700 As title. 1539844 1 370818 sihui_liu 2019-05-28 18:05:31 -0700 Created attachment 370818 Patch 1539934 2 370818 sam 2019-05-29 06:39:53 -0700 Comment on attachment 370818 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370818&action=review > Source/WebCore/ChangeLog:9 > + Add write barrier to JSValueInWrappedObject for garbage collection > + https://bugs.webkit.org/show_bug.cgi?id=198319 > + > + Reviewed by NOBODY (OOPS!). > + > + * bindings/js/JSValueInWrappedObject.h: > + (WebCore::cachedPropertyValue): What issue is this fixing? Can a regression test be added? 1742660 3 370818 rniwa 2021-03-22 21:03:46 -0700 Comment on attachment 370818 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370818&action=review >> Source/WebCore/ChangeLog:9 >> + (WebCore::cachedPropertyValue): > > What issue is this fixing? Can a regression test be added? Without a write barrier, it's possible that "value" can be collected before value can be prematurely collected. We also have an issue with JSValueInWrappedObject's m_value using Variant without any lock. Since m_value can be read concurrency in a GC thread, that is also unsafe. 1742672 4 370818 fpizlo 2021-03-22 21:53:24 -0700 Comment on attachment 370818 Patch This looks good to me. I don’t expect you to write a test for missing write barriers. 1742825 5 370818 darin 2021-03-23 09:06:56 -0700 Comment on attachment 370818 Patch I’m excited that using a class for this means we can fix this across the entire project with a single patch. In fact it is part of what I intended when I created this class, which tried to standardize an idiom that was spread out inconsistently across a lot of code. Like Sam, I’d like to know more about how we can tell if we’ve done it right and detect future mistakes. Even if it’s not a something we can make a regression test for, what technique can we use to increase our confidence? 1742917 6 424042 sihui_liu 2021-03-23 11:27:53 -0700 Created attachment 424042 Patch for landing 1742944 7 ews-feeder 2021-03-23 12:19:49 -0700 Committed r274890: <https://commits.webkit.org/r274890> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424042. 1742945 8 webkit-bug-importer 2021-03-23 12:20:21 -0700 <rdar://problem/75749358> 1743074 9 rniwa 2021-03-23 17:01:04 -0700 (In reply to Darin Adler from comment #5) > Comment on attachment 370818 [details] > Patch > > I’m excited that using a class for this means we can fix this across the > entire project with a single patch. In fact it is part of what I intended > when I created this class, which tried to standardize an idiom that was > spread out inconsistently across a lot of code. > > Like Sam, I’d like to know more about how we can tell if we’ve done it right > and detect future mistakes. Even if it’s not a something we can make a > regression test for, what technique can we use to increase our confidence? I think this is a tough question. It's akin to using a raw pointer without realizing that an object could have been deleted before the pointer value is cleared. Other than things like ASAN and GuardMalloc, there isn't really a way for us to detect such a bad pointer. I think the situation is similar here and harder because we don't have an equivalent tool. Perhaps, we can do something like what we're trying to do with the smart pointer and come up with some kind of rule we can just follow? 370818 2019-05-28 18:05:31 -0700 2021-03-23 11:27:50 -0700 Patch bug-198319-20190528180531.patch text/plain 1498 sihui_liu U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ1ODI0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYzJmN2M2OGZhZjg5YjBi ZTQ2ODVlMzE4YTY4MjcyN2FmZDZhZDZkZC4uNzU1MjE0ZGM4MzA5YjRmOTFkZWM4MmNjOGFlNDk1 Njc0MjU5ODIzNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDE5LTA1LTI4ICBTaWh1 aSBMaXUgIDxzaWh1aV9saXVAYXBwbGUuY29tPgorCisgICAgICAgIEFkZCB3cml0ZSBiYXJyaWVy IHRvIEpTVmFsdWVJbldyYXBwZWRPYmplY3QgZm9yIGdhcmJhZ2UgY29sbGVjdGlvbgorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTk4MzE5CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBiaW5kaW5ncy9qcy9K U1ZhbHVlSW5XcmFwcGVkT2JqZWN0Lmg6CisgICAgICAgIChXZWJDb3JlOjpjYWNoZWRQcm9wZXJ0 eVZhbHVlKToKKwogMjAxOS0wNS0yOCAgQnJlbnQgRnVsZ2hhbSAgPGJmdWxnaGFtQGFwcGxlLmNv bT4KIAogICAgICAgICBQcm90ZWN0IGZyYW1lcyBkdXJpbmcgc3R5bGUgYW5kIGxheW91dCBjaGFu Z2VzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9qcy9KU1ZhbHVlSW5XcmFw cGVkT2JqZWN0LmggYi9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9qcy9KU1ZhbHVlSW5XcmFwcGVk T2JqZWN0LmgKaW5kZXggOWY1ZGRhMjdlMjY2NjliMjY3ZmQyNDY1ZDg3NWM1YWE4NGM1NjkzYi4u NDNiNWQwMjIwY2QyOTFkOWE2NTYzYzRkYTU4MGY0YTMzZDg1MWU0YSAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYkNvcmUvYmluZGluZ3MvanMvSlNWYWx1ZUluV3JhcHBlZE9iamVjdC5oCisrKyBiL1Nv dXJjZS9XZWJDb3JlL2JpbmRpbmdzL2pzL0pTVmFsdWVJbldyYXBwZWRPYmplY3QuaApAQCAtMTE4 LDcgKzExOCw5IEBAIGlubGluZSBKU0M6OkpTVmFsdWUgY2FjaGVkUHJvcGVydHlWYWx1ZShKU0M6 OkV4ZWNTdGF0ZSYgc3RhdGUsIGNvbnN0IEpTRE9NT2JqZWN0CiB7CiAgICAgaWYgKGNhY2hlZFZh bHVlICYmIGlzV29ybGRDb21wYXRpYmxlKHN0YXRlLCBjYWNoZWRWYWx1ZSkpCiAgICAgICAgIHJl dHVybiBjYWNoZWRWYWx1ZTsKLSAgICBjYWNoZWRWYWx1ZSA9IGNsb25lQWNyb3NzV29ybGRzKHN0 YXRlLCBvd25lciwgZnVuY3Rpb24oKSk7CisgICAgYXV0byB2YWx1ZSA9IGZ1bmN0aW9uKCk7Cisg ICAgY2FjaGVkVmFsdWUgPSBjbG9uZUFjcm9zc1dvcmxkcyhzdGF0ZSwgb3duZXIsIHZhbHVlKTsK KyAgICBzdGF0ZS52bSgpLmhlYXAud3JpdGVCYXJyaWVyKCZvd25lciwgdmFsdWUpOwogICAgIEFT U0VSVChpc1dvcmxkQ29tcGF0aWJsZShzdGF0ZSwgY2FjaGVkVmFsdWUpKTsKICAgICByZXR1cm4g Y2FjaGVkVmFsdWU7CiB9Cg== 424042 2021-03-23 11:27:53 -0700 2021-03-23 12:19:51 -0700 Patch for landing bug-198319-20210323112752.patch text/plain 1644 sihui_liu U3VidmVyc2lvbiBSZXZpc2lvbjogMjc0ODc4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggM2Y2ZDk0NzkzODYyNmQx N2EwMDA4MzZhNTY0NjE1M2Q3NTE0NWFmYy4uODViNDdkNzIyNjg1MGZmNmE0MGMyZjU1YTQ4YjJl Yjg4MmZhZGJkMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDIxLTAzLTIzICBTaWh1 aSBMaXUgIDxzaWh1aV9saXVAYXBwbGUuY29tPgorCisgICAgICAgIEFkZCB3cml0ZSBiYXJyaWVy IHRvIEpTVmFsdWVJbldyYXBwZWRPYmplY3QgZm9yIGdhcmJhZ2UgY29sbGVjdGlvbgorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTk4MzE5CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgRmlsaXAgUGl6bG8uCisKKyAgICAgICAgKiBiaW5kaW5ncy9qcy9KU1Zh bHVlSW5XcmFwcGVkT2JqZWN0Lmg6CisgICAgICAgIChXZWJDb3JlOjpjYWNoZWRQcm9wZXJ0eVZh bHVlKToKKwogMjAyMS0wMy0yMyAgUGVuZyBMaXUgIDxwZW5nLmxpdTZAYXBwbGUuY29tPgogCiAg ICAgICAgIEVuYWJsZSB0aGUgcXVpcmsgdG8gZGlzYWJsZSBmaXJpbmcgIndlYmtpdGVuZGZ1bGxz Y3JlZW4iIGV2ZW50IHdoZW4gYSB2aWRlbyBlbnRlcnMgcGljdHVyZS1pbi1waWN0dXJlIGZyb20g ZnVsbHNjcmVlbiBvbiBlc3BuLmNvbQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvYmluZGlu Z3MvanMvSlNWYWx1ZUluV3JhcHBlZE9iamVjdC5oIGIvU291cmNlL1dlYkNvcmUvYmluZGluZ3Mv anMvSlNWYWx1ZUluV3JhcHBlZE9iamVjdC5oCmluZGV4IGE3NDFlMmRhYWEwZTVhZDVhZGM5NjU1 MDgwZjc0Y2NjOTRhYjRiNjIuLmQ3ZTUyYjFiMWQxNmQyY2IxNzIyOTQzYTJiNmUwOGRhNTI0M2Qz M2YgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL2pzL0pTVmFsdWVJbldyYXBw ZWRPYmplY3QuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9qcy9KU1ZhbHVlSW5XcmFw cGVkT2JqZWN0LmgKQEAgLTEyMiw3ICsxMjIsOSBAQCBpbmxpbmUgSlNDOjpKU1ZhbHVlIGNhY2hl ZFByb3BlcnR5VmFsdWUoSlNDOjpKU0dsb2JhbE9iamVjdCYgbGV4aWNhbEdsb2JhbE9iamVjdAog ewogICAgIGlmIChjYWNoZWRWYWx1ZSAmJiBpc1dvcmxkQ29tcGF0aWJsZShsZXhpY2FsR2xvYmFs T2JqZWN0LCBjYWNoZWRWYWx1ZSkpCiAgICAgICAgIHJldHVybiBjYWNoZWRWYWx1ZTsKLSAgICBj YWNoZWRWYWx1ZSA9IGNsb25lQWNyb3NzV29ybGRzKGxleGljYWxHbG9iYWxPYmplY3QsIG93bmVy LCBmdW5jdGlvbigpKTsKKyAgICBhdXRvIHZhbHVlID0gZnVuY3Rpb24oKTsKKyAgICBjYWNoZWRW YWx1ZSA9IGNsb25lQWNyb3NzV29ybGRzKGxleGljYWxHbG9iYWxPYmplY3QsIG93bmVyLCB2YWx1 ZSk7CisgICAgbGV4aWNhbEdsb2JhbE9iamVjdC52bSgpLmhlYXAud3JpdGVCYXJyaWVyKCZvd25l ciwgdmFsdWUpOwogICAgIEFTU0VSVChpc1dvcmxkQ29tcGF0aWJsZShsZXhpY2FsR2xvYmFsT2Jq ZWN0LCBjYWNoZWRWYWx1ZSkpOwogICAgIHJldHVybiBjYWNoZWRWYWx1ZTsKIH0K