Skip to content
Permalink
Browse files
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: https://svn.webkit.org/repository/webkit/trunk@295721 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
twilco committed Jun 22, 2022
1 parent 3b7a0aa commit 49c1b53f803fe6952e293bfde206b22dc4ceadef
Showing 5 changed files with 28 additions and 28 deletions.
@@ -38,26 +38,26 @@

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.
ASSERT_NOT_REACHED();
}
}

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 @@ void AXIsolatedObject::init()
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 @@ void AXIsolatedObject::initializeProperties(AXCoreObject& coreObject, IsRoot isR
if (object.isWidget())
setProperty(AXPropertyName::IsWidget, true);

initializePlatformProperties(object, isRoot);
initializePlatformProperties(coreObject, isRoot);
}

AXCoreObject* AXIsolatedObject::associatedAXObject() const
@@ -47,7 +47,7 @@ class AXIsolatedTree;
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 @@ class AXIsolatedObject final : public AXCoreObject {
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*);
@@ -209,15 +209,15 @@ void AXIsolatedTree::generateSubtree(AXCoreObject& axObject)
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 @@ std::optional<AXIsolatedTree::NodeChange> AXIsolatedTree::nodeChangeForObject(AX
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 };
@@ -385,7 +385,7 @@ class AXIsolatedTree : public ThreadSafeRefCounted<AXIsolatedTree> {
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);
@@ -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();
}
}

0 comments on commit 49c1b53

Please sign in to comment.