Skip to content

AX: Add a failsafe to the while loop in AXCoreObject::partialOrder #47392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 1, 2025
Merged

AX: Add a failsafe to the while loop in AXCoreObject::partialOrder #47392

merged 1 commit into from
Jul 1, 2025
+20 −1

Conversation

twilco
Copy link
Contributor

@twilco twilco commented Jun 30, 2025 •
edited by webkit-early-warning-system
Loading

c187483

AX: Add a failsafe to the while loop in AXCoreObject::partialOrder
https://bugs.webkit.org/show_bug.cgi?id=295222
rdar://154696128

Reviewed by Joshua Hoffman.

When testing a different bug, I was able to enter an infinite loop in this function. I tried to rebuild with logging
to debug, but wasn't able to hit it again despite much effort.

To avoid a permanent hang for our users, add a failsafe counter, and an ASSERT which should hopefully help us debug the
root cause when it triggers.

* Source/WebCore/accessibility/AXCoreObject.cpp:
(WebCore::AXCoreObject::partialOrder):

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

bd4754d

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@twilco twilco self-assigned this Jun 30, 2025
@twilco twilco added the Accessibility For bugs related to accessibility. label Jun 30, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 30, 2025 •
edited
Loading

EWS run on previous version of this PR (hash fb364f3)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2   🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2 ⏳ 🧪 api-mac ✅ 🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 wpe-cairo
  🧪 api-ios   🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ⏳ 🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim   🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ⏳ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

minorninth
minorninth reviewed Jun 30, 2025
View reviewed changes
Copy link
Contributor

@minorninth minorninth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Source/WebCore/accessibility/AXCoreObject.cpp
while (current || otherCurrent) {
while (failsafeCounter < maxIterations && (current || otherCurrent)) {
++failsafeCounter;

if (RefPtr maybeParent = current ? current->parentObject() : nullptr) {
if (maybeParent == &other) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it'd be worth a debug assert that maybeParent isn't the same as current? While of course that should never happen, that feels like one of the most likely ways we could end up in an endless loop here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Done in latest commit.

@twilco twilco force-pushed the eng/AX-Add-a-failsafe-to-the-while-loop-in-AXCoreObject-partialOrder branch from fb364f3 to a66ff09 Compare June 30, 2025 21:00
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 30, 2025 •
edited
Loading

EWS run on previous version of this PR (hash a66ff09)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2   🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2 ⏳ 🧪 api-mac ✅ 🧪 api-wpe
  🧪 ios-wk2-wpt ⏳ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
  🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision   🧪 mac-AS-debug-wk2 ⏳ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress   🧪 api-gtk
✅ 🧪 vision-wk2 ⏳ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

hoffmanjoshua
hoffmanjoshua approved these changes Jun 30, 2025
View reviewed changes
Source/WebCore/accessibility/AXCoreObject.cpp
// For now, add a failsafe and ASSERT() so we can try to debug the root cause when it does
// happen again.
unsigned failsafeCounter = 0;
static constexpr unsigned maxIterations = Settings::defaultMaximumRenderTreeDepth * 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need 2* the maximum depth? A comment explaining that might be helpful here

Copy link
Contributor Author

@twilco twilco Jun 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To ensure, without a doubt, that this failsafe only triggers when something is wrong. Just Settings::defaultMaximumRenderTreeDepth would be wrong — the accessibility tree can be deeper than that when considering things like scrollviews, which in the ancestry chain but not associated with any renderer.

Comment added in latest commit

@twilco twilco force-pushed the eng/AX-Add-a-failsafe-to-the-while-loop-in-AXCoreObject-partialOrder branch from a66ff09 to bd4754d Compare June 30, 2025 21:37
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 30, 2025 •
edited
Loading

EWS run on current version of this PR (hash bd4754d)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@twilco twilco added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Jun 30, 2025
@webkit-ews-buildbot
Copy link
Collaborator

Failed api-gtk checks. Please resolve failures and re-apply safe-merge-queue label.

Rejecting #47392 from merge queue.

@webkit-ews-buildbot webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Jul 1, 2025
@webkit-ews-buildbot
Copy link
Collaborator

Safe-Merge-Queue: Build #61904.

@twilco twilco added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Jul 1, 2025
@twilco
AX: Add a failsafe to the while loop in AXCoreObject::partialOrder
c187483 
https://bugs.webkit.org/show_bug.cgi?id=295222
rdar://154696128

Reviewed by Joshua Hoffman.

When testing a different bug, I was able to enter an infinite loop in this function. I tried to rebuild with logging
to debug, but wasn't able to hit it again despite much effort.

To avoid a permanent hang for our users, add a failsafe counter, and an ASSERT which should hopefully help us debug the
root cause when it triggers.

* Source/WebCore/accessibility/AXCoreObject.cpp:
(WebCore::AXCoreObject::partialOrder):

Canonical link: https://commits.webkit.org/296846@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/AX-Add-a-failsafe-to-the-while-loop-in-AXCoreObject-partialOrder branch from bd4754d to c187483 Compare July 1, 2025 02:18
@webkit-commit-queue
Copy link
Collaborator

Committed 296846@main (c187483): https://commits.webkit.org/296846@main

Reviewed commits have been landed. Closing PR #47392 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit c187483 into WebKit:main Jul 1, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minorninth minorninth minorninth left review comments

@hoffmanjoshua hoffmanjoshua hoffmanjoshua approved these changes

Assignees

@twilco twilco

Labels
Accessibility For bugs related to accessibility.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@twilco @webkit-early-warning-system @webkit-ews-buildbot @webkit-commit-queue @minorninth @hoffmanjoshua