Bug 249480 – AX: Add AXID identification to both AXObjectCache and AXIsolatedTree so that instances of these classes can be retrieved by ID.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andres Gonzalez]

Needed for TextMarker refactoring in https://bugs.webkit.org/show_bug.cgi?id=230558.

[Radar WebKit Bug Importer]

<rdar://problem/103449294>

[Andres Gonzalez]

Created attachment 464077 [details]
Patch

[Tyler Wilcock]

Comment on attachment 464077 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:270
> +    AXTRACE(makeString("AXObjectCache::~AXObjectCache 0x"_s, hex(reinterpret_cast<uintptr_t>(this))));

With this patch, we now rely on some outside logic to call AXObjectCache::willBeDestroyed(), which seems fallible. The lifetime of the cache is pretty simple right now (it dies when the document does), but there's nothing stopping it from becoming harder to reason about. Missing the call to AXObjectCache::willBeDestroyed() would leak a lot of memory.

If we must move this destruction logic out of the destructor, can we somehow make it impossible for the cache to not be cleaned up? Maybe a flag indicating willBeDestroyed has been called, and then the destructor checks that flag and performs willBeDestroyed if necessary?

> Source/WebCore/accessibility/AXObjectStore.h:35
> +class AXObjectStore {

This name confuses me, but that may be a product of me not having wrapped my mind around it yet. My initial reaction based on the name is that it's an abstraction over adding + removing objects by ID, but that's not what it is. I can't think of a better one right now, so if you disagree the name is confusing, or if you also can't think of a better one, then it's fine.

> Source/WebCore/accessibility/AXObjectStore.h:95
> +    } while (!axID.isValid() || map().contains(axID));

Is it possible for ObjectIdentifier::generate() to create an invalid ID? If not, this !axID.isValid() doesn't seem needed.

[Andres Gonzalez]

Created attachment 464079 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 464077 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=464077&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:270
> > +    AXTRACE(makeString("AXObjectCache::~AXObjectCache 0x"_s, hex(reinterpret_cast<uintptr_t>(this))));
> 
> With this patch, we now rely on some outside logic to call
> AXObjectCache::willBeDestroyed(), which seems fallible. The lifetime of the
> cache is pretty simple right now (it dies when the document does), but
> there's nothing stopping it from becoming harder to reason about. Missing
> the call to AXObjectCache::willBeDestroyed() would leak a lot of memory.
> 
> If we must move this destruction logic out of the destructor, can we somehow
> make it impossible for the cache to not be cleaned up? Maybe a flag
> indicating willBeDestroyed has been called, and then the destructor checks
> that flag and performs willBeDestroyed if necessary?

This issue is no longer since the map is now <AXID, WeakPtr>, so it is fine to continue cleaning up in the destructor as before. However, we should revisit the construction/destruction of AXObjectCache since it doesn't follow the WebKit pattern of a create static method with private constructor. Also all the cleanup code in the destructor should probably moved to a cleanup method that can be invoked from the destructor.

> 
> > Source/WebCore/accessibility/AXObjectStore.h:35
> > +class AXObjectStore {
> 
> This name confuses me, but that may be a product of me not having wrapped my
> mind around it yet. My initial reaction based on the name is that it's an
> abstraction over adding + removing objects by ID, but that's not what it is.
> I can't think of a better one right now, so if you disagree the name is
> confusing, or if you also can't think of a better one, then it's fine.

The abstraction is exactly what you think it was, but since in this context AXObject has a very specific meaning, it makes it confusing. An alternative could be AXTreeStore. We'll give it some more thought and see if we come up with a better name than that.

> 
> > Source/WebCore/accessibility/AXObjectStore.h:95
> > +    } while (!axID.isValid() || map().contains(axID));
> 
> Is it possible for ObjectIdentifier::generate() to create an invalid ID? If
> not, this !axID.isValid() doesn't seem needed.

Yes, it can return 0 which is not a valid identifier, so we need to keep it.

[Tyler Wilcock]

> > > Source/WebCore/accessibility/AXObjectStore.h:95
> > > +    } while (!axID.isValid() || map().contains(axID));
> > 
> > Is it possible for ObjectIdentifier::generate() to create an invalid ID? If
> > not, this !axID.isValid() doesn't seem needed.
> 
> Yes, it can return 0 which is not a valid identifier, so we need to keep it.

static ObjectIdentifier generate()
{
    return ObjectIdentifier { generateIdentifierInternal() };
}

uint64_t ObjectIdentifierBase::generateIdentifierInternal()
{
    static uint64_t current;
    return ++current;
}

This will always be 1 or greater, won't it? Or am I missing something?

[Andres Gonzalez]

Created attachment 464089 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #6)
> > > > Source/WebCore/accessibility/AXObjectStore.h:95
> > > > +    } while (!axID.isValid() || map().contains(axID));
> > > 
> > > Is it possible for ObjectIdentifier::generate() to create an invalid ID? If
> > > not, this !axID.isValid() doesn't seem needed.
> > 
> > Yes, it can return 0 which is not a valid identifier, so we need to keep it.
> 
> static ObjectIdentifier generate()
> {
>     return ObjectIdentifier { generateIdentifierInternal() };
> }
> 
> uint64_t ObjectIdentifierBase::generateIdentifierInternal()
> {
>     static uint64_t current;
>     return ++current;
> }
> 
> This will always be 1 or greater, won't it? Or am I missing something?

    uint64_t i = 0xffffffffffffffff;
    WTFLogAlways("%llu %llu", i, i + 1);

Output:
18446744073709551615 0

[Andres Gonzalez]

Created attachment 464327 [details]
Patch

Fix for win and gtk builds.

[chris fleizach]

Comment on attachment 464327 [details]
Patch

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

> COMMIT_MESSAGE:7
> +Added a base template class named AXObjectStore to both AXObjectCache and AXIsolatedTree that implements a map-based storage for instances. The keys in the map are the objects IDs which are assigned on construction. This is necessary to retrieve an instance based on its ID. Retrieving an AXObjectCache or AXIsolatedTree instance is needed for the TextMarker refactoring in https://bugs.webkit.org/show_bug.cgi?id=230558.

"AXObjectStore to both AXObjectCache" -> "AXObjectStore for both AXObjectCache"

> Source/WebCore/accessibility/AXTreeStore.h:47
> +    static HashMap<AXID, WeakPtr<T>>& map() WTF_REQUIRES_LOCK(s_storeLock);

do we need to expose map() as protected? or can it be private? I see it requires the lock usage which is good, but also seems like it might be used incorrectly.
we also have all the methods we need to interact with it

[Andres Gonzalez]

Created attachment 464337 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #10)
> Comment on attachment 464327 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=464327&action=review
> 
> > COMMIT_MESSAGE:7
> > +Added a base template class named AXObjectStore to both AXObjectCache and AXIsolatedTree that implements a map-based storage for instances. The keys in the map are the objects IDs which are assigned on construction. This is necessary to retrieve an instance based on its ID. Retrieving an AXObjectCache or AXIsolatedTree instance is needed for the TextMarker refactoring in https://bugs.webkit.org/show_bug.cgi?id=230558.
> 
> "AXObjectStore to both AXObjectCache" -> "AXObjectStore for both
> AXObjectCache"

Fixed.
> 
> > Source/WebCore/accessibility/AXTreeStore.h:47
> > +    static HashMap<AXID, WeakPtr<T>>& map() WTF_REQUIRES_LOCK(s_storeLock);
> 
> do we need to expose map() as protected? or can it be private? I see it
> requires the lock usage which is good, but also seems like it might be used
> incorrectly.
> we also have all the methods we need to interact with it

Fixed.

[EWS]

Committed 258480@main (da43d7445735): <https://commits.webkit.org/258480@main>

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