WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
206765
Crash in AXIsolatedObject::tagName.
https://bugs.webkit.org/show_bug.cgi?id=206765
Summary
Crash in AXIsolatedObject::tagName.
Andres Gonzalez
Reported
2020-01-24 12:27:15 PST
Crash in AXIsolatedObject::tagName.
Attachments
Patch
(2.88 KB, patch)
2020-01-24 12:33 PST
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Patch
(1.88 KB, patch)
2020-01-24 14:01 PST
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Andres Gonzalez
Comment 1
2020-01-24 12:33:58 PST
Created
attachment 388718
[details]
Patch
Andres Gonzalez
Comment 2
2020-01-24 14:01:31 PST
Created
attachment 388723
[details]
Patch
WebKit Commit Bot
Comment 3
2020-01-24 14:51:41 PST
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.
WebKit Commit Bot
Comment 4
2020-01-24 14:52:12 PST
Comment on
attachment 388723
[details]
Patch Clearing flags on attachment: 388723 Committed
r255095
: <
https://trac.webkit.org/changeset/255095
>
WebKit Commit Bot
Comment 5
2020-01-24 14:52:14 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 6
2020-01-24 14:53:16 PST
<
rdar://problem/58882399
>
Darin Adler
Comment 7
2020-01-25 09:59:43 PST
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.
Andres Gonzalez
Comment 8
2020-01-25 11:03:45 PST
(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?
Darin Adler
Comment 9
2020-01-25 11:45:54 PST
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug