Bug 268997

Summary:

AX: Backing object can be destroyed in _accessibilityActivate

Product:

WebKit

Reporter:

Joshua Hoffman <jhoffman23>

Component:

Accessibility

Assignee:

Joshua Hoffman <jhoffman23>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, samuel_white, tyler_w, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Joshua Hoffman

Reported 2024-02-08 09:51:23 PST

By calling press() on the backing object, we may destroy the object.

Attachments
Patch (1.87 KB, patch) 2024-02-08 09:58 PST, Joshua Hoffman	no flags	Details Formatted Diff Diff
Patch (1.87 KB, patch) 2024-02-08 10:23 PST, Joshua Hoffman	no flags	Details Formatted Diff Diff
Patch (3.88 KB, patch) 2024-02-08 20:21 PST, Joshua Hoffman	no flags	Details Formatted Diff Diff
Patch (3.83 KB, patch) 2024-02-09 10:52 PST, Joshua Hoffman	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2024-02-08 09:51:35 PST

<rdar://problem/122554193>

Joshua Hoffman

Comment 2 2024-02-08 09:51:42 PST

rdar://122491194

Joshua Hoffman

Comment 3 2024-02-08 09:58:25 PST

Created attachment 469773 [details] Patch

Joshua Hoffman

Comment 4 2024-02-08 10:23:29 PST

Created attachment 469775 [details] Patch

chris fleizach

Comment 5 2024-02-08 12:43:01 PST

Comment on attachment 469775 [details] Patch feels like we might be able to get a test for this by destroying a button on a press

Joshua Hoffman

Comment 6 2024-02-08 14:40:13 PST

(In reply to chris fleizach from comment #5) > Comment on attachment 469775 [details] > Patch > > feels like we might be able to get a test for this by destroying a button on > a press Good idea, I can add one here.

Joshua Hoffman

Comment 7 2024-02-08 20:21:04 PST

Created attachment 469784 [details] Patch

Tyler Wilcock

Comment 8 2024-02-09 10:46:56 PST

Comment on attachment 469784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=469784&action=review > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:2011 > + RefPtr<AccessibilityObject> backingObject = self.axBackingObject; Nit: I'd personally remove the newline before this line, grouping the statements of preparing, and subsequently protecting if said preparation is successful > LayoutTests/accessibility/ios-simulator/destroy-on-press.html:7 > +<body id="body"> Is this id necessary? > LayoutTests/accessibility/ios-simulator/destroy-on-press.html:26 > + let output = "This makes sure that we don't crash when activating an element that destroys itself.\n\n" > + > + if (window.accessibilityController) { > + // Should not crash here > + accessibilityController.accessibleElementById("summary1").childAtIndex(0).press(); > + output += "Did not crash when pressing #summary1."; > + debug(output); > + } > + > + function destroySummary() { > + document.getElementById("summary1").remove(); > + } I prefer not to indent inside a script tag in layout tests (easier to read).

Joshua Hoffman

Comment 9 2024-02-09 10:52:09 PST

Created attachment 469798 [details] Patch

Joshua Hoffman

Comment 10 2024-02-09 10:52:39 PST

(In reply to Tyler Wilcock from comment #8) > Comment on attachment 469784 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=469784&action=review > > > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:2011 > > + RefPtr<AccessibilityObject> backingObject = self.axBackingObject; > > Nit: I'd personally remove the newline before this line, grouping the > statements of preparing, and subsequently protecting if said preparation is > successful > > > LayoutTests/accessibility/ios-simulator/destroy-on-press.html:7 > > +<body id="body"> > > Is this id necessary? > > > LayoutTests/accessibility/ios-simulator/destroy-on-press.html:26 > > + let output = "This makes sure that we don't crash when activating an element that destroys itself.\n\n" > > + > > + if (window.accessibilityController) { > > + // Should not crash here > > + accessibilityController.accessibleElementById("summary1").childAtIndex(0).press(); > > + output += "Did not crash when pressing #summary1."; > > + debug(output); > > + } > > + > > + function destroySummary() { > > + document.getElementById("summary1").remove(); > > + } > > I prefer not to indent inside a script tag in layout tests (easier to read). Addressed all of these in the latest attachment

EWS

Comment 11 2024-02-12 11:57:23 PST

Committed 274477@main (fcc36e12e4cb): <https://commits.webkit.org/274477@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 469798 [details].

Note You need to log in before you can comment on or make changes to this bug.