206765 2020-01-24 12:27:15 -0800 Crash in AXIsolatedObject::tagName. 2020-01-25 11:45:54 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 andresg_22 andresg_22 aboxhall apinheiro cfleizach commit-queue darin dmazzoni ews-watchlist jcraig jdiggs samuel_white webkit-bug-importer oldest_to_newest 1610819 0 andresg_22 2020-01-24 12:27:15 -0800 Crash in AXIsolatedObject::tagName. 1610820 1 388718 andresg_22 2020-01-24 12:33:58 -0800 Created attachment 388718 Patch 1610852 2 388723 andresg_22 2020-01-24 14:01:31 -0800 Created attachment 388723 Patch 1610867 3 commit-queue 2020-01-24 14:51:41 -0800 The commit-queue encountered the following flaky tests while processing attachment 388723: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch. 1610868 4 388723 commit-queue 2020-01-24 14:52:12 -0800 Comment on attachment 388723 Patch Clearing flags on attachment: 388723 Committed r255095: <https://trac.webkit.org/changeset/255095> 1610869 5 commit-queue 2020-01-24 14:52:14 -0800 All reviewed patches have been landed. Closing bug. 1610870 6 webkit-bug-importer 2020-01-24 14:53:16 -0800 <rdar://problem/58882399> 1611066 7 388723 darin 2020-01-25 09:59:43 -0800 Comment on attachment 388723 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388723&action=review > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:207 > + setProperty(AXPropertyName::TagName, object.tagName().isolatedCopy()); I don’t understand why we need isolated copying here. Normally this is only needed when copying something across threads, but then only using it on first one thread and then another. I don’t think that applies here. I am worried that we are misusing it. 1611078 8 andresg_22 2020-01-25 11:03:45 -0800 (In reply to Darin Adler from comment #7) > Comment on attachment 388723 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388723&action=review > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:207 > > + setProperty(AXPropertyName::TagName, object.tagName().isolatedCopy()); > > I don’t understand why we need isolated copying here. Normally this is only > needed when copying something across threads, but then only using it on > first one thread and then another. I don’t think that applies here. I am > worried that we are misusing it. Darin, thanks for the question. We are isolated copying these strings in the main thread to cache these properties, and then they will be read in the accessibility secondary thread. My understanding was that we do need isolatedCopy for this scenario but I'm not sure, since I don't completely understand the threading model for the String class. Would you please clarify whether we need to use isolatedCopy for this case? 1611085 9 388723 darin 2020-01-25 11:45:54 -0800 Comment on attachment 388723 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388723&action=review >>> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:207 >>> + setProperty(AXPropertyName::TagName, object.tagName().isolatedCopy()); >> >> I don’t understand why we need isolated copying here. Normally this is only needed when copying something across threads, but then only using it on first one thread and then another. I don’t think that applies here. I am worried that we are misusing it. > > Darin, thanks for the question. We are isolated copying these strings in the main thread to cache these properties, and then they will be read in the accessibility secondary thread. My understanding was that we do need isolatedCopy for this scenario but I'm not sure, since I don't completely understand the threading model for the String class. Would you please clarify whether we need to use isolatedCopy for this case? Short answer: Good news, that sounds like it does work and is what isolatedCopy is designed for. The approach seems fine. Long answer: As long as the handoff is clean and all the subsequent accesses are only one that one single accessibility thread. The trickiest part is that you have to be sure that nothing ends up keeping a reference to something on the main thread and racing with the first access on the second thread. On the other hand, individual ad hoc calls to isolatedCopy for each string is likely a difficult-to-maintain-correctly approach for this. We have a header CrossThreadCopier.h, a struct template named CrossThreadCopier that we specialize as needed, and a function named crossThreadCopy that helps us do this correctly throughout WebCore, designed to work generically across types and efficiently do nothing for scalars. It’s probably better to do crossThreadCopy calls in the implementation of a setProperty template function rather than writing custom code for each property in the initializeAttributeData function. We’d want the template function that in turn calls the underlying function that constructs the Variant, if we don’t want to pay the cost of a runtime check of every value before doing the crossThreadCopy. That might make things slow for simple scalars. This could a slightly more elegant way to handle AXPropertyName::AccessibilityText, for example. And this makes sure that adding a future property automatically gets the isolated copying that is needed, based on the type of the property, not with separate thinking for each property. I see some small harmless errors in this AXIsolatedObject class. There is m_attributeMapLock, which is defined but never used. it’s strange that function has a return type of "const String". That’s really the same as "String"; the "const" doesn’t add anything. If anyone called the stringAttributeValue function, or any function that access an element of the Variant with single-thread-only reference counting, on more than one thread, there would be a problem. I don’t see any assertions checking that, but I will assume it’s guaranteed. Might be worth checking that just as the existing code already checks that setProperty is only called before initialization and on the main thread. 388718 2020-01-24 12:33:58 -0800 2020-01-24 14:01:29 -0800 Patch bug-206765-20200124153357.patch text/plain 2953 andresg_22 U3VidmVyc2lvbiBSZXZpc2lvbjogMjU1MDgwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOWFmY2FmNTcxOTk3MjE3 N2EwZTYyM2I3NTQ4ZWZhMTg0YWY1N2U5ZC4uNDkxMjgwZWY0ZTUyYzc2YTZhMDhkNTkyMmJhYmM3 NDk1ZWNmY2M4OCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDIwLTAxLTI0ICBBbmRy ZXMgR29uemFsZXogIDxhbmRyZXNnXzIyQGFwcGxlLmNvbT4KKworICAgICAgICBDcmFzaCBpbiBB WElzb2xhdGVkT2JqZWN0Ojp0YWdOYW1lLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MjA2NzY1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgRml4ZXMgY3Jhc2ggaW4gQVhJc29sYXRlZE9iamVjdDo6dGFnTmFt ZS4KKworICAgICAgICAqIGFjY2Vzc2liaWxpdHkvQWNjZXNzaWJpbGl0eU9iamVjdC5jcHA6Cisg ICAgICAgIChXZWJDb3JlOjpBY2Nlc3NpYmlsaXR5T2JqZWN0Ojp0YWdOYW1lIGNvbnN0KTovIE1h ZGUgb2J2aW91cyB0aGF0IEVsZW1lbnQ6OmxvY2FsTmFtZSByZXR1cm5zIGFuIEF0b21TdHJpbmcg YW5kIHRodXMgY2Fubm90IGNyb3NzIHRocmVhZCBib3VuZGFyaWVzLgorICAgICAgICAqIGFjY2Vz c2liaWxpdHkvaXNvbGF0ZWR0cmVlL0FYSXNvbGF0ZWRPYmplY3QuY3BwOgorICAgICAgICAoV2Vi Q29yZTo6QVhJc29sYXRlZE9iamVjdDo6aW5pdGlhbGl6ZUF0dHJpYnV0ZURhdGEpOiBXYXMgbWlz c2luZyB0aGUgaXNvbGF0ZWRDb3B5IHdoZW4gY2FjaGluZyB0aGUgdGFnTmFtZS4KKwogMjAyMC0w MS0yNCAgQW5kcmVzIEdvbnphbGV6ICA8YW5kcmVzZ18yMkBhcHBsZS5jb20+CiAKICAgICAgICAg SW1wbGVtZW50YXRpb24gb2YgQVhJc29sYXRlZE9iamVjdDo6aXNTdGF0aWNUZXh0IGFuZCBpc0xh bmRtYXJrIG1ldGhvZHMuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5 L0FjY2Vzc2liaWxpdHlPYmplY3QuY3BwIGIvU291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9B Y2Nlc3NpYmlsaXR5T2JqZWN0LmNwcAppbmRleCBkODAzMDQ0OTgyZTZhM2Q0YmVlNjBhYjUwODc3 YzY5MGM5YWU3YjM1Li45NGNlMjNlY2ZkOWI2NzE2ZDM1YTQ4NTIxZmEwOGU1ZWNiNTQzY2IwIDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L0FjY2Vzc2liaWxpdHlPYmpl Y3QuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkvQWNjZXNzaWJpbGl0eU9i amVjdC5jcHAKQEAgLTM0MjQsOCArMzQyNCwxMSBAQCB1aW50NjRfdCBBY2Nlc3NpYmlsaXR5T2Jq ZWN0OjpzZXNzaW9uSUQoKSBjb25zdAogCiBTdHJpbmcgQWNjZXNzaWJpbGl0eU9iamVjdDo6dGFn TmFtZSgpIGNvbnN0CiB7Ci0gICAgaWYgKEVsZW1lbnQqIGVsZW1lbnQgPSB0aGlzLT5lbGVtZW50 KCkpCi0gICAgICAgIHJldHVybiBlbGVtZW50LT5sb2NhbE5hbWUoKTsKKyAgICBpZiAoRWxlbWVu dCogZWxlbWVudCA9IHRoaXMtPmVsZW1lbnQoKSkgeworICAgICAgICAvLyBFbGVtZW50Ojpsb2Nh bE5hbWUgcmV0dXJucyBhIGNvbnN0IEF0b21TdHJpbmcmIHRoYXQgY2Fubm90IGNyb3NzCisgICAg ICAgIC8vIHRocmVhZCBib3VuZGFyaWVzLiBTbyBjb25zdHJ1Y3QgYSBTdHJpbmcgdGhhdCBjYW4g YmUgaXNvbGF0ZWRDb3BpZWQuCisgICAgICAgIHJldHVybiB7IGVsZW1lbnQtPmxvY2FsTmFtZSgp IH07CisgICAgfQogCiAgICAgcmV0dXJuIFN0cmluZygpOwogfQpkaWZmIC0tZ2l0IGEvU291cmNl L1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9pc29sYXRlZHRyZWUvQVhJc29sYXRlZE9iamVjdC5jcHAg Yi9Tb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2lzb2xhdGVkdHJlZS9BWElzb2xhdGVkT2Jq ZWN0LmNwcAppbmRleCA0ZjcxY2M3OGQ0MGVlZjA5MDk3YzljMzRkZGE3ZTEyZDZlMjk4ZWY3Li4y YTlmMzY2ZjEzZjYzMDc1ODk5M2MwNDUxMzYwY2VhZTNlNjYzZjc4IDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2lzb2xhdGVkdHJlZS9BWElzb2xhdGVkT2JqZWN0LmNw cAorKysgYi9Tb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2lzb2xhdGVkdHJlZS9BWElzb2xh dGVkT2JqZWN0LmNwcApAQCAtMjA0LDcgKzIwNCw3IEBAIHZvaWQgQVhJc29sYXRlZE9iamVjdDo6 aW5pdGlhbGl6ZUF0dHJpYnV0ZURhdGEoQVhDb3JlT2JqZWN0JiBvYmplY3QsIGJvb2wgaXNSb290 CiAgICAgc2V0UHJvcGVydHkoQVhQcm9wZXJ0eU5hbWU6Okxhbmd1YWdlLCBvYmplY3QubGFuZ3Vh Z2UoKSk7CiAgICAgc2V0UHJvcGVydHkoQVhQcm9wZXJ0eU5hbWU6OkNhbkhhdmVTZWxlY3RlZENo aWxkcmVuLCBvYmplY3QuY2FuSGF2ZVNlbGVjdGVkQ2hpbGRyZW4oKSk7CiAgICAgc2V0UHJvcGVy dHkoQVhQcm9wZXJ0eU5hbWU6Okhhc0FSSUFWYWx1ZU5vdywgb2JqZWN0Lmhhc0FSSUFWYWx1ZU5v dygpKTsKLSAgICBzZXRQcm9wZXJ0eShBWFByb3BlcnR5TmFtZTo6VGFnTmFtZSwgb2JqZWN0LnRh Z05hbWUoKSk7CisgICAgc2V0UHJvcGVydHkoQVhQcm9wZXJ0eU5hbWU6OlRhZ05hbWUsIG9iamVj dC50YWdOYW1lKCkuaXNvbGF0ZWRDb3B5KCkpOwogICAgIHNldFByb3BlcnR5KEFYUHJvcGVydHlO YW1lOjpTdXBwb3J0c0xpdmVSZWdpb24sIG9iamVjdC5zdXBwb3J0c0xpdmVSZWdpb24oKSk7CiAg ICAgc2V0UHJvcGVydHkoQVhQcm9wZXJ0eU5hbWU6OklzSW5zaWRlTGl2ZVJlZ2lvbiwgb2JqZWN0 LmlzSW5zaWRlTGl2ZVJlZ2lvbigpKTsKICAgICBzZXRQcm9wZXJ0eShBWFByb3BlcnR5TmFtZTo6 TGl2ZVJlZ2lvblN0YXR1cywgb2JqZWN0LmxpdmVSZWdpb25TdGF0dXMoKSk7Cg== 388723 2020-01-24 14:01:31 -0800 2020-01-24 14:52:12 -0800 Patch bug-206765-20200124170130.patch text/plain 1926 andresg_22 U3VidmVyc2lvbiBSZXZpc2lvbjogMjU1MDgwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOWFmY2FmNTcxOTk3MjE3 N2EwZTYyM2I3NTQ4ZWZhMTg0YWY1N2U5ZC4uODVkNDMxYTU0M2NiNGE2ZTY2YWIyOTllNmM2MWI2 NjcyZDEzNWRmNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDIwLTAxLTI0ICBBbmRy ZXMgR29uemFsZXogIDxhbmRyZXNnXzIyQGFwcGxlLmNvbT4KKworICAgICAgICBDcmFzaCBpbiBB WElzb2xhdGVkT2JqZWN0Ojp0YWdOYW1lLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MjA2NzY1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgRml4ZXMgY3Jhc2ggaW4gQVhJc29sYXRlZE9iamVjdDo6dGFnTmFt ZS4KKworICAgICAgICAqIGFjY2Vzc2liaWxpdHkvaXNvbGF0ZWR0cmVlL0FYSXNvbGF0ZWRPYmpl Y3QuY3BwOgorICAgICAgICAoV2ViQ29yZTo6QVhJc29sYXRlZE9iamVjdDo6aW5pdGlhbGl6ZUF0 dHJpYnV0ZURhdGEpOiBXYXMgbWlzc2luZyB0aGUgaXNvbGF0ZWRDb3B5IHdoZW4gY2FjaGluZyB0 aGUgdGFnTmFtZS4KKwogMjAyMC0wMS0yNCAgQW5kcmVzIEdvbnphbGV6ICA8YW5kcmVzZ18yMkBh cHBsZS5jb20+CiAKICAgICAgICAgSW1wbGVtZW50YXRpb24gb2YgQVhJc29sYXRlZE9iamVjdDo6 aXNTdGF0aWNUZXh0IGFuZCBpc0xhbmRtYXJrIG1ldGhvZHMuCmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2lzb2xhdGVkdHJlZS9BWElzb2xhdGVkT2JqZWN0LmNwcCBi L1NvdXJjZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkvaXNvbGF0ZWR0cmVlL0FYSXNvbGF0ZWRPYmpl Y3QuY3BwCmluZGV4IDRmNzFjYzc4ZDQwZWVmMDkwOTdjOWMzNGRkYTdlMTJkNmUyOThlZjcuLjJh OWYzNjZmMTNmNjMwNzU4OTkzYzA0NTEzNjBjZWFlM2U2NjNmNzggMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkvaXNvbGF0ZWR0cmVlL0FYSXNvbGF0ZWRPYmplY3QuY3Bw CisrKyBiL1NvdXJjZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkvaXNvbGF0ZWR0cmVlL0FYSXNvbGF0 ZWRPYmplY3QuY3BwCkBAIC0yMDQsNyArMjA0LDcgQEAgdm9pZCBBWElzb2xhdGVkT2JqZWN0Ojpp bml0aWFsaXplQXR0cmlidXRlRGF0YShBWENvcmVPYmplY3QmIG9iamVjdCwgYm9vbCBpc1Jvb3QK ICAgICBzZXRQcm9wZXJ0eShBWFByb3BlcnR5TmFtZTo6TGFuZ3VhZ2UsIG9iamVjdC5sYW5ndWFn ZSgpKTsKICAgICBzZXRQcm9wZXJ0eShBWFByb3BlcnR5TmFtZTo6Q2FuSGF2ZVNlbGVjdGVkQ2hp bGRyZW4sIG9iamVjdC5jYW5IYXZlU2VsZWN0ZWRDaGlsZHJlbigpKTsKICAgICBzZXRQcm9wZXJ0 eShBWFByb3BlcnR5TmFtZTo6SGFzQVJJQVZhbHVlTm93LCBvYmplY3QuaGFzQVJJQVZhbHVlTm93 KCkpOwotICAgIHNldFByb3BlcnR5KEFYUHJvcGVydHlOYW1lOjpUYWdOYW1lLCBvYmplY3QudGFn TmFtZSgpKTsKKyAgICBzZXRQcm9wZXJ0eShBWFByb3BlcnR5TmFtZTo6VGFnTmFtZSwgb2JqZWN0 LnRhZ05hbWUoKS5pc29sYXRlZENvcHkoKSk7CiAgICAgc2V0UHJvcGVydHkoQVhQcm9wZXJ0eU5h bWU6OlN1cHBvcnRzTGl2ZVJlZ2lvbiwgb2JqZWN0LnN1cHBvcnRzTGl2ZVJlZ2lvbigpKTsKICAg ICBzZXRQcm9wZXJ0eShBWFByb3BlcnR5TmFtZTo6SXNJbnNpZGVMaXZlUmVnaW9uLCBvYmplY3Qu aXNJbnNpZGVMaXZlUmVnaW9uKCkpOwogICAgIHNldFByb3BlcnR5KEFYUHJvcGVydHlOYW1lOjpM aXZlUmVnaW9uU3RhdHVzLCBvYmplY3QubGl2ZVJlZ2lvblN0YXR1cygpKTsK