AX: Live AX objects can be destroyed while building a node change for them
https://bugs.webkit.org/show_bug.cgi?id=241810
Reviewed by Chris Fleizach.
While building a node change in AXIsolatedTree::nodeChangeForObject,
the initialization of several different `AXPropertyName`s can cause layout,
which in turn can cause the backing live object to be deleted. This
causes a crash.
This patch fixes this by holding a `Ref` to the AccessibilityObject,
ensuring it stays alive for as long we need it.
I wasn't able to make a layout test for this, as the circumstances to
reproduce the issue are complex.
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:
(WebCore::AXIsolatedObject::AXIsolatedObject):
(WebCore::AXIsolatedObject::create):
(WebCore::AXIsolatedObject::initializeProperties):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:
(WebCore::AXIsolatedTree::nodeChangeForObject):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:
* Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm:
(WebCore::AXIsolatedObject::initializePlatformProperties):
Canonical link: https://commits.webkit.org/251726@main
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@295721 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
index 6f4b4fd..4dfd7c7 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
@@ -38,18 +38,18 @@
namespace WebCore {
-AXIsolatedObject::AXIsolatedObject(AXCoreObject& axObject, AXIsolatedTree* tree)
+AXIsolatedObject::AXIsolatedObject(Ref<AXCoreObject> axObject, AXIsolatedTree* tree)
: m_cachedTree(tree)
- , m_id(axObject.objectID())
+ , m_id(axObject->objectID())
{
ASSERT(isMainThread());
ASSERT(is<AccessibilityObject>(axObject));
- auto* axParent = axObject.parentObjectUnignored();
+ auto* axParent = axObject->parentObjectUnignored();
m_parentID = axParent ? axParent->objectID() : AXID();
if (m_id.isValid()) {
- auto isRoot = !axParent && axObject.isScrollView() ? IsRoot::Yes : IsRoot::No;
+ auto isRoot = !axParent && axObject->isScrollView() ? IsRoot::Yes : IsRoot::No;
initializeProperties(axObject, isRoot);
} else {
// Should never happen under normal circumstances.
@@ -57,7 +57,7 @@
}
}
-Ref<AXIsolatedObject> AXIsolatedObject::create(AXCoreObject& object, AXIsolatedTree* tree)
+Ref<AXIsolatedObject> AXIsolatedObject::create(Ref<AXCoreObject> object, AXIsolatedTree* tree)
{
return adoptRef(*new AXIsolatedObject(object, tree));
}
@@ -72,10 +72,10 @@
ASSERT_NOT_REACHED();
}
-void AXIsolatedObject::initializeProperties(AXCoreObject& coreObject, IsRoot isRoot)
+void AXIsolatedObject::initializeProperties(Ref<AXCoreObject> coreObject, IsRoot isRoot)
{
ASSERT(is<AccessibilityObject>(coreObject));
- auto& object = downcast<AccessibilityObject>(coreObject);
+ auto& object = downcast<AccessibilityObject>(coreObject.get());
setProperty(AXPropertyName::ARIALandmarkRoleDescription, object.ariaLandmarkRoleDescription().isolatedCopy());
setProperty(AXPropertyName::AccessibilityDescription, object.accessibilityDescription().isolatedCopy());
@@ -400,7 +400,7 @@
if (object.isWidget())
setProperty(AXPropertyName::IsWidget, true);
- initializePlatformProperties(object, isRoot);
+ initializePlatformProperties(coreObject, isRoot);
}
AXCoreObject* AXIsolatedObject::associatedAXObject() const
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
index 5abce8f..9513dc8 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
@@ -47,7 +47,7 @@
class AXIsolatedObject final : public AXCoreObject {
friend class AXIsolatedTree;
public:
- static Ref<AXIsolatedObject> create(AXCoreObject&, AXIsolatedTree*);
+ static Ref<AXIsolatedObject> create(Ref<AXCoreObject>, AXIsolatedTree*);
~AXIsolatedObject();
void setObjectID(AXID id) override { m_id = id; }
@@ -66,13 +66,13 @@
AXIsolatedTree* tree() const { return m_cachedTree.get(); }
AXIsolatedObject() = default;
- AXIsolatedObject(AXCoreObject&, AXIsolatedTree*);
+ AXIsolatedObject(Ref<AXCoreObject>, AXIsolatedTree*);
bool isAXIsolatedObjectInstance() const override { return true; }
AXCoreObject* associatedAXObject() const;
enum class IsRoot : bool { Yes, No };
- void initializeProperties(AXCoreObject&, IsRoot);
- void initializePlatformProperties(const AXCoreObject&, IsRoot);
+ void initializeProperties(Ref<AXCoreObject>, IsRoot);
+ void initializePlatformProperties(Ref<const AXCoreObject>, IsRoot);
void setProperty(AXPropertyName, AXPropertyValueVariant&&, bool shouldRemove = false);
void setObjectProperty(AXPropertyName, AXCoreObject*);
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 22662e9..bd83a98 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -209,15 +209,15 @@
queueRemovalsAndUnresolvedChanges({ });
}
-std::optional<AXIsolatedTree::NodeChange> AXIsolatedTree::nodeChangeForObject(AXCoreObject& axObject, AttachWrapper attachWrapper)
+std::optional<AXIsolatedTree::NodeChange> AXIsolatedTree::nodeChangeForObject(Ref<AXCoreObject> axObject, AttachWrapper attachWrapper)
{
ASSERT(isMainThread());
// We should never create an isolated object from an ignored object.
- if (axObject.accessibilityIsIgnored())
+ if (axObject->accessibilityIsIgnored())
return std::nullopt;
- if (!axObject.objectID().isValid()) {
+ if (!axObject->objectID().isValid()) {
// Either the axObject has an invalid ID or something else went terribly wrong. Don't bother doing anything else.
ASSERT_NOT_REACHED();
return std::nullopt;
@@ -226,15 +226,15 @@
auto object = AXIsolatedObject::create(axObject, this);
NodeChange nodeChange { object, nullptr };
- ASSERT(axObject.wrapper());
+ ASSERT(axObject->wrapper());
if (attachWrapper == AttachWrapper::OnMainThread)
- object->attachPlatformWrapper(axObject.wrapper());
+ object->attachPlatformWrapper(axObject->wrapper());
else {
// Set the wrapper in the NodeChange so that it is set on the AX thread.
- nodeChange.wrapper = axObject.wrapper();
+ nodeChange.wrapper = axObject->wrapper();
}
- m_nodeMap.set(axObject.objectID(), ParentChildrenIDs { nodeChange.isolatedObject->parent(), axObject.childrenIDs() });
+ m_nodeMap.set(axObject->objectID(), ParentChildrenIDs { nodeChange.isolatedObject->parent(), axObject->childrenIDs() });
if (!nodeChange.isolatedObject->parent().isValid()) {
Locker locker { m_changeLogLock };
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
index 2497b52f..228e0e0 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
@@ -385,7 +385,7 @@
static HashMap<PageIdentifier, Ref<AXIsolatedTree>>& treePageCache() WTF_REQUIRES_LOCK(s_cacheLock);
enum class AttachWrapper : bool { OnMainThread, OnAXThread };
- std::optional<NodeChange> nodeChangeForObject(AXCoreObject&, AttachWrapper = AttachWrapper::OnMainThread);
+ std::optional<NodeChange> nodeChangeForObject(Ref<AXCoreObject>, AttachWrapper = AttachWrapper::OnMainThread);
void collectNodeChangesForSubtree(AXCoreObject&);
bool isCollectingNodeChanges() const { return m_collectingNodeChangesAtTreeLevel > 0; }
void queueChange(const NodeChange&) WTF_REQUIRES_LOCK(m_changeLogLock);
diff --git a/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm b/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm
index ed19415..fdc26fe 100644
--- a/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm
+++ b/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm
@@ -32,18 +32,18 @@
namespace WebCore {
-void AXIsolatedObject::initializePlatformProperties(const AXCoreObject& object, IsRoot isRoot)
+void AXIsolatedObject::initializePlatformProperties(Ref<const AXCoreObject> object, IsRoot isRoot)
{
- setProperty(AXPropertyName::HasApplePDFAnnotationAttribute, object.hasApplePDFAnnotationAttribute());
- setProperty(AXPropertyName::SpeechHint, object.speechHintAttributeValue().isolatedCopy());
- setProperty(AXPropertyName::CaretBrowsingEnabled, object.caretBrowsingEnabled());
+ setProperty(AXPropertyName::HasApplePDFAnnotationAttribute, object->hasApplePDFAnnotationAttribute());
+ setProperty(AXPropertyName::SpeechHint, object->speechHintAttributeValue().isolatedCopy());
+ setProperty(AXPropertyName::CaretBrowsingEnabled, object->caretBrowsingEnabled());
if (isRoot == IsRoot::Yes)
- setProperty(AXPropertyName::PreventKeyboardDOMEventDispatch, object.preventKeyboardDOMEventDispatch());
+ setProperty(AXPropertyName::PreventKeyboardDOMEventDispatch, object->preventKeyboardDOMEventDispatch());
- if (object.isScrollView()) {
- m_platformWidget = object.platformWidget();
- m_remoteParent = object.remoteParentObject();
+ if (object->isScrollView()) {
+ m_platformWidget = object->platformWidget();
+ m_remoteParent = object->remoteParentObject();
}
}
