Bug 267348 – AX: ASSERTION FAILED: m_table inside of AXIsolatedTree::create

• Component: Accessibility
• Status: RESOLVED FIXED

[Dominic Mazzoni]

When I build WebKit on tip-of-tree in debug mode, then visit any webpage (even a simple Hello, world html page) with VoiceOver running, I get an assertion failure:

ASSERTION FAILED: m_table
/Volumes/Code/safari/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/HashTable.h(231)

The crash is happening inside of AXIsolatedTree::create at the top of this for loop:

    for (auto& relatedObjectID : axObjectCache.relations().keys()) {

That loop was added here, so I think it's likely a recent regression from that change:

    AX: Move objectsForIds off of the main thread
    https://bugs.webkit.org/show_bug.cgi?id=266608
    rdar://116133586

[Radar WebKit Bug Importer]

<rdar://problem/120788145>

[Joshua Hoffman]

Created attachment 469369 [details]
Patch

[Joshua Hoffman]

Created attachment 469370 [details]
Patch

[chris fleizach]

Comment on attachment 469370 [details]
Patch

How does this work? Won't getting a reference to the real thing still reference the underlying thing that can be modified?

[Joshua Hoffman]

(In reply to chris fleizach from comment #4)
> Comment on attachment 469370 [details]
> Patch
> 
> How does this work? Won't getting a reference to the real thing still
> reference the underlying thing that can be modified?

When we would call axObjectCache.relations(), we always had the chance of modifying m_relations since updateRelationsIsNeeded is part of that method. Now we prevent that possibility. We can hold a value if we want to be extra safe, but there is some space/time complexity added there—do you have a preference?

[chris fleizach]

(In reply to Joshua Hoffman from comment #5)
> (In reply to chris fleizach from comment #4)
> > Comment on attachment 469370 [details]
> > Patch
> > 
> > How does this work? Won't getting a reference to the real thing still
> > reference the underlying thing that can be modified?
> 
> When we would call axObjectCache.relations(), we always had the chance of
> modifying m_relations since updateRelationsIsNeeded is part of that method.
> Now we prevent that possibility. We can hold a value if we want to be extra
> safe, but there is some space/time complexity added there—do you have a
> preference?

I'm just saying isn't this a reference, so it can still be modified during iteration?

[Joshua Hoffman]

(In reply to chris fleizach from comment #6)
> (In reply to Joshua Hoffman from comment #5)
> > (In reply to chris fleizach from comment #4)
> > > Comment on attachment 469370 [details]
> > > Patch
> > > 
> > > How does this work? Won't getting a reference to the real thing still
> > > reference the underlying thing that can be modified?
> > 
> > When we would call axObjectCache.relations(), we always had the chance of
> > modifying m_relations since updateRelationsIsNeeded is part of that method.
> > Now we prevent that possibility. We can hold a value if we want to be extra
> > safe, but there is some space/time complexity added there—do you have a
> > preference?
> 
> I'm just saying isn't this a reference, so it can still be modified during
> iteration?

Yeah, that is true. I'll update it to `const auto relations =` so we use a copy instead.

[Joshua Hoffman]

Created attachment 469372 [details]
Patch

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #8)
> Created attachment 469372 [details]
> Patch

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index f6f0622abe93..c003ae7462a9 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -160,9 +160,10 @@ Ref<AXIsolatedTree> AXIsolatedTree::create(AXObjectCache& axObjectCache)

     tree->updateLoadingProgress(axObjectCache.loadingProgress());

-    tree->updateRelations(axObjectCache.relations());
+    const auto relations = axObjectCache.relations();
+    tree->updateRelations(relations);

-    for (auto& relatedObjectID : axObjectCache.relations().keys()) {
+    for (auto& relatedObjectID : relations.keys()) {
         RefPtr axObject = axObjectCache.objectForID(relatedObjectID);
         if (axObject && axObject->accessibilityIsIgnored())
             tree->addUnconnectedNode(axObject.releaseNonNull());

AG: I don't think we need this for loop at all since the unconnected iso objects are added in AXObjectCache::addRelation when relations are updated. I think this is another left over from a previous change that wasn't necessary. But I may be missing something...

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #9)
> (In reply to Joshua Hoffman from comment #8)
> > Created attachment 469372 [details]
> > Patch
> 
> diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> index f6f0622abe93..c003ae7462a9 100644
> --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> @@ -160,9 +160,10 @@ Ref<AXIsolatedTree>
> AXIsolatedTree::create(AXObjectCache& axObjectCache)
> 
>      tree->updateLoadingProgress(axObjectCache.loadingProgress());
> 
> -    tree->updateRelations(axObjectCache.relations());
> +    const auto relations = axObjectCache.relations();
> +    tree->updateRelations(relations);
> 
> -    for (auto& relatedObjectID : axObjectCache.relations().keys()) {
> +    for (auto& relatedObjectID : relations.keys()) {
>          RefPtr axObject = axObjectCache.objectForID(relatedObjectID);
>          if (axObject && axObject->accessibilityIsIgnored())
>              tree->addUnconnectedNode(axObject.releaseNonNull());
> 
> AG: I don't think we need this for loop at all since the unconnected iso
> objects are added in AXObjectCache::addRelation when relations are updated.
> I think this is another left over from a previous change that wasn't
> necessary. But I may be missing something...

We need this loop in addition to the code in addRelations so that if the isolated tree is enabled/initialized after relations have been added, we still create unconnected objects for the existing relations on the tree.

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #10)
> (In reply to Andres Gonzalez from comment #9)
> > (In reply to Joshua Hoffman from comment #8)
> > > Created attachment 469372 [details]
> > > Patch
> > 
> > diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> > b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> > index f6f0622abe93..c003ae7462a9 100644
> > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> > +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> > @@ -160,9 +160,10 @@ Ref<AXIsolatedTree>
> > AXIsolatedTree::create(AXObjectCache& axObjectCache)
> > 
> >      tree->updateLoadingProgress(axObjectCache.loadingProgress());
> > 
> > -    tree->updateRelations(axObjectCache.relations());
> > +    const auto relations = axObjectCache.relations();
> > +    tree->updateRelations(relations);
> > 
> > -    for (auto& relatedObjectID : axObjectCache.relations().keys()) {
> > +    for (auto& relatedObjectID : relations.keys()) {
> >          RefPtr axObject = axObjectCache.objectForID(relatedObjectID);
> >          if (axObject && axObject->accessibilityIsIgnored())
> >              tree->addUnconnectedNode(axObject.releaseNonNull());
> > 
> > AG: I don't think we need this for loop at all since the unconnected iso
> > objects are added in AXObjectCache::addRelation when relations are updated.
> > I think this is another left over from a previous change that wasn't
> > necessary. But I may be missing something...
> 
> We need this loop in addition to the code in addRelations so that if the
> isolated tree is enabled/initialized after relations have been added, we
> still create unconnected objects for the existing relations on the tree.

Wouldn't it make more sense to dirty the relations in that scenario?

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #11)
> (In reply to Joshua Hoffman from comment #10)
> > (In reply to Andres Gonzalez from comment #9)
> > > (In reply to Joshua Hoffman from comment #8)
> > > > Created attachment 469372 [details]
> > > > Patch
> > > 
> > > diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> > > b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> > > index f6f0622abe93..c003ae7462a9 100644
> > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> > > +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> > > @@ -160,9 +160,10 @@ Ref<AXIsolatedTree>
> > > AXIsolatedTree::create(AXObjectCache& axObjectCache)
> > > 
> > >      tree->updateLoadingProgress(axObjectCache.loadingProgress());
> > > 
> > > -    tree->updateRelations(axObjectCache.relations());
> > > +    const auto relations = axObjectCache.relations();
> > > +    tree->updateRelations(relations);
> > > 
> > > -    for (auto& relatedObjectID : axObjectCache.relations().keys()) {
> > > +    for (auto& relatedObjectID : relations.keys()) {
> > >          RefPtr axObject = axObjectCache.objectForID(relatedObjectID);
> > >          if (axObject && axObject->accessibilityIsIgnored())
> > >              tree->addUnconnectedNode(axObject.releaseNonNull());
> > > 
> > > AG: I don't think we need this for loop at all since the unconnected iso
> > > objects are added in AXObjectCache::addRelation when relations are updated.
> > > I think this is another left over from a previous change that wasn't
> > > necessary. But I may be missing something...
> > 
> > We need this loop in addition to the code in addRelations so that if the
> > isolated tree is enabled/initialized after relations have been added, we
> > still create unconnected objects for the existing relations on the tree.
> 
> Wouldn't it make more sense to dirty the relations in that scenario?

I worry that dirtying relations if they have already been created could add some time complexity issues, but definitely open to discussing that.

In terms of why adding the unconnected nodes here would be needed if ITM is already on, I added logging in both AXObjectCache::addRelation and AXIsolatedTree::createEmpty, and because of timing, we begin to add relations while the isolated tree is also being created, meaning we would miss out on some relations:

LOG: AXObjectCache::addRelation
LOG: AXObjectCache::addRelation
LOG: AXObjectCache::addRelation
LOG: AXObjectCache::addRelation
LOG: AXObjectCache::addRelation
LOG: AXIsolatedTree::createEmpty
LOG: AXIsolatedTree::create
LOG: AXObjectCache::addRelation
LOG: AXObjectCache::addRelation

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #12)
> (In reply to Andres Gonzalez from comment #11)
> > (In reply to Joshua Hoffman from comment #10)
> > > (In reply to Andres Gonzalez from comment #9)
> > > > (In reply to Joshua Hoffman from comment #8)
> > > > > Created attachment 469372 [details]
> > > > > Patch
> > > > 
> > > > diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> > > > b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> > > > index f6f0622abe93..c003ae7462a9 100644
> > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> > > > +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
> > > > @@ -160,9 +160,10 @@ Ref<AXIsolatedTree>
> > > > AXIsolatedTree::create(AXObjectCache& axObjectCache)
> > > > 
> > > >      tree->updateLoadingProgress(axObjectCache.loadingProgress());
> > > > 
> > > > -    tree->updateRelations(axObjectCache.relations());
> > > > +    const auto relations = axObjectCache.relations();
> > > > +    tree->updateRelations(relations);
> > > > 
> > > > -    for (auto& relatedObjectID : axObjectCache.relations().keys()) {
> > > > +    for (auto& relatedObjectID : relations.keys()) {
> > > >          RefPtr axObject = axObjectCache.objectForID(relatedObjectID);
> > > >          if (axObject && axObject->accessibilityIsIgnored())
> > > >              tree->addUnconnectedNode(axObject.releaseNonNull());
> > > > 
> > > > AG: I don't think we need this for loop at all since the unconnected iso
> > > > objects are added in AXObjectCache::addRelation when relations are updated.
> > > > I think this is another left over from a previous change that wasn't
> > > > necessary. But I may be missing something...
> > > 
> > > We need this loop in addition to the code in addRelations so that if the
> > > isolated tree is enabled/initialized after relations have been added, we
> > > still create unconnected objects for the existing relations on the tree.
> > 
> > Wouldn't it make more sense to dirty the relations in that scenario?
> 
> I worry that dirtying relations if they have already been created could add
> some time complexity issues, but definitely open to discussing that.
> 
> In terms of why adding the unconnected nodes here would be needed if ITM is
> already on, I added logging in both AXObjectCache::addRelation and
> AXIsolatedTree::createEmpty, and because of timing, we begin to add
> relations while the isolated tree is also being created, meaning we would
> miss out on some relations:
> 
> LOG: AXObjectCache::addRelation
> LOG: AXObjectCache::addRelation
> LOG: AXObjectCache::addRelation
> LOG: AXObjectCache::addRelation
> LOG: AXObjectCache::addRelation
> LOG: AXIsolatedTree::createEmpty
> LOG: AXIsolatedTree::create
> LOG: AXObjectCache::addRelation
> LOG: AXObjectCache::addRelation

Thanks for the additional confirmation.

[EWS]

Committed 272958@main (0c339289de37): <https://commits.webkit.org/272958@main>

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