Bug 215790 – Crash in WebCore::AccessibilityRenderObject::textUnderElement in isolated tree mode.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andres Gonzalez]

Crash in WebCore::AccessibilityRenderObject::textUnderElement in isolated tree mode.

[Andres Gonzalez]

To reproduce:
1. browse to https://codepen.io/mra11yx/full/OJVzpBK
2. VoiceOver navigate to the radio buttons.
3. Press VO keys + SpaceBar to select a radio button.
Crash occurs.

[Andres Gonzalez]

Stack trace:
ASSERTION FAILED: !nodeDocument->childNeedsStyleRecalc()
./accessibility/AccessibilityRenderObject.cpp(667) : virtual WTF::String WebCore::AccessibilityRenderObject::textUnderElement(WebCore::AccessibilityTextUnderElementMode) const
1   0x4c2c1dd19 WTFCrash
2   0x4c2c1dd39 WTFCrashWithSecurityImplication
3   0x4a81bd0cd WebCore::AccessibilityRenderObject::textUnderElement(WebCore::AccessibilityTextUnderElementMode) const
4   0x4a81a6d2f WebCore::AccessibilityNodeObject::textUnderElement(WebCore::AccessibilityTextUnderElementMode) const
5   0x4a81bd2f8 WebCore::AccessibilityRenderObject::textUnderElement(WebCore::AccessibilityTextUnderElementMode) const
6   0x4a81a4592 WebCore::accessibleNameForNode(WebCore::Node*, WebCore::Node*)
7   0x4a81a3faa WebCore::AccessibilityNodeObject::textForLabelElement(WebCore::Element*) const
8   0x4a81a4789 WebCore::AccessibilityNodeObject::titleElementText(WTF::Vector<WebCore::AccessibilityText, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&) const
9   0x4a81c0018 WebCore::AccessibilityRenderObject::titleElementText(WTF::Vector<WebCore::AccessibilityText, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&) const
10  0x4a81a5e48 WebCore::AccessibilityNodeObject::accessibilityText(WTF::Vector<WebCore::AccessibilityText, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&) const
11  0x4a62aca40 WebCore::AccessibilityObject::descriptionAttributeValue() const
12  0x4a81fa900 WebCore::AXIsolatedObject::initializeAttributeData(WebCore::AXCoreObject&, bool)
13  0x4a81fa5cd WebCore::AXIsolatedObject::AXIsolatedObject(WebCore::AXCoreObject&, unsigned int, unsigned long)
14  0x4a820060b WebCore::AXIsolatedObject::AXIsolatedObject(WebCore::AXCoreObject&, unsigned int, unsigned long)
15  0x4a8200669 WebCore::AXIsolatedObject::create(WebCore::AXCoreObject&, unsigned int, unsigned long)
16  0x4a820aeb1 WebCore::AXIsolatedTree::updateNode(WebCore::AXCoreObject&)
17  0x4a814426d WebCore::AXObjectCache::updateIsolatedTree(WTF::Vector<std::__1::pair<WTF::RefPtr<WebCore::AXCoreObject, WTF::DumbPtrTraits<WebCore::AXCoreObject> >, WebCore::AXObjectCache::AXNotification>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&)
18  0x4a813cdb9 WebCore::AXObjectCache::notificationPostTimerFired()
19  0x4a816a487 decltype(*(std::__1::forward<WebCore::AXObjectCache*&>(fp0)).*fp()) std::__1::__invoke<void (WebCore::AXObjectCache::*&)(), WebCore::AXObjectCache*&, void>(void (WebCore::AXObjectCache::*&)(), WebCore::AXObjectCache*&)
20  0x4a816a400 std::__1::__bind_return<void (WebCore::AXObjectCache::*)(), std::__1::tuple<WebCore::AXObjectCache*>, std::__1::tuple<>, __is_valid_bind_return<void (WebCore::AXObjectCache::*)(), std::__1::tuple<WebCore::AXObjectCache*>, std::__1::tuple<> >::value>::type std::__1::__apply_functor<void (WebCore::AXObjectCache::*)(), std::__1::tuple<WebCore::AXObjectCache*>, 0ul, std::__1::tuple<> >(void (WebCore::AXObjectCache::*&)(), std::__1::tuple<WebCore::AXObjectCache*>&, std::__1::__tuple_indices<0ul>, std::__1::tuple<>&&)
21  0x4a816a3b9 std::__1::__bind_return<void (WebCore::AXObjectCache::*)(), std::__1::tuple<WebCore::AXObjectCache*>, std::__1::tuple<>, __is_valid_bind_return<void (WebCore::AXObjectCache::*)(), std::__1::tuple<WebCore::AXObjectCache*>, std::__1::tuple<> >::value>::type std::__1::__bind<void (WebCore::AXObjectCache::*&)(), WebCore::AXObjectCache*>::operator()<>()
22  0x4a816a35e WTF::Detail::CallableWrapper<std::__1::__bind<void (WebCore::AXObjectCache::*&)(), WebCore::AXObjectCache*>, void>::call()
23  0x4a5c99632 WTF::Function<void ()>::operator()() const
24  0x4a5cceafe WebCore::Timer::fired()
25  0x4a98715d4 WebCore::ThreadTimers::sharedTimerFiredInternal()
26  0x4a98790d1 WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0::operator()() const
27  0x4a987907e WTF::Detail::CallableWrapper<WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0, void>::call()
28  0x4a5c99632 WTF::Function<void ()>::operator()() const
29  0x4a9836fab WebCore::MainThreadSharedTimer::fired()
30  0x4a98d4626 WebCore::timerFired(__CFRunLoopTimer*, void*)
31  0x7fff2902d2eb __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
2020-08-24 21:26:51.275 MiniBrowser[58230:1154106] WebContent process crashed; reloading

