[webkit-changes] [WebKit/WebKit] 52a7fd: AX: Make m_nodeObjectMapping a WeakHashMap to avoi...

[Tyler Wilcock]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 52a7fd41f9066841d26530ac96f5c0310d42e1c0
      https://github.com/WebKit/WebKit/commit/52a7fd41f9066841d26530ac96f5c0310d42e1c0
  Author: Tyler Wilcock <tyler_w at apple.com>
  Date:   2025-01-23 (Thu, 23 Jan 2025)

  Changed paths:
    M Source/WebCore/accessibility/AXObjectCache.cpp
    M Source/WebCore/accessibility/AXObjectCache.h
    M Source/WebCore/dom/Node.cpp

  Log Message:
  -----------
  AX: Make m_nodeObjectMapping a WeakHashMap to avoid WeakRef crashes
https://bugs.webkit.org/show_bug.cgi?id=286398
rdar://143446394

Reviewed by Chris Fleizach.

We made an attempt to fix this crash in https://commits.webkit.org/289060@main, but have since found a website
where this bug reproduces consistently, confirming that the speculative fix from that commit did not work. The crash
is reproducible by turning VoiceOver on and starting a Speedometer run at https://browserbench.org/Speedometer3.0.

Fundamentally, we are failing to remove nodes from m_nodeObjectMapping, which prior to this commit was of type:

UncheckedKeyHashMap<WeakRef<Node, WeakPtrImplWithEventTargetData>, AXID>

This happens due to this sequence:

  1. We add a node to the map
  2. The document that node belongs to gets its m_frame set to nullptr via Document::willDetachFrame().
  3. When that node is destroyed, we can no longer get an existingAXObjectCache() to clean it up, since
     the node's document has no m_frame. The WeakRef for that node in m_nodeObjectMapping becomes null.
  4. Something else tries to access m_nodeObjectMapping, causing a lookup. During the lookup, when we try
     to compare against the null WeakRef, we crash.

One option would be to clear out AXObjectCache::m_nodeObjectMapping when willDetachFrame() is called. But its not clear
that's always the right thing to do, as the document and frame lifecycle is complicated when considering things like
the back-forward cache.

As a stop-gap fix to prevent the crashes and ensure at least m_nodeObjectMapping is cleaned up (even though we're still
missing clean-up for other things in AXObjectCache::remove(Node&)), make m_nodeObjectMapping a WeakHashMap. This is
an improvement over status quo — we stop crashing, and we leak less. This seems fine for now, as this scenario seems
very rare.

* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::get const):
(WebCore::AXObjectCache::remove):
(WebCore::AXObjectCache::onRendererCreated):
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/dom/Node.cpp:
(WebCore::Node::~Node):

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



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications