Bug 256207 – AX: Avoid hitting the main thread to retrieve position and size of WKAccessibilityWebPageObjects in isolated tree mode.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

This is one of the factors contributing to sluggishness during page load.

[Radar WebKit Bug Importer]

<rdar://problem/108787870>

[Andres Gonzalez]

Created attachment 466172 [details]
Patch

[Tyler Wilcock]

Comment on attachment 466172 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.h:45
> +    NakedPtr<WebCore::AXCoreObject> m_isolatedTreeRoot;

Does NakedPtr prevent UAF? If not, maybe this should be CheckedPtr?

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.h:46
> +    Lock m_lock;

Is there a more descriptive name for this lock than m_lock?

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:234
> +    });

Why do we no longer need autorelease here?

[Andres Gonzalez]

Created attachment 466179 [details]
Patch

[Andres Gonzalez]

Created attachment 466180 [details]
Patch

[Chris Dumez]

Comment on attachment 466180 [details]
Patch

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

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1904
> +    if (auto* webPage = m_frame->page())

Can we add an ASSERT(isMainRunLoop()) here?

The code wouldn't be safe if it ran off the main thread since this this "AXIsolatedTree" feature seems threading-related, probably a good thing to check.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.h:44
> +    WebCore::IntSize m_size;

Would be nice to use our threading macros to validate which data members are protected by the lock: `WTF_GUARDED_BY_LOCK(m_lock)`. Not sure if it will work in an ObjC header though.

[Andres Gonzalez]

Created attachment 466181 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 466172 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466172&action=review
> 
> > Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.h:45
> > +    NakedPtr<WebCore::AXCoreObject> m_isolatedTreeRoot;
> 
> Does NakedPtr prevent UAF? If not, maybe this should be CheckedPtr?

AX objects pointers cannot be held in CheckedPtr as is. All the platform wrappers are keeping raw or naked pointers at the moment, so I would take the conversion of that as a separate task/patch.
> 
> > Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.h:46
> > +    Lock m_lock;
> 
> Is there a more descriptive name for this lock than m_lock?

Renamed it to m_cacheLock. Let me know if you are thinking of a better name.
> 
> > Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:234
> > +    });
> 
> Why do we no longer need autorelease here?

Because retrieveAutoreleasedValueFromMainThread.

Thanks.

[Andres Gonzalez]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 466180 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466180&action=review
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1904
> > +    if (auto* webPage = m_frame->page())
> 
> Can we add an ASSERT(isMainRunLoop()) here?

Done.
> 
> The code wouldn't be safe if it ran off the main thread since this this
> "AXIsolatedTree" feature seems threading-related, probably a good thing to
> check.
> 
> > Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.h:44
> > +    WebCore::IntSize m_size;
> 
> Would be nice to use our threading macros to validate which data members are
> protected by the lock: `WTF_GUARDED_BY_LOCK(m_lock)`. Not sure if it will
> work in an ObjC header though.

Done. At least it compiles if the Lock is declared above the guarded members. Don't know if it would actually trigger compilation errors if trying to use the variables without holding the Lock like in C++ classes.

Thanks!

[Tyler Wilcock]

Comment on attachment 466181 [details]
Patch

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

> Source/WebCore/loader/EmptyFrameLoaderClient.h:183
> +    void setAXIsolatedTreeRoot(WebCore::AXCoreObject*) final;

This should always be an AXIsolatedObject*, right? Typing this as AXCoreObject implicitly means "this could be an AccessibilityObject or an AXIsolatedObject", which seems wrong — the AXIsolatedTreeRoot should never be an AccessibilityObject.

[EWS]

Committed 263618@main (7b312e8e9f18): <https://commits.webkit.org/263618@main>

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

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #10)
> Comment on attachment 466181 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466181&action=review
> 
> > Source/WebCore/loader/EmptyFrameLoaderClient.h:183
> > +    void setAXIsolatedTreeRoot(WebCore::AXCoreObject*) final;
> 
> This should always be an AXIsolatedObject*, right? Typing this as
> AXCoreObject implicitly means "this could be an AccessibilityObject or an
> AXIsolatedObject", which seems wrong — the AXIsolatedTreeRoot should never
> be an AccessibilityObject.

No, code outside core accessibility should not have to know anything about isolated objects versus live objects. That's one of the purposes of the common AXCoreObject interface. It would be wrong for non-AX code to have to know that an object is isolated. This is not absolute of course, there may be cases where it may be necessary, but in general using the AXCoreObject abstraction outside core AX code is what we should be shooting for. Thanks.