Skip to content

Commit

Permalink
Adopt SafeTextMarkerData in more places
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=274035
rdar://127935353

Reviewed by Darin Adler.

Rename TextMarkerData to RawTextMarkerData since it contains
raw pointers. Rename SafeTextMarkerData to TextMarkerData and
adopt as much as possible in the code base.

RawTextMarkerData is now only used to convert to and from
bytes.

* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::visiblePositionForTextMarkerData):
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/AXTextMarker.cpp:
(WebCore::AXTextMarker::AXTextMarker):
(WebCore::AXTextMarker::setNodeIfNeeded const):
(WebCore::AXTextMarker::operator CharacterOffset const):
* Source/WebCore/accessibility/AXTextMarker.h:
(WebCore::AXTextMarker::AXTextMarker):
(WebCore::AXTextMarker::treeID const):
(WebCore::AXTextMarker::objectID const):
(WebCore::AXTextMarker::node const):

Canonical link: https://commits.webkit.org/278666@main
  • Loading branch information
@cdumez
cdumez committed May 12, 2024
1 parent 6a9c5f8 commit c0cc24b
Show file tree
Hide file tree
Showing 8 changed files with 127 additions and 120 deletions.
15 changes: 7 additions & 8 deletions Source/WebCore/accessibility/AXObjectCache.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2814,9 +2814,11 @@ Ref<Document> AXObjectCache::protectedDocument() const
return document();
}

template<typename TextMarkerDataType>
VisiblePosition AXObjectCache::visiblePositionForTextMarkerData(const TextMarkerDataType& textMarkerData)
VisiblePosition AXObjectCache::visiblePositionForTextMarkerData(const TextMarkerData& textMarkerData)
{
if (!textMarkerData.node)
return { };

Ref node = *textMarkerData.node;
if (!isNodeInUse(node) || node->isPseudoElement())
return { };
Expand All @@ -2831,16 +2833,13 @@ VisiblePosition AXObjectCache::visiblePositionForTextMarkerData(const TextMarker
return { };

auto* cache = renderer->document().axObjectCache();
if (cache && !cache->m_idsInUse.contains(textMarkerData.axObjectID()))
if (cache && !cache->m_idsInUse.contains(textMarkerData.objectID))
return { };

return visiblePosition;
}

template VisiblePosition AXObjectCache::visiblePositionForTextMarkerData(const TextMarkerData&);
template VisiblePosition AXObjectCache::visiblePositionForTextMarkerData(const SafeTextMarkerData&);

CharacterOffset AXObjectCache::characterOffsetForTextMarkerData(const SafeTextMarkerData& textMarkerData)
CharacterOffset AXObjectCache::characterOffsetForTextMarkerData(const TextMarkerData& textMarkerData)
{
if (textMarkerData.ignored || !textMarkerData.node)
return { };
Expand Down Expand Up @@ -3381,7 +3380,7 @@ CharacterOffset AXObjectCache::characterOffsetFromVisiblePosition(const VisibleP
return result;
}

AccessibilityObject* AXObjectCache::accessibilityObjectForTextMarkerData(const SafeTextMarkerData& textMarkerData)
AccessibilityObject* AXObjectCache::accessibilityObjectForTextMarkerData(const TextMarkerData& textMarkerData)
{
if (textMarkerData.ignored)
return nullptr;
Expand Down
9 changes: 5 additions & 4 deletions Source/WebCore/accessibility/AXObjectCache.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -450,14 +450,14 @@ class AXObjectCache final : public CanMakeWeakPtr<AXObjectCache>, public CanMake
AXTextMarker nextTextMarker(const AXTextMarker&);
TextMarkerData textMarkerDataForPreviousCharacterOffset(const CharacterOffset&);
AXTextMarker previousTextMarker(const AXTextMarker&);
template<typename TextMarkerDataType> VisiblePosition visiblePositionForTextMarkerData(const TextMarkerDataType&);
CharacterOffset characterOffsetForTextMarkerData(const SafeTextMarkerData&);
VisiblePosition visiblePositionForTextMarkerData(const TextMarkerData&);
CharacterOffset characterOffsetForTextMarkerData(const TextMarkerData&);
// 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(const SafeTextMarkerData&);
AccessibilityObject* accessibilityObjectForTextMarkerData(const TextMarkerData&);
std::optional<SimpleRange> rangeForUnorderedCharacterOffsets(const CharacterOffset&, const CharacterOffset&);
static SimpleRange rangeForNodeContents(Node&);
static unsigned lengthForRange(const SimpleRange&);
Expand Down Expand Up @@ -573,6 +573,8 @@ class AXObjectCache final : public CanMakeWeakPtr<AXObjectCache>, public CanMake
static bool clientIsInTestMode();
#endif

bool isNodeInUse(Node& node) const { return m_textMarkerNodes.contains(node); }

#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
void scheduleObjectRegionsUpdate(bool scheduleImmediately = false) { m_geometryManager->scheduleObjectRegionsUpdate(scheduleImmediately); }
void willUpdateObjectRegions() { m_geometryManager->willUpdateObjectRegions(); }
Expand Down Expand Up @@ -626,7 +628,6 @@ class AXObjectCache final : public CanMakeWeakPtr<AXObjectCache>, public CanMake
// This is a weak reference cache for knowing if Nodes used by TextMarkers are valid.
void setNodeInUse(Node& node) { m_textMarkerNodes.add(node); }
void removeNodeForUse(Node& node) { m_textMarkerNodes.remove(node); }
bool isNodeInUse(Node& node) { return m_textMarkerNodes.contains(node); }

// CharacterOffset functions.
enum TraverseOption { TraverseOptionDefault = 1 << 0, TraverseOptionToNodeEnd = 1 << 1, TraverseOptionIncludeStart = 1 << 2, TraverseOptionValidateOffset = 1 << 3, TraverseOptionDoNotEnterTextControls = 1 << 4 };
Expand Down
71 changes: 38 additions & 33 deletions Source/WebCore/accessibility/AXTextMarker.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2023 Apple Inc. All rights reserved.
* Copyright (C) 2023-2024 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -35,46 +35,40 @@
namespace WebCore {
DEFINE_ALLOCATOR_WITH_HEAP_IDENTIFIER(AXTextMarker);

TextMarkerData::TextMarkerData(AXObjectCache& cache, Node* nodeParam, const VisiblePosition& visiblePosition, int charStart, int charOffset, bool ignoredParam)
static AXID nodeID(AXObjectCache& cache, Node* node)
{
initializeAXIDs(cache, nodeParam);
if (RefPtr object = cache.getOrCreate(node))
return object->objectID();
return { };
}

node = nodeParam;
TextMarkerData::TextMarkerData(AXObjectCache& cache, Node* node, const VisiblePosition& visiblePosition, int charStart, int charOffset, bool ignoredParam)
: treeID(cache.treeID())
, objectID(nodeID(cache, node))
, node(node)
, affinity(visiblePosition.affinity())
, characterStart(std::max(charStart, 0))
, characterOffset(std::max(charOffset, 0))
, ignored(ignoredParam)
{
auto position = visiblePosition.deepEquivalent();
offset = !visiblePosition.isNull() ? std::max(position.deprecatedEditingOffset(), 0) : 0;
anchorType = position.anchorType();
affinity = visiblePosition.affinity();

characterStart = std::max(charStart, 0);
characterOffset = std::max(charOffset, 0);
ignored = ignoredParam;
}

TextMarkerData::TextMarkerData(AXObjectCache& cache, const CharacterOffset& characterOffset, bool ignoredParam)
: treeID(cache.treeID())
, objectID(nodeID(cache, characterOffset.node.get()))
, node(characterOffset.node.get())
, anchorType(Position::PositionIsOffsetInAnchor)
, characterStart(std::max(characterOffset.startIndex, 0))
, characterOffset(std::max(characterOffset.offset, 0))
, ignored(ignoredParam)
{
RefPtr characterOffsetNode = characterOffset.node;
initializeAXIDs(cache, characterOffsetNode.get());

node = characterOffsetNode.get();
auto visiblePosition = cache.visiblePositionFromCharacterOffset(characterOffset);
auto position = visiblePosition.deepEquivalent();
offset = !visiblePosition.isNull() ? std::max(position.deprecatedEditingOffset(), 0) : 0;
// When creating from a CharacterOffset, always set the anchorType to PositionIsOffsetInAnchor.
anchorType = Position::PositionIsOffsetInAnchor;
affinity = visiblePosition.affinity();

characterStart = std::max(characterOffset.startIndex, 0);
this->characterOffset = std::max(characterOffset.offset, 0);
ignored = ignoredParam;
}

void TextMarkerData::initializeAXIDs(AXObjectCache& cache, Node* node)
{
memset(static_cast<void*>(this), 0, sizeof(*this));

treeID = cache.treeID().toUInt64();
if (RefPtr object = cache.getOrCreate(node))
objectID = object->objectID().toUInt64();
}

AXTextMarker::AXTextMarker(const VisiblePosition& visiblePosition)
Expand All @@ -94,7 +88,7 @@ AXTextMarker::AXTextMarker(const VisiblePosition& visiblePosition)
return;

if (auto data = cache->textMarkerDataForVisiblePosition(visiblePosition))
m_data = *data;
m_data = WTFMove(*data);
}

AXTextMarker::AXTextMarker(const CharacterOffset& characterOffset)
Expand Down Expand Up @@ -126,7 +120,7 @@ void AXTextMarker::setNodeIfNeeded() const
if (!node)
return;

const_cast<AXTextMarker*>(this)->m_data.node = node.get();
const_cast<AXTextMarker*>(this)->m_data.node = *node;
cache->setNodeInUse(*node);
}

Expand All @@ -149,18 +143,18 @@ AXTextMarker::operator CharacterOffset() const
if (isIgnored() || isNull())
return { };

WeakPtr cache = AXTreeStore<AXObjectCache>::axObjectCacheForID(m_data.axTreeID());
WeakPtr cache = AXTreeStore<AXObjectCache>::axObjectCacheForID(m_data.treeID);
if (!cache)
return { };

if (RefPtr node = m_data.node) {
if (RefPtr node = m_data.node.get()) {
// Make sure that this node is still in cache->m_textMarkerNodes. Since this method can be called as a result of a dispatch from the AX thread, the Node may have gone away in a previous main loop cycle.
if (!cache->isNodeInUse(*node))
return { };
} else
setNodeIfNeeded();

CharacterOffset result((RefPtr { m_data.node }).get(), m_data.characterStart, m_data.characterOffset);
CharacterOffset result(RefPtr { m_data.node.get() }.get(), m_data.characterStart, m_data.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 (m_data.affinity == Affinity::Upstream)
Expand Down Expand Up @@ -916,4 +910,15 @@ std::partial_ordering AXTextMarker::partialOrderByTraversal(const AXTextMarker&
}
#endif // ENABLE(AX_THREAD_TEXT_APIS)

TextMarkerData RawTextMarkerData::toTextMarkerData() const
{
ObjectIdentifier<AXIDType> axTreeID(treeID);
WeakPtr cache = AXTreeStore<AXObjectCache>::axObjectCacheForID(axTreeID);
// Make sure the node is not stale before storing it in a WeakPtr.
Node* validNode = nullptr;
if (node && cache && cache->isNodeInUse(*node))
validNode = node;
return { axTreeID, ObjectIdentifier<AXIDType>(objectID), validNode, offset, anchorType, affinity, characterStart, characterOffset, ignored };
}

} // namespace WebCore
73 changes: 33 additions & 40 deletions Source/WebCore/accessibility/AXTextMarker.h
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2023 Apple Inc. All rights reserved.
* Copyright (C) 2023-2024 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
Expand Down Expand Up @@ -48,9 +48,9 @@ enum class LineRangeType : uint8_t {
Right,
};

struct SafeTextMarkerData;
struct TextMarkerData;

struct TextMarkerData {
struct RawTextMarkerData {
unsigned treeID;
unsigned objectID;

Expand All @@ -63,15 +63,15 @@ struct TextMarkerData {
unsigned characterOffset;
bool ignored;

// Constructors of TextMarkerData must zero the struct's block of memory because platform client code may rely on a byte-comparison to determine instances equality.
// Constructors of RawTextMarkerData must zero the struct's block of memory because platform client code may rely on a byte-comparison to determine instances equality.
// Members initialization alone is not enough to guaranty that all bytes in the struct memeory are initialized, and may cause random inequalities when doing byte-comparisons.
// For an exampel of such byte-comparison, see the TestRunner WTR::AccessibilityTextMarker::isEqual.
TextMarkerData()
// For an example of such byte-comparison, see the TestRunner WTR::AccessibilityTextMarker::isEqual.
RawTextMarkerData()
{
memset(static_cast<void*>(this), 0, sizeof(*this));
}

TextMarkerData(AXID axTreeID, AXID axObjectID,
RawTextMarkerData(AXID axTreeID, AXID axObjectID,
Node* nodeParam = nullptr, unsigned offsetParam = 0,
Position::AnchorType anchorTypeParam = Position::PositionIsOffsetInAnchor,
Affinity affinityParam = Affinity::Downstream,
Expand All @@ -89,29 +89,13 @@ struct TextMarkerData {
ignored = ignoredParam;
}

TextMarkerData(AXObjectCache&, Node*, const VisiblePosition&, int charStart = 0, int charOffset = 0, bool ignoredParam = false);
TextMarkerData(AXObjectCache&, const CharacterOffset&, bool ignoredParam = false);

AXID axTreeID() const
{
return ObjectIdentifier<AXIDType>(treeID);
}

AXID axObjectID() const
{
return ObjectIdentifier<AXIDType>(objectID);
}

SafeTextMarkerData toSafeTextMarkerData() const;

private:
void initializeAXIDs(AXObjectCache&, Node*);
TextMarkerData toTextMarkerData() const;
};

// Safer version of TextMarkerData with a WeakPtr for Node.
// TextMarkerData uses a raw pointer for Node because it is
// Safer version of RawTextMarkerData with a WeakPtr for Node.
// RawTextMarkerData uses a raw pointer for Node because it is
// used with memset / memcmp.
struct SafeTextMarkerData {
struct TextMarkerData {
AXID treeID;
AXID objectID;

Expand All @@ -124,20 +108,29 @@ struct SafeTextMarkerData {
unsigned characterOffset { 0 };
bool ignored { false };

TextMarkerData toTextMarkerData() const;
AXID axObjectID() const { return objectID; }
TextMarkerData() = default;
TextMarkerData(AXObjectCache&, Node*, const VisiblePosition&, int charStart = 0, int charOffset = 0, bool ignoredParam = false);
TextMarkerData(AXID treeID, AXID objectID, Node* node, unsigned offset, Position::AnchorType anchorType = Position::AnchorType::PositionIsOffsetInAnchor, Affinity affinity = Affinity::Upstream, unsigned characterStart = 0, unsigned characterOffset = 0, bool ignored = false)
: treeID(treeID)
, objectID(objectID)
, node(node)
, offset(offset)
, anchorType(anchorType)
, affinity(affinity)
, characterStart(characterStart)
, characterOffset(characterOffset)
, ignored(ignored)
{ }
TextMarkerData(AXObjectCache&, const CharacterOffset&, bool ignoredParam = false);

RawTextMarkerData toRawTextMarkerData() const;
};

inline TextMarkerData SafeTextMarkerData::toTextMarkerData() const
inline RawTextMarkerData TextMarkerData::toRawTextMarkerData() 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)
Expand All @@ -157,7 +150,7 @@ class AXTextMarker {
: m_data(data)
{ }
AXTextMarker(TextMarkerData&& data)
: m_data(data)
: m_data(WTFMove(data))
{ }
#if PLATFORM(COCOA)
AXTextMarker(PlatformTextMarkerData);
Expand All @@ -178,8 +171,8 @@ class AXTextMarker {
operator PlatformTextMarkerData() const { return platformData().autorelease(); }
#endif

AXID treeID() const { return m_data.axTreeID(); }
AXID objectID() const { return m_data.axObjectID(); }
AXID treeID() const { return m_data.treeID; }
AXID objectID() const { return m_data.objectID; }
unsigned offset() const { return m_data.offset; }
bool isNull() const { return !treeID().isValid() || !objectID().isValid(); }
#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
Expand Down Expand Up @@ -245,7 +238,7 @@ class AXTextMarker {
bool atLineEnd() const { return atLineBoundaryForDirection(AXDirection::Next); }
#endif // ENABLE(AX_THREAD_TEXT_APIS)

TextMarkerData m_data; // FIXME: This should be a SafeTextMarkerData.
TextMarkerData m_data;
};

class AXTextMarkerRange {
Expand Down Expand Up @@ -296,7 +289,7 @@ class AXTextMarkerRange {
inline Node* AXTextMarker::node() const
{
ASSERT(isMainThread());
return m_data.node;
return m_data.node.get();
}

} // namespace WebCore
15 changes: 10 additions & 5 deletions Source/WebCore/accessibility/cocoa/AXTextMarkerCocoa.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,18 @@
return;
}

if (AXTextMarkerGetLength(platformData) != sizeof(m_data)) {
RawTextMarkerData rawTextMarkerData;
if (AXTextMarkerGetLength(platformData) != sizeof(rawTextMarkerData)) {
ASSERT_NOT_REACHED();
return;
}

memcpy(&m_data, AXTextMarkerGetBytePtr(platformData), sizeof(m_data));
memcpy(&rawTextMarkerData, AXTextMarkerGetBytePtr(platformData), sizeof(rawTextMarkerData));
m_data = rawTextMarkerData.toTextMarkerData();
#else // PLATFORM(IOS_FAMILY)
[platformData getBytes:&m_data length:sizeof(m_data)];
RawTextMarkerData rawTextMarkerData;
[platformData getBytes:&rawTextMarkerData length:sizeof(rawTextMarkerData)];
m_data = rawTextMarkerData.toTextMarkerData();
#endif

if (isMainThread())
Expand All @@ -62,10 +66,11 @@

RetainPtr<PlatformTextMarkerData> AXTextMarker::platformData() const
{
auto rawTextMarkerData = m_data.toRawTextMarkerData();
#if PLATFORM(MAC)
return adoptCF(AXTextMarkerCreate(kCFAllocatorDefault, (const UInt8*)&m_data, sizeof(m_data)));
return adoptCF(AXTextMarkerCreate(kCFAllocatorDefault, (const UInt8*)&rawTextMarkerData, sizeof(rawTextMarkerData)));
#else // PLATFORM(IOS_FAMILY)
return [NSData dataWithBytes:&m_data length:sizeof(m_data)];
return [NSData dataWithBytes:&rawTextMarkerData length:sizeof(rawTextMarkerData)];
#endif
}

Expand Down
Loading

0 comments on commit c0cc24b

Please sign in to comment.