Bug 206765

Created attachment 388718 [details] Patch

Created attachment 388723 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 388723 [details]: 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.

Comment on attachment 388723 [details] Patch Clearing flags on attachment: 388723 Committed r255095: <https://trac.webkit.org/changeset/255095>

All reviewed patches have been landed. Closing bug.

<rdar://problem/58882399>

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.

(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?

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? 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.