Bug 251688 – AX: Avoid hitting the main thread on "AXTextMarkerIsValid" calls.

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

[Andres Gonzalez]

This call is hitting the main thread. Avoid using the new AXTextMarker class.

[Radar WebKit Bug Importer]

<rdar://problem/105005458>

[Andres Gonzalez]

Created attachment 464824 [details]
Patch

[Tyler Wilcock]

Comment on attachment 464824 [details]
Patch

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

> Source/WebCore/accessibility/AXTextMarker.cpp:28
> +#include "AXLogger.h"

What requires the addition of this include?

> Source/WebCore/accessibility/AXTextMarker.cpp:112
> +    auto cache = AXTreeStore<AXObjectCache>::axObjectCacheForID(treeID());

Thoughts on using WeakPtr rather than auto here in terms of style? I like specifying WeakPtr, RefPtr, CheckedPtr, etc. over auto since these smart pointers have important semantics that are good to see at a glance. A lot of WebCore follows this pattern, too.

> LayoutTests/accessibility/mac/textmarker-for-index-out-of-bounds-crash.html:14
> +    output += expect("text.isTextMarkerNull(text.textMarkerForIndex(99999999999))", "false");

Can you help me understand why we need to add a new AccessibilityUIElement::isTextMarkerNull() method instead of using the existing AccessibilityUIElement::isTextMarkerValid()? Given these implementations:

AXTextMarker.h

bool isNull() const { return !treeID().isValid() || !objectID().isValid(); }
bool isValid() const { return object(); }

Isn't AcessibilityUIElement::isTextMarkerValid() + AXTextMarker::isValid() all that is necessary? Since we wouldn't be able to get a non-null object() if:

  1. The tree ID is invalid
  2. The object ID is invalid
  3. Both are valid, but the underlying object was deleted and removed from the cache / isolated tree.

[Andres Gonzalez]

Created attachment 464828 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 464824 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=464824&action=review
> 
> > Source/WebCore/accessibility/AXTextMarker.cpp:28
> > +#include "AXLogger.h"
> 
> What requires the addition of this include?

Every time you want to log something you have to add it again. I could leave some AXLOG to justify it but thought you would be a bit indulgent to me and let me keep it there. :-)
> 
> > Source/WebCore/accessibility/AXTextMarker.cpp:112
> > +    auto cache = AXTreeStore<AXObjectCache>::axObjectCacheForID(treeID());
> 
> Thoughts on using WeakPtr rather than auto here in terms of style? I like
> specifying WeakPtr, RefPtr, CheckedPtr, etc. over auto since these smart
> pointers have important semantics that are good to see at a glance. A lot of
> WebCore follows this pattern, too.


Agree, changed it, I've felt many times we abuse auto.
> 
> > LayoutTests/accessibility/mac/textmarker-for-index-out-of-bounds-crash.html:14
> > +    output += expect("text.isTextMarkerNull(text.textMarkerForIndex(99999999999))", "false");
> 
> Can you help me understand why we need to add a new
> AccessibilityUIElement::isTextMarkerNull() method instead of using the
> existing AccessibilityUIElement::isTextMarkerValid()? Given these
> implementations:
> 
> AXTextMarker.h
> 
> bool isNull() const { return !treeID().isValid() || !objectID().isValid(); }
> bool isValid() const { return object(); }
> 
> Isn't AcessibilityUIElement::isTextMarkerValid() + AXTextMarker::isValid()
> all that is necessary? Since we wouldn't be able to get a non-null object()
> if:
> 
>   1. The tree ID is invalid
>   2. The object ID is invalid
>   3. Both are valid, but the underlying object was deleted and removed from
> the cache / isolated tree.

!isNull is a weaker condition than isValid. The previous isValid was actually equivalent to !isNull. This test passes with !isNull but don't pass with the new isValid. I had to make the distinction in order to get this test passing.

[Tyler Wilcock]

> Agree, changed it, I've felt many times we abuse auto.
I think there are some other places where this patch uses auto instead of WeakPtr / RefPtr (e.g. all the calls to AXIsolatedTree::objectForID).

[Andres Gonzalez]

Created attachment 464863 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #6)
> > Agree, changed it, I've felt many times we abuse auto.
> I think there are some other places where this patch uses auto instead of
> WeakPtr / RefPtr (e.g. all the calls to AXIsolatedTree::objectForID).

I think I got all those now.

[Andres Gonzalez]

Created attachment 464865 [details]
Patch

[Andres Gonzalez]

Created attachment 464890 [details]
Patch

[EWS]

Commit message contains (OOPS!) and no reviewer found, blocking PR #None

[Andres Gonzalez]

Created attachment 464922 [details]
Patch

[EWS]

Committed 260069@main (caa465314d87): <https://commits.webkit.org/260069@main>

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