<rdar://problem/89992486>

Created attachment 454159 [details]
Patch

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

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

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?

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

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

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

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

Created attachment 454245 [details]
Patch

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

Created attachment 454249 [details]
Patch

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

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