[Andres Gonzalez]

Created attachment 407165 [details]
Patch

[Darin Adler]

Comment on attachment 407165 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407165&action=review

Changes here seem good, but the mix of the real fix and name changes makes this hard to review. Maybe separate out the name changes into a patch that has no behavior change?

> Source/WebCore/accessibility/AccessibilityObject.h:76
>  struct AccessibilityText {
> -    String text;
> -    AccessibilityTextSource textSource;
> -    
> -    AccessibilityText(const String& t, const AccessibilityTextSource& s)
> -        : text(t)
> -        , textSource(s)
> +    String m_text;
> +    AccessibilityTextSource m_source;
> +
> +    AccessibilityText(const String& text, AccessibilityTextSource source)
> +        : m_text(text)
> +        , m_source(source)
>      { }
>  };

These name changes are not a good idea.

Generally, public data members do not have "m_" prefixes. That’s why these didn’t have them.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:490
> +            std::for_each(change.m_properties.begin(), change.m_properties.end(), [&object] (auto&& property) {
> +                object->setProperty(property.key, WTFMove(property.value));
> +            });

Why use for_each here instead of a range-based for loop?

    for (auto& property : change.m_properties)
        object->setProperty(property.key, WTFMove(property.value));

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:315
> +    AXID m_axID { InvalidAXID }; // ID of the object whose properties changed.
> +    AXPropertyMap m_properties; // Changed properties.

These should have structure-style names, without the "m_" prefix. We use the "m_" prefix for private or protected data members of classes, not for structure members.

[Andres Gonzalez]

Created attachment 407199 [details]
Patch

[Andres Gonzalez]

(In reply to Darin Adler from comment #4)
> Comment on attachment 407165 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407165&action=review
> 
> Changes here seem good, but the mix of the real fix and name changes makes
> this hard to review. Maybe separate out the name changes into a patch that
> has no behavior change?

Done, the patch now contains only the changes related to the crash fix, code cleanup and renaming will be separate.

> 
> > Source/WebCore/accessibility/AccessibilityObject.h:76
> >  struct AccessibilityText {
> > -    String text;
> > -    AccessibilityTextSource textSource;
> > -    
> > -    AccessibilityText(const String& t, const AccessibilityTextSource& s)
> > -        : text(t)
> > -        , textSource(s)
> > +    String m_text;
> > +    AccessibilityTextSource m_source;
> > +
> > +    AccessibilityText(const String& text, AccessibilityTextSource source)
> > +        : m_text(text)
> > +        , m_source(source)
> >      { }
> >  };
> 
> These name changes are not a good idea.
> 
> Generally, public data members do not have "m_" prefixes. That’s why these
> didn’t have them.


Reverted, for some reason I had engraved on my mind that we use m_ for struct members as well. Will correct NodeChange structure that I had submitted before to also remove m_ prefixes.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:490
> > +            std::for_each(change.m_properties.begin(), change.m_properties.end(), [&object] (auto&& property) {
> > +                object->setProperty(property.key, WTFMove(property.value));
> > +            });
> 
> Why use for_each here instead of a range-based for loop?
> 
>     for (auto& property : change.m_properties)
>         object->setProperty(property.key, WTFMove(property.value));

Fixed. Wasn't sure that the map could be iterated like that.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:315
> > +    AXID m_axID { InvalidAXID }; // ID of the object whose properties changed.
> > +    AXPropertyMap m_properties; // Changed properties.
> 
> These should have structure-style names, without the "m_" prefix. We use the
> "m_" prefix for private or protected data members of classes, not for
> structure members.

Fixed.

[Darin Adler]

Comment on attachment 407165 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407165&action=review

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:82
> +#if PLATFORM(COCOA) && !PLATFORM(IOS_FAMILY)

I think #if PLATFORM(MAC) is a better way to write this.

[Darin Adler]

Comment on attachment 407199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407199&action=review

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:49
> +    None = 0,

Do we really need a None value? What is it used for?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:82
> +#if PLATFORM(COCOA) && !PLATFORM(IOS_FAMILY)

I suggest #if PLATFORM(MAC) for greater clarity.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:310
> +typedef std::pair<AXID, AXID> AccessibilityIsolatedTreeMathMultiscriptPair;

New code should use "using" instead of "typedef".

[EWS]

Committed r266136: <https://trac.webkit.org/changeset/266136>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407199 [details].

[Radar WebKit Bug Importer]

<rdar://problem/67750039>