- Revision
- 295721
- Author
- [email protected]
- Date
- 2022-06-21 21:11:35 -0700 (Tue, 21 Jun 2022)
Log Message
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
Modified Paths
Diff
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp (295720 => 295721)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp 2022-06-22 03:50:19 UTC (rev 295720)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp 2022-06-22 04:11:35 UTC (rev 295721)
@@ -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
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h (295720 => 295721)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h 2022-06-22 03:50:19 UTC (rev 295720)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h 2022-06-22 04:11:35 UTC (rev 295721)
@@ -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*);
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp (295720 => 295721)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2022-06-22 03:50:19 UTC (rev 295720)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2022-06-22 04:11:35 UTC (rev 295721)
@@ -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 };
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h (295720 => 295721)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2022-06-22 03:50:19 UTC (rev 295720)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2022-06-22 04:11:35 UTC (rev 295721)
@@ -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);
Modified: trunk/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm (295720 => 295721)
--- trunk/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm 2022-06-22 03:50:19 UTC (rev 295720)
+++ trunk/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm 2022-06-22 04:11:35 UTC (rev 295721)
@@ -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();
}
}
