REGRESSION (274876@main): [ iOS Debug ] accessibility/text-marker/tex…

…t-marker-range-stale-node-crash.html is a consistent crash

https://bugs.webkit.org/show_bug.cgi?id=274020
rdar://127901543

Reviewed by Darin Adler.

Introduce SafeTextMarkerData as an alternative to TextMarkerData, which uses
a WeakPtr for `node` instead of a raw pointer. TextMarkerData currently has
to keep using a raw pointer because memcpy() / memcmp() are used with this
type's bytes. However, we should use SafeTextMarkerData wherever possible
and this patch starts doing some adoption to address the crash.

* LayoutTests/platform/ios/TestExpectations:
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::characterOffsetForTextMarkerData):
(WebCore::AXObjectCache::accessibilityObjectForTextMarkerData):
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/AXTextMarker.h:
(WebCore::SafeTextMarkerData::toTextMarkerData const):
(WebCore::TextMarkerData::toSafeTextMarkerData const):
* Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:
(-[WebAccessibilityTextMarker initWithTextMarker:cache:]):
(-[WebAccessibilityTextMarker initWithData:cache:]):
(-[WebAccessibilityTextMarker dataRepresentation]):
(-[WebAccessibilityTextMarker visiblePosition]):
(-[WebAccessibilityTextMarker characterOffset]):
(-[WebAccessibilityTextMarker isIgnored]):
(-[WebAccessibilityTextMarker accessibilityObject]):
(-[WebAccessibilityTextMarker description]):
(-[WebAccessibilityTextMarker textMarkerData]):

Canonical link: https://commits.webkit.org/278655@main

LayoutTests/platform/ios/TestExpectations

@@ -7298,5 +7298,3 @@ webkit.org/b/273848 [ Release ] imported/w3c/web-platform-tests/html/semantics/e
 webkit.org/b/273905 svg/filters/filter-on-root-tile-boundary.html [ Pass ImageOnlyFailure ]
 
 webkit.org/b/273984 http/tests/site-isolation/window-open-with-name-cross-site.html [ Timeout ]
-
-webkit.org/b/274020 [ Debug ] accessibility/text-marker/text-marker-range-stale-node-crash.html [ Skip ]

Source/WebCore/accessibility/AXObjectCache.cpp

@@ -2814,7 +2814,8 @@ Ref<Document> AXObjectCache::protectedDocument() const
     return document();
 }
 
-VisiblePosition AXObjectCache::visiblePositionForTextMarkerData(const TextMarkerData& textMarkerData)
+template<typename TextMarkerDataType>
+VisiblePosition AXObjectCache::visiblePositionForTextMarkerData(const TextMarkerDataType& textMarkerData)
 {
     Ref node = *textMarkerData.node;
     if (!isNodeInUse(node) || node->isPseudoElement())
@@ -2836,16 +2837,19 @@ VisiblePosition AXObjectCache::visiblePositionForTextMarkerData(const TextMarker
     return visiblePosition;
 }
 
-CharacterOffset AXObjectCache::characterOffsetForTextMarkerData(TextMarkerData& textMarkerData)
+template VisiblePosition AXObjectCache::visiblePositionForTextMarkerData(const TextMarkerData&);
+template VisiblePosition AXObjectCache::visiblePositionForTextMarkerData(const SafeTextMarkerData&);
+
+CharacterOffset AXObjectCache::characterOffsetForTextMarkerData(const SafeTextMarkerData& textMarkerData)
 {
-    if (textMarkerData.ignored)
+    if (textMarkerData.ignored || !textMarkerData.node)
         return { };
 
-    RefPtrAllowingPartiallyDestroyed<Node> node = textMarkerData.node;
-    if (!node || !isNodeInUse(*node))
+    RefAllowingPartiallyDestroyed<Node> node = *textMarkerData.node;
+    if (!isNodeInUse(node))
         return { };
 
-    CharacterOffset result(node.get(), textMarkerData.characterStart, textMarkerData.characterOffset);
+    CharacterOffset result(node.ptr(), textMarkerData.characterStart, textMarkerData.characterOffset);
     // When we are at a line wrap and the VisiblePosition is upstream, it means the text marker is at the end of the previous line.
     // We use the previous CharacterOffset so that it will match the Range.
     if (textMarkerData.affinity == Affinity::Upstream)
@@ -3377,10 +3381,13 @@ CharacterOffset AXObjectCache::characterOffsetFromVisiblePosition(const VisibleP
     return result;
 }
 
-AccessibilityObject* AXObjectCache::accessibilityObjectForTextMarkerData(TextMarkerData& textMarkerData)
+AccessibilityObject* AXObjectCache::accessibilityObjectForTextMarkerData(const SafeTextMarkerData& textMarkerData)
 {
-    RefPtr domNode = textMarkerData.node;
-    if (!isNodeInUse(*domNode))
+    if (textMarkerData.ignored)
+        return nullptr;
+
+    RefPtr domNode = textMarkerData.node.get();
+    if (!domNode || !isNodeInUse(*domNode))
         return nullptr;
 
     return getOrCreate(*domNode);

Source/WebCore/accessibility/AXObjectCache.h

@@ -450,14 +450,14 @@ class AXObjectCache final : public CanMakeWeakPtr<AXObjectCache>, public CanMake
     AXTextMarker nextTextMarker(const AXTextMarker&);
     TextMarkerData textMarkerDataForPreviousCharacterOffset(const CharacterOffset&);
     AXTextMarker previousTextMarker(const AXTextMarker&);
-    VisiblePosition visiblePositionForTextMarkerData(const TextMarkerData&);
-    CharacterOffset characterOffsetForTextMarkerData(TextMarkerData&);
+    template<typename TextMarkerDataType> VisiblePosition visiblePositionForTextMarkerData(const TextMarkerDataType&);
+    CharacterOffset characterOffsetForTextMarkerData(const SafeTextMarkerData&);
     // Use ignoreNextNodeStart/ignorePreviousNodeEnd to determine the behavior when we are at node boundary.
     CharacterOffset nextCharacterOffset(const CharacterOffset&, bool ignoreNextNodeStart = true);
     CharacterOffset previousCharacterOffset(const CharacterOffset&, bool ignorePreviousNodeEnd = true);
     TextMarkerData startOrEndTextMarkerDataForRange(const SimpleRange&, bool);
     CharacterOffset startOrEndCharacterOffsetForRange(const SimpleRange&, bool, bool enterTextControls = false);
-    AccessibilityObject* accessibilityObjectForTextMarkerData(TextMarkerData&);
+    AccessibilityObject* accessibilityObjectForTextMarkerData(const SafeTextMarkerData&);
     std::optional<SimpleRange> rangeForUnorderedCharacterOffsets(const CharacterOffset&, const CharacterOffset&);
     static SimpleRange rangeForNodeContents(Node&);
     static unsigned lengthForRange(const SimpleRange&);

Source/WebCore/accessibility/AXTextMarker.h

@@ -48,11 +48,13 @@ enum class LineRangeType : uint8_t {
     Right,
 };
 
+struct SafeTextMarkerData;
+
 struct TextMarkerData {
     unsigned treeID;
     unsigned objectID;
 
-    Node* node; // FIXME: This should use a smart pointer.
+    Node* node;
     unsigned offset;
     Position::AnchorType anchorType;
     Affinity affinity;
@@ -99,10 +101,43 @@ struct TextMarkerData {
     {
         return ObjectIdentifier<AXIDType>(objectID);
     }
+
+    SafeTextMarkerData toSafeTextMarkerData() const;
+
 private:
     void initializeAXIDs(AXObjectCache&, Node*);
 };
 
+// Safer version of TextMarkerData with a WeakPtr for Node.
+// TextMarkerData uses a raw pointer for Node because it is
+// used with memset / memcmp.
+struct SafeTextMarkerData {
+    AXID treeID;
+    AXID objectID;
+
+    WeakPtr<Node, WeakPtrImplWithEventTargetData> node;
+    unsigned offset { 0 };
+    Position::AnchorType anchorType { Position::AnchorType::PositionIsOffsetInAnchor };
+    Affinity affinity { Affinity::Upstream };
+
+    unsigned characterStart { 0 };
+    unsigned characterOffset { 0 };
+    bool ignored { false };
+
+    TextMarkerData toTextMarkerData() const;
+    AXID axObjectID() const { return objectID; }
+};
+
+inline TextMarkerData SafeTextMarkerData::toTextMarkerData() const
+{
+    return { treeID, objectID, node.get(), offset, anchorType, affinity, characterStart, characterOffset, ignored };
+}
+
+inline SafeTextMarkerData TextMarkerData::toSafeTextMarkerData() const
+{
+    return { axTreeID(), axObjectID(), node, offset, anchorType, affinity, characterStart, characterOffset, ignored };
+}
+
 #if PLATFORM(MAC)
 using PlatformTextMarkerData = AXTextMarkerRef;
 #elif PLATFORM(IOS_FAMILY)
@@ -210,7 +245,7 @@ class AXTextMarker {
     bool atLineEnd() const { return atLineBoundaryForDirection(AXDirection::Next); }
 #endif // ENABLE(AX_THREAD_TEXT_APIS)
 
-    TextMarkerData m_data;
+    TextMarkerData m_data; // FIXME: This should be a SafeTextMarkerData.
 };
 
 class AXTextMarkerRange {

Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm

@@ -126,7 +126,7 @@ typedef NS_ENUM(NSInteger, UIAccessibilityScrollDirection) {
 @interface WebAccessibilityTextMarker : NSObject
 {
     AXObjectCache* _cache;
-    TextMarkerData _textMarkerData;
+    SafeTextMarkerData _safeTextMarkerData;
 }
 
 + (WebAccessibilityTextMarker *)textMarkerWithVisiblePosition:(VisiblePosition&)visiblePos cache:(AXObjectCache*)cache;
@@ -144,7 +144,7 @@ - (id)initWithTextMarker:(TextMarkerData *)data cache:(AXObjectCache*)cache
         return nil;
 
     _cache = cache;
-    memcpy(&_textMarkerData, data, sizeof(TextMarkerData));
+    _safeTextMarkerData = data->toSafeTextMarkerData();
     return self;
 }
 
@@ -154,7 +154,9 @@ - (id)initWithData:(NSData *)data cache:(AXObjectCache*)cache
         return nil;
 
     _cache = cache;
-    [data getBytes:&_textMarkerData length:sizeof(TextMarkerData)];
+    TextMarkerData textMarkerData;
+    [data getBytes:&textMarkerData length:sizeof(TextMarkerData)];
+    _safeTextMarkerData = textMarkerData.toSafeTextMarkerData();
 
     return self;
 }
@@ -201,39 +203,38 @@ + (WebAccessibilityTextMarker *)startOrEndTextMarkerForRange:(const std::optiona
 
 - (NSData *)dataRepresentation
 {
-    return [NSData dataWithBytes:&_textMarkerData length:sizeof(TextMarkerData)];
+    auto textMarkerData = _safeTextMarkerData.toTextMarkerData();
+    return [NSData dataWithBytes:&textMarkerData length:sizeof(TextMarkerData)];
 }
 
 - (VisiblePosition)visiblePosition
 {
-    return _cache->visiblePositionForTextMarkerData(_textMarkerData);
+    return _cache->visiblePositionForTextMarkerData(_safeTextMarkerData);
 }
 
 - (CharacterOffset)characterOffset
 {
-    return _cache->characterOffsetForTextMarkerData(_textMarkerData);
+    return _cache->characterOffsetForTextMarkerData(_safeTextMarkerData);
 }
 
 - (BOOL)isIgnored
 {
-    return _textMarkerData.ignored;
+    return _safeTextMarkerData.ignored;
 }
 
 - (AccessibilityObject*)accessibilityObject
 {
-    if (_textMarkerData.ignored)
-        return nullptr;
-    return _cache->accessibilityObjectForTextMarkerData(_textMarkerData);
+    return _cache->accessibilityObjectForTextMarkerData(_safeTextMarkerData);
 }
 
 - (NSString *)description
 {
-    return [NSString stringWithFormat:@"[AXTextMarker %p] = node: %p offset: %d", self, _textMarkerData.node, _textMarkerData.offset];
+    return [NSString stringWithFormat:@"[AXTextMarker %p] = node: %p offset: %d", self, _safeTextMarkerData.node.get(), _safeTextMarkerData.offset];
 }
 
 - (TextMarkerData)textMarkerData
 {
-    return _textMarkerData;
+    return _safeTextMarkerData.toTextMarkerData();
 }
 
 @end

Source/WebCore/accessibility/mac/AXObjectCacheMac.mm

@@ -847,22 +847,22 @@ static bool isTestAXClientType(AXClientType client)
     return adoptCF(AXTextMarkerRangeCopyEndMarker(textMarkerRange));
 }
 
-static TextMarkerData getBytesFromAXTextMarker(AXTextMarkerRef textMarker)
+static SafeTextMarkerData getBytesFromAXTextMarker(AXTextMarkerRef textMarker)
 {
-    TextMarkerData data;
     if (!textMarker)
-        return data;
+        return { };
 
     ASSERT(CFGetTypeID(textMarker) == AXTextMarkerGetTypeID());
     if (CFGetTypeID(textMarker) != AXTextMarkerGetTypeID())
-        return data;
+        return { };
 
-    ASSERT(AXTextMarkerGetLength(textMarker) == sizeof(data));
-    if (AXTextMarkerGetLength(textMarker) != sizeof(data))
-        return data;
+    ASSERT(AXTextMarkerGetLength(textMarker) == sizeof(TextMarkerData));
+    if (AXTextMarkerGetLength(textMarker) != sizeof(TextMarkerData))
+        return { };
 
-    memcpy(&data, AXTextMarkerGetBytePtr(textMarker), sizeof(data));
-    return data;
+    TextMarkerData data;
+    memcpy(&data, AXTextMarkerGetBytePtr(textMarker), sizeof(TextMarkerData));
+    return data.toSafeTextMarkerData();
 }
 
 AccessibilityObject* accessibilityObjectForTextMarker(AXObjectCache* cache, AXTextMarkerRef textMarker)
@@ -871,11 +871,8 @@ static TextMarkerData getBytesFromAXTextMarker(AXTextMarkerRef textMarker)
     if (!cache || !textMarker)
         return nullptr;
 
-    auto textMarkerData = getBytesFromAXTextMarker(textMarker);
-    if (textMarkerData.ignored)
-        return nullptr;
-
-    return cache->accessibilityObjectForTextMarkerData(textMarkerData);
+    auto safeTextMarkerData = getBytesFromAXTextMarker(textMarker);
+    return cache->accessibilityObjectForTextMarkerData(safeTextMarkerData);
 }
 
 // TextMarker <-> VisiblePosition conversion.
@@ -899,8 +896,8 @@ VisiblePosition visiblePositionForTextMarker(AXObjectCache* cache, AXTextMarkerR
     if (!cache || !textMarker)
         return { };
 
-    auto textMarkerData = getBytesFromAXTextMarker(textMarker);
-    return cache->visiblePositionForTextMarkerData(textMarkerData);
+    auto safeTextMarkerData = getBytesFromAXTextMarker(textMarker);
+    return cache->visiblePositionForTextMarkerData(safeTextMarkerData);
 }
 
 // TextMarkerRange <-> VisiblePositionRange conversion.
@@ -947,8 +944,8 @@ CharacterOffset characterOffsetForTextMarker(AXObjectCache* cache, AXTextMarkerR
     if (!cache || !textMarker)
         return { };
 
-    auto textMarkerData = getBytesFromAXTextMarker(textMarker);
-    return cache->characterOffsetForTextMarkerData(textMarkerData);
+    auto safeTextMarkerData = getBytesFromAXTextMarker(textMarker);
+    return cache->characterOffsetForTextMarkerData(safeTextMarkerData);
 }
 
 // TextMarkerRange <-> SimpleRange conversion.