Bug 240239 - AX ITM: Cache several object relationships that were not cached in isolated tree mode.
Summary: AX ITM: Cache several object relationships that were not cached in isolated t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-05-09 10:13 PDT by Andres Gonzalez
Modified: 2022-06-02 18:07 PDT (History)
12 users (show)

See Also:


Attachments
Patch (10.68 KB, patch)
2022-05-09 10:32 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (22.55 KB, patch)
2022-05-31 11:31 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (22.58 KB, patch)
2022-06-01 19:21 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2022-05-09 10:13:14 PDT
AX ITM: Cache several object relationships that were not cached in isolated tree mode.
Comment 1 Radar WebKit Bug Importer 2022-05-09 10:13:24 PDT
<rdar://problem/92970244>
Comment 2 Andres Gonzalez 2022-05-09 10:32:59 PDT
Created attachment 459056 [details]
Patch
Comment 3 Andres Gonzalez 2022-05-31 11:31:03 PDT
Created attachment 459893 [details]
Patch
Comment 4 Tyler Wilcock 2022-05-31 14:34:43 PDT
Comment on attachment 459893 [details]
Patch

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

Looks good to me!

> COMMIT_MESSAGE:3
> +

Your COMMIT_MESSAGE is missing a "Reviewed by" below the bug URL and above the long-form commit description.
Comment 5 Andres Gonzalez 2022-06-01 05:58:04 PDT
(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 459893 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459893&action=review
> 
> Looks good to me!
> 
> > COMMIT_MESSAGE:3
> > +
> 
> Your COMMIT_MESSAGE is missing a "Reviewed by" below the bug URL and above
> the long-form commit description.

I'm not getting the commit message template with the webkit-patch upload script. Does it work only if you have uncommitted changes on the main branch? I have my changes committed in a working branch.
Comment 6 Andres Gonzalez 2022-06-01 19:21:10 PDT
Created attachment 459952 [details]
Patch
Comment 7 Andres Gonzalez 2022-06-01 19:29:35 PDT
(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 459893 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459893&action=review
> 
> Looks good to me!
> 
> > COMMIT_MESSAGE:3
> > +
> 
> Your COMMIT_MESSAGE is missing a "Reviewed by" below the bug URL and above
> the long-form commit description.

Somewhat amended the commit message, but couldn't re-generate the list of changed functions. So I guess we'll go without that this time.
Comment 8 chris fleizach 2022-06-01 23:17:27 PDT
Comment on attachment 459952 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:627
> +                return cache->relations();

is this going to make a copy of the relations map? or just assign it the same pointer?
Comment 9 Andres Gonzalez 2022-06-02 05:44:48 PDT
(In reply to chris fleizach from comment #8)
> Comment on attachment 459952 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459952&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:627
> > +                return cache->relations();
> 
> is this going to make a copy of the relations map? or just assign it the
> same pointer?

It is a copy accessed only on the AX thread. I.e., we maintain two copies of the m_relations data structure, one in the AXObjectCache and one in the AXIsolatedTree. The one in the cache is only accessed on the main thread and the one on the IsolatedTree is accessed only on the AX thread. The atomic m_relationsNeedUpdate is used to mark that the m_relations in the IsolatedTree is dirty and needs to be copied from the cache's one. Since the copy happens by blocking the AX thread until we get the value from the main thread, I believe we are safe:

    if (m_relationsNeedUpdate) {
        m_relations = Accessibility::retrieveValueFromMainThread<HashMap<AXID, AXRelations>>([this] () -> HashMap<AXID, AXRelations> {
            if (auto* cache = axObjectCache())
                return cache->relations();
            return { };
        });
        m_relationsNeedUpdate = false;
    }
Comment 10 EWS 2022-06-02 12:46:56 PDT
Committed r295139 (251230@main): <https://commits.webkit.org/251230@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 459952 [details].
Comment 11 Ross Kirsling 2022-06-02 18:07:41 PDT
Landing build fix for !ENABLE(ACCESSIBILITY) in https://github.com/WebKit/WebKit/pull/1262.