Bug 264045

Summary: AX: Fix thread safety issue in AXIsolatedTree::setSelectedTextMarkerRange
Product: WebKit Reporter: Joshua Hoffman <jhoffman23>
Component: AccessibilityAssignee: Joshua Hoffman <jhoffman23>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch none

Description Joshua Hoffman 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).
Comment 1 Radar WebKit Bug Importer 2023-11-01 10:26:19 PDT 
<rdar://problem/117803232>
Comment 2 Joshua Hoffman 2023-11-01 10:34:01 PDT 
Created attachment 468448 [details]
Patch
Comment 3 Andres Gonzalez 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;
Comment 4 Joshua Hoffman 2023-11-01 23:01:01 PDT 
Created attachment 468452 [details]
Patch
Comment 5 EWS 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].