Bug 264045

Summary:

AX: Fix thread safety issue in AXIsolatedTree::setSelectedTextMarkerRange

Product:

WebKit

Reporter:

Joshua Hoffman <jhoffman23>

Component:

Accessibility

Assignee:

Joshua Hoffman <jhoffman23>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

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

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none

Joshua Hoffman

Reported 2023-11-01 10:26:09 PDT

We need to hold a lock when modifying m_selectedTextMarkerRange on the main thread (modified on the main thread in AXIsolatedTree::setSelectedTextMarkerRange).

Attachments
Patch (2.75 KB, patch) 2023-11-01 10:34 PDT, Joshua Hoffman	no flags	Details Formatted Diff Diff
Patch (3.07 KB, patch) 2023-11-01 23:01 PDT, Joshua Hoffman	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2023-11-01 10:26:19 PDT

<rdar://problem/117803232>

Joshua Hoffman

Comment 2 2023-11-01 10:34:01 PDT

Created attachment 468448 [details] Patch

Andres Gonzalez

Comment 3 2023-11-01 19:19:57 PDT

(In reply to Joshua Hoffman from comment #2) > Created attachment 468448 [details] > Patch diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp index 057f683c009d..e2d551382ff4 100644 --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp @@ -910,6 +910,24 @@ void AXIsolatedTree::setFocusedNodeID(AXID axID) m_pendingFocusedNodeID = axID; } +AXTextMarkerRange AXIsolatedTree::selectedTextMarkerRange() +{ + AXTRACE("AXIsolatedTree::selectedTextMarkerRange"_s); + Locker locker { m_changeLogLock }; + return m_selectedTextMarkerRange; +} + AG: extra blank line. + +void AXIsolatedTree::setSelectedTextMarkerRange(AXTextMarkerRange range) +{ + AXTRACE("AXIsolatedTree::setSelectedTextMarkerRange"_s); + ASSERT(isMainThread()); + + Locker locker { m_changeLogLock }; + m_selectedTextMarkerRange = range; +} + AG: extra blank line. + void AXIsolatedTree::labelCreated(AccessibilityObject& axObject) { ASSERT(axObject.isLabel()); diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h index ebc80fa48083..78b51ca89317 100644 --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h +++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h @@ -321,8 +321,8 @@ public: // Use only if the s_storeLock is already held like in findAXTree. WEBCORE_EXPORT OptionSet<ActivityState> lockedPageActivityState() const; - AXTextMarkerRange selectedTextMarkerRange() { return m_selectedTextMarkerRange; }; - void setSelectedTextMarkerRange(AXTextMarkerRange range) { m_selectedTextMarkerRange = range; } + AXTextMarkerRange selectedTextMarkerRange(); + void setSelectedTextMarkerRange(AXTextMarkerRange); AG: we should pass AXTextMarkerRange&& instead of by value. private: AXIsolatedTree(AXObjectCache&); AG: Add the macro WTF_GUARDED_BY_LOCK to the declaration: AXTextMarkerRange m_selectedTextMarkerRange;

Joshua Hoffman

Comment 4 2023-11-01 23:01:01 PDT

Created attachment 468452 [details] Patch

EWS

Comment 5 2023-11-02 13:01:31 PDT

Committed 270134@main (3e8a39cebd79): <https://commits.webkit.org/270134@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 468452 [details].

Note You need to log in before you can comment on or make changes to this bug.