Bug 257894

Summary:

AX: Make the generation and update of isolated subtrees asynchronous.

Product:

WebKit

Reporter:

Andres Gonzalez <andresg_22>

Component:

Accessibility

Assignee:

Andres Gonzalez <andresg_22>

Status:

NEW ---

Severity:

Normal

CC:

aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, tyler_w, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	andresg_22: review?

Description Andres Gonzalez 2023-06-09 09:14:41 PDT

This synchronous method contributes to Safari becoming irresponsive to AT requests.

Comment 1 Radar WebKit Bug Importer 2023-06-09 09:14:53 PDT

<rdar://problem/110532050>

Comment 2 Andres Gonzalez 2023-06-09 09:22:01 PDT

Created attachment 466651 [details]
Patch

Comment 3 Tyler Wilcock 2023-06-09 16:51:31 PDT

Comment on attachment 466651 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:902
> +    auto tree = AXIsolatedTree::create(*this, [this] () {

When capturing `this` in the completion handler, we should be storing it in a `WeakPtr` or `CheckedPtr` (or RefPtr, but currently you can't make a ref-counted AXObjectCache) to avoid a UAF if the cache gets deleted before the completion handler runs.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:362
> +    auto now = MonotonicTime::now();

I wonder if this should be called something like "start" or "startTime". It might make the comparison below more clear (which currently subtracts "MonotonicTome::now" from "now").

Comment 4 Andres Gonzalez 2023-06-12 07:52:23 PDT

Created attachment 466669 [details]
Patch

Comment 5 Andres Gonzalez 2023-06-12 07:56:56 PDT

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 466651 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466651&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:902
> > +    auto tree = AXIsolatedTree::create(*this, [this] () {
> 
> When capturing `this` in the completion handler, we should be storing it in
> a `WeakPtr` or `CheckedPtr` (or RefPtr, but currently you can't make a
> ref-counted AXObjectCache) to avoid a UAF if the cache gets deleted before
> the completion handler runs.

Done.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:362
> > +    auto now = MonotonicTime::now();
> 
> I wonder if this should be called something like "start" or "startTime". It
> might make the comparison below more clear (which currently subtracts
> "MonotonicTome::now" from "now").

Renamed.

Also added in the new revision the check for Accessibility::waitingForMainThread.

Comment 6 Andres Gonzalez 2023-10-18 08:30:08 PDT

Created attachment 468261 [details]
Patch

Comment 7 Andres Gonzalez 2023-10-18 12:21:59 PDT

Created attachment 468262 [details]
Patch

Updating expectation for glib test.