In isolated tree mode, do not call NSAccessibilityUnregisterUniqueIdForUIElement until the isolated object is detached.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

In isolated tree mode, do not call NSAccessibilityUnregisterUniqueIdForUIElement until the isolated object is detached.

[Radar WebKit Bug Importer]

<rdar://problem/89992486>

[Andres Gonzalez]

Created attachment 454159 [details]
Patch

[chris fleizach]

Comment on attachment 454159 [details]
Patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:535
> +    if (!AXObjectCache::isIsolatedTreeEnabled())

I think we may want to verify that this wrapper DOES have an isolated object with it, otherwise its possible this object will only exist on the main thread and never get an isolated element

[Andres Gonzalez]

(In reply to chris fleizach from comment #3)
> Comment on attachment 454159 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454159&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:535
> > +    if (!AXObjectCache::isIsolatedTreeEnabled())
> 
> I think we may want to verify that this wrapper DOES have an isolated object
> with it, otherwise its possible this object will only exist on the main
> thread and never get an isolated element

The problem with accessing the wrapper's m_isolatedObject here is that this is on the main thread, and m_isolatedObject should only be accessed on the AX thread after creation.

[chris fleizach]

Comment on attachment 454159 [details]
Patch

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

>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:535
>>> +    if (!AXObjectCache::isIsolatedTreeEnabled())
>> 
>> I think we may want to verify that this wrapper DOES have an isolated object with it, otherwise its possible this object will only exist on the main thread and never get an isolated element
> 
> The problem with accessing the wrapper's m_isolatedObject here is that this is on the main thread, and m_isolatedObject should only be accessed on the AX thread after creation.

can we check for its presence safely? just doing a read on the pointer value?

[Andres Gonzalez]

(In reply to chris fleizach from comment #5)
> Comment on attachment 454159 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454159&action=review
> 
> >>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:535
> >>> +    if (!AXObjectCache::isIsolatedTreeEnabled())
> >> 
> >> I think we may want to verify that this wrapper DOES have an isolated object with it, otherwise its possible this object will only exist on the main thread and never get an isolated element
> > 
> > The problem with accessing the wrapper's m_isolatedObject here is that this is on the main thread, and m_isolatedObject should only be accessed on the AX thread after creation.
> 
> can we check for its presence safely? just doing a read on the pointer value?

There is no guaranty that we can read that pointer on the main thread. The AX thread could be writing to it at that point, and thus you would read garbage. For instance, in the AXObjectCache we have:

#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
    if (m_pageID) {
        if (auto tree = AXIsolatedTree::treeForPageID(*m_pageID))
            tree->removeNode(*object);
    }
#endif

    object->detach(AccessibilityDetachmentType::ElementDestroyed);

tree->removeNode will schedule the isolated object to be detached, so when the live object detach is called, the applyPendingChanges may be detaching the isolated object.

I think we would need to add a variable to be accessed on the main thread to signal whether the isolated object was attached or not.

[Andres Gonzalez]

(In reply to Andres Gonzalez from comment #6)
> (In reply to chris fleizach from comment #5)
> > Comment on attachment 454159 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=454159&action=review
> > 
> > >>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:535
> > >>> +    if (!AXObjectCache::isIsolatedTreeEnabled())
> > >> 
> > >> I think we may want to verify that this wrapper DOES have an isolated object with it, otherwise its possible this object will only exist on the main thread and never get an isolated element
> > > 
> > > The problem with accessing the wrapper's m_isolatedObject here is that this is on the main thread, and m_isolatedObject should only be accessed on the AX thread after creation.
> > 
> > can we check for its presence safely? just doing a read on the pointer value?
> 
> There is no guaranty that we can read that pointer on the main thread. The
> AX thread could be writing to it at that point, and thus you would read
> garbage. For instance, in the AXObjectCache we have:
> 
> #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
>     if (m_pageID) {
>         if (auto tree = AXIsolatedTree::treeForPageID(*m_pageID))
>             tree->removeNode(*object);
>     }
> #endif
> 
>     object->detach(AccessibilityDetachmentType::ElementDestroyed);
> 
> tree->removeNode will schedule the isolated object to be detached, so when
> the live object detach is called, the applyPendingChanges may be detaching
> the isolated object.
> 
> I think we would need to add a variable to be accessed on the main thread to
> signal whether the isolated object was attached or not.

Maybe if I revert the order in the AXObjectCache snippet above, then it will be fine to read that pointer on the main thread.

[chris fleizach]

Comment on attachment 454159 [details]
Patch

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

>>>>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:535
>>>>>> +    if (!AXObjectCache::isIsolatedTreeEnabled())
>>>>> 
>>>>> I think we may want to verify that this wrapper DOES have an isolated object with it, otherwise its possible this object will only exist on the main thread and never get an isolated element
>>>> 
>>>> The problem with accessing the wrapper's m_isolatedObject here is that this is on the main thread, and m_isolatedObject should only be accessed on the AX thread after creation.
>>> 
>>> can we check for its presence safely? just doing a read on the pointer value?
>> 
>> There is no guaranty that we can read that pointer on the main thread. The AX thread could be writing to it at that point, and thus you would read garbage. For instance, in the AXObjectCache we have:
>> 
>> #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
>>     if (m_pageID) {
>>         if (auto tree = AXIsolatedTree::treeForPageID(*m_pageID))
>>             tree->removeNode(*object);
>>     }
>> #endif
>> 
>>     object->detach(AccessibilityDetachmentType::ElementDestroyed);
>> 
>> tree->removeNode will schedule the isolated object to be detached, so when the live object detach is called, the applyPendingChanges may be detaching the isolated object.
>> 
>> I think we would need to add a variable to be accessed on the main thread to signal whether the isolated object was attached or not.
> 
> Maybe if I revert the order in the AXObjectCache snippet above, then it will be fine to read that pointer on the main thread.

Or adding a tracking bit seems ok too

[Andres Gonzalez]

(In reply to chris fleizach from comment #8)
> Comment on attachment 454159 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454159&action=review
> 
> >>>>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:535
> >>>>>> +    if (!AXObjectCache::isIsolatedTreeEnabled())
> >>>>> 
> >>>>> I think we may want to verify that this wrapper DOES have an isolated object with it, otherwise its possible this object will only exist on the main thread and never get an isolated element
> >>>> 
> >>>> The problem with accessing the wrapper's m_isolatedObject here is that this is on the main thread, and m_isolatedObject should only be accessed on the AX thread after creation.
> >>> 
> >>> can we check for its presence safely? just doing a read on the pointer value?
> >> 
> >> There is no guaranty that we can read that pointer on the main thread. The AX thread could be writing to it at that point, and thus you would read garbage. For instance, in the AXObjectCache we have:
> >> 
> >> #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> >>     if (m_pageID) {
> >>         if (auto tree = AXIsolatedTree::treeForPageID(*m_pageID))
> >>             tree->removeNode(*object);
> >>     }
> >> #endif
> >> 
> >>     object->detach(AccessibilityDetachmentType::ElementDestroyed);
> >> 
> >> tree->removeNode will schedule the isolated object to be detached, so when the live object detach is called, the applyPendingChanges may be detaching the isolated object.
> >> 
> >> I think we would need to add a variable to be accessed on the main thread to signal whether the isolated object was attached or not.
> > 
> > Maybe if I revert the order in the AXObjectCache snippet above, then it will be fine to read that pointer on the main thread.
> 
> Or adding a tracking bit seems ok too

Yes, that is actually the only choice because the m_isolatedObject can be written to not only in detach but in AXIsolatedObject::attachPlatformWrapper which is called during updates in applyPendingChanges on the AX thread, so it can be called at any time.

[Andres Gonzalez]

Created attachment 454245 [details]
Patch

[chris fleizach]

Comment on attachment 454245 [details]
Patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:290
> +    m_isolatedObjectInitialized = false;

do we need this? won't it be false by default

[Andres Gonzalez]

Created attachment 454249 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #11)
> Comment on attachment 454245 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454245&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:290
> > +    m_isolatedObjectInitialized = false;
> 
> do we need this? won't it be false by default

Right, ObjC does that! Fixed in latest rev.

[EWS]

Committed r291084 (248246@main): <https://commits.webkit.org/248246@main>

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