AX: On macOS with site isolation enabled, VoiceOver cannot traverse into iframes because UIElementsForSearchPredicate only searches within the same process#58826
Conversation
|
EWS run on previous version of this PR (hash 3852830) Details
|
3852830 to
0dd5954
Compare
|
EWS run on previous version of this PR (hash 0dd5954) Details
|
0dd5954 to
b2753a3
Compare
|
EWS run on previous version of this PR (hash b2753a3) Details
|
Safer C++ Build #81085 (b2753a3)❌ Found 4 failing files with 4 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
b2753a3 to
5dc6d38
Compare
|
EWS run on previous version of this PR (hash 5dc6d38) Details
|
minorninth
left a comment
There was a problem hiding this comment.
Great work! A few ideas and questions, but no architectural concerns, this looks like a very comprehensive design
| // Token used to identify accessibility objects across process boundaries. | ||
| // On Mac, this is an opaque byte array from NSAccessibilityRemoteUIElement. | ||
| // On other platforms, it's a UUID and process ID pair. | ||
| struct AccessibilityRemoteToken { |
There was a problem hiding this comment.
Thanks for abstracting this!
|
|
||
| // FIXME: This should be replaced by AXDirection (or vice versa). | ||
| enum class AccessibilitySearchDirection { | ||
| enum class AccessibilitySearchDirection : uint8_t { |
There was a problem hiding this comment.
FYI: This might make sense if AccessibilitySearchDirection is in a packed struct and saves a significant number of bytes, but could actually hurt performance in other cases (adds a truncation/masking instruction every time a value that's not already uint8 is assigned).
There was a problem hiding this comment.
Totally makes sense! The motivation for doing this is because we are now sending this over IPC between web content processes, and having an explicitly defined size is required for that.
|
|
||
| // Spins the run loop on the main thread while waiting for a condition to become true. | ||
| template<typename Predicate> | ||
| static DidTimeout spinRunLoopUntil(Predicate&& isComplete, Seconds timeout) |
There was a problem hiding this comment.
This seems totally reasonable for now, but should we add a comment or file a radar about updating this to exit the run loop immediately when the condition is hit, enabling using a longer timeout rather than polling?
| // convert them into results. | ||
| auto tokens = coordinator->takeRemoteResults(entry.streamIndex()); | ||
| for (auto& token : tokens) { | ||
| if (results.size() >= limit) |
There was a problem hiding this comment.
This feels weirdly asymmetric to be checking the size at the top of the outer for loop, and also at the top of the inner for loop.
What about instead, just check after each append:
for (const auto& entry : entries) {
if (RefPtr object = entry.objectIfLocalResult()) {
results.append(...);
if (results.size() >= limit)
break;
} else if (coordinator) {
...
for (auto& token : tokens) {
results.append(...);
if (results.size() >= limit)
break;
}
}
There was a problem hiding this comment.
Your suggestion would break out of the inner for loop, but not the outer one, which is why the code is structured as-is. Happy to adjust though if you have another idea!
| return crossProcessSearchTimeout; | ||
|
|
||
| auto remaining = *deadline - MonotonicTime::now() - crossProcessSearchIPCOverhead; | ||
| return std::max(crossProcessSearchMinimumTimeout, remaining); |
There was a problem hiding this comment.
Let's say a process receives the IPC super late, after the query timed out. We should probably just skip the search in that case, otherwise when a process finally becomes unblocked it could waste time servicing a long queue of expired queries.
There was a problem hiding this comment.
Good point! Will fix in latest commit.
| criteriaForIPC.deadline = MonotonicTime::now() + crossProcessSearchTimeout; | ||
|
|
||
| if (!treeID) { | ||
| // No tree ID means we can't coordinate with remote frames. |
There was a problem hiding this comment.
Would it be worth merging this with the non-Mac case? Like basically have a canDoRemoteSearch flag that's always false on non-Mac, but if there's no treeID, Mac could set that flag - then have a single fallback block.
Another idea is that for debugging we might want to have a flag to disable remote search.
| } | ||
|
|
||
| #if PLATFORM(MAC) | ||
| // Ref-counted class for safe parent search coordination across threads. |
There was a problem hiding this comment.
Does Parent here mean "Parent Frame"? Or does it mean the caller of the IPC? I get the issue with preventing UAF if the callback comes after the timeout, but it's not clear what "parent" here refers to, it feels like it should actually relate to whoever initiated the search.
There was a problem hiding this comment.
It does mean "Parent frame" — it's the context we send to the parent frame via IPC to continue the search from where we, a child frame, are. I will rename this to ParentFrameSearchContext since maybe that could help avoid confusion?
There was a problem hiding this comment.
Also improved the code comment to help better describe what's happening here.
| if (auto token = tokenFromWrapper(protect(object->wrapper()))) | ||
| results.append(WTF::move(*token)); | ||
| } else if (result.isRemote()) | ||
| results.append(*result.remoteToken()); |
There was a problem hiding this comment.
One thought: it probably wouldn't be that wasteful to store a tiny bit more info here other than the remote token - you could also return the role of the result, for example, enabling tests to provide more info than just a bunch of remote tokens.
There was a problem hiding this comment.
I explored this. I agree that it wouldn't be overly wasteful or costly to do it, but the mechanics of piping the information through ended up being a bit complicated. I think a better option may just be porting these tests to be client tests in a later PR, where we can get the role for free.
Safer C++ Build #81154 (5dc6d38)❌ Found 2 failing files with 2 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
https://bugs.webkit.org/show_bug.cgi?id=308161 rdar://170668752 Reviewed by NOBODY (OOPS!). When accessibility is enabled via enableAccessibilityForAllProcesses, set a flag that this has happened so that we can eagerly also enable accessibility in any new processes that are created (e.g. via site isolated iframes). This is important because otherwise the new web content process may reject accessibility state synchronization from other processes, or reject accessibility requests that stared in another web content process and moved to this one (i.e. for WebKit#58826 which propagates UIElementsForSearchPredicate requests between processes). * Source/WebKit/Shared/WebPageCreationParameters.h: * Source/WebKit/Shared/WebPageCreationParameters.serialization.in: * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::enableAccessibilityForAllProcesses): (WebKit::WebPageProxy::creationParameters): * Source/WebKit/UIProcess/WebPageProxy.h: * Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundleFrame.cpp: (_WKAccessibilityRootObjectForTesting):
5dc6d38 to
ff88ed5
Compare
|
EWS run on previous version of this PR (hash ff88ed5) Details
|
ff88ed5 to
69a20d9
Compare
|
EWS run on previous version of this PR (hash 69a20d9) Details
|
69a20d9 to
d165df5
Compare
|
EWS run on previous version of this PR (hash d165df5) Details
|
https://bugs.webkit.org/show_bug.cgi?id=308161 rdar://170668752 Reviewed by Joshua Hoffman. When accessibility is enabled via enableAccessibilityForAllProcesses, set a flag that this has happened so that we can eagerly also enable accessibility in any new processes that are created (e.g. via site isolated iframes). This is important because otherwise the new web content process may reject accessibility state synchronization from other processes, or reject accessibility requests that stared in another web content process and moved to this one (i.e. for #58826 which propagates UIElementsForSearchPredicate requests between processes). * Source/WebKit/Shared/WebPageCreationParameters.h: * Source/WebKit/Shared/WebPageCreationParameters.serialization.in: * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::enableAccessibilityForAllProcesses): (WebKit::WebPageProxy::creationParameters): * Source/WebKit/UIProcess/WebPageProxy.h: * Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundleFrame.cpp: (_WKAccessibilityRootObjectForTesting): Canonical link: https://commits.webkit.org/307836@main
d165df5 to
71d544f
Compare
|
EWS run on previous version of this PR (hash 71d544f) Details
|
iOS Safer C++ Build #1174 (71d544f)❌ Found 3 failing files with 3 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
I'd rephrase that to:
it's not like we have "the" local process. It's that we're doing the search within a single process. |
macOS Safer C++ Build #82882 (71d544f)❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
71d544f to
2562f37
Compare
|
EWS run on current version of this PR (hash 2562f37) Details
|
Fixed, thanks! |
iOS Safer C++ Build #1214 (2562f37)❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
|
iOS safer-cpp warning is for code I didn't touch in this PR, so going to merge. |
…nto iframes because UIElementsForSearchPredicate only searches within the same process https://bugs.webkit.org/show_bug.cgi?id=307994 rdar://170487304 Reviewed by Joshua Hoffman. This change implements cross-process accessibility search to support VoiceOver navigation across site-isolated iframes. When a search encounters a remote frame, it sends async IPC to the child process to continue the search, then continues its search in its own process. The parent process is also queried via async IPC, since the search may require moving beyond this frame (e.g. asking for the next element at the last element in an iframe). When children + parent processes return results, we merge them back in tree order, resolving AccessibilityRemoteToken results into NSAccessibilityRemoteUIElements as necessary. The search uses a single absolute deadline established when the top-level search begins, rather than giving each nested frame its own independent timeout. When dispatching to child frames, the remaining time until the deadline (minus IPC overhead buffer) is passed along, ensuring deeply nested frame hierarchies share the original timeout budget. This prevents timeout multiplication where N levels of nesting would otherwise allow N × timeout total wait time, while still guaranteeing each frame gets at least a minimum search duration. Other key changes: - Add AXCrossProcessSearch.{h,cpp} with coordination infrastructure for parallel IPC dispatch and cascading timeouts (described above) across nested frames - Refactor AXSearchManager to return AccessibilitySearchResultStream that records both local results and remote frame placeholders in tree order - Add ChromeClient methods for performAccessibilitySearchInRemoteFrame() and continueAccessibilitySearchFromChildFrame() to enable bidirectional search. - Add uiElementsForSearchPredicate() test helper for verifying multi-result searches. - Track accessibility enabled state in WebPageCreationParameters so newly-created web processes for site-isolated frames start with accessibility being enabled. This was required to make my three new tests pass because accessibility must be enabled by the time they got a cross-process accessibility search request. - Searched frames are tracked by ID and will not be searched multiple times. New tests were added to exercise various interesting scenarios: - http/tests/site-isolation/accessibility/cross-process-search-headings.html * Ensures proper merging of local and remote results. - http/tests/site-isolation/accessibility/cross-process-search-nested-iframes.html * Verifies search progresses through multiple layers of iframes. - http/tests/site-isolation/accessibility/cross-process-search-traversal.html * Ensures that we can traverse all the way forwards and back through the all content on the page. * LayoutTests/accessibility/ax-object-destroyed-on-reload-expected.txt: * LayoutTests/accessibility/ax-object-destroyed-on-reload.html: * LayoutTests/http/tests/site-isolation/accessibility/cross-process-search-headings-expected.txt: Added. * LayoutTests/http/tests/site-isolation/accessibility/cross-process-search-headings.html: Added. * LayoutTests/http/tests/site-isolation/accessibility/cross-process-search-nested-iframes-expected.txt: Added. * LayoutTests/http/tests/site-isolation/accessibility/cross-process-search-nested-iframes.html: Added. * LayoutTests/http/tests/site-isolation/accessibility/cross-process-search-traversal-expected.txt: Added. * LayoutTests/http/tests/site-isolation/accessibility/cross-process-search-traversal.html: Added. * LayoutTests/http/tests/site-isolation/accessibility/resources/iframe-with-headings.html: Added. * LayoutTests/http/tests/site-isolation/accessibility/resources/nested-iframe-inner.html: Added. * LayoutTests/http/tests/site-isolation/accessibility/resources/nested-iframe-middle.html: Added. * LayoutTests/platform/ios/accessibility/ax-object-destroyed-on-reload-expected.txt: * Source/WebCore/Headers.cmake: * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/accessibility/AXAnnouncementTypes.h: Added. * Source/WebCore/accessibility/AXCoreObject.cpp: (WebCore::AXCoreObject::partialOrder): (WebCore::AXCoreObject::findMatchingObjectsWithin): * Source/WebCore/accessibility/AXCoreObject.h: * Source/WebCore/accessibility/AXCrossProcessSearch.cpp: Added. (WebCore::canDoRemoteSearch): (WebCore::spinRunLoopUntil): (WebCore::AXCrossProcessSearchCoordinator::waitWithTimeout): (WebCore::mergeStreamResults): (WebCore::computeRemainingTimeout): (WebCore::dispatchRemoteFrameSearch): (WebCore::performCrossProcessSearch): (WebCore::performSearchWithCrossProcessCoordination): (WebCore::mergeParentSearchResults): (WebCore::ParentFrameSearchContext::signal): (WebCore::ParentFrameSearchContext::waitWithTimeout): (WebCore::ParentFrameSearchContext::markParentDispatched): (WebCore::ParentFrameSearchContext::didDispatchParent const): (WebCore::ParentFrameSearchContext::setParentTokens): (WebCore::ParentFrameSearchContext::takeParentTokens): (WebCore::performSearchWithParentCoordination): * Source/WebCore/accessibility/AXCrossProcessSearch.h: Added. (WebCore::AXCrossProcessSearchCoordinator::create): (WebCore::AXCrossProcessSearchCoordinator::addPendingRequest): (WebCore::AXCrossProcessSearchCoordinator::markSearchComplete): (WebCore::AXCrossProcessSearchCoordinator::responseReceived): (WebCore::AXCrossProcessSearchCoordinator::storeRemoteResults): (WebCore::AXCrossProcessSearchCoordinator::takeRemoteResults): (WebCore::AXCrossProcessSearchCoordinator::markFrameAsSearched): (WebCore::AXCrossProcessSearchCoordinator::checkCompletion): * Source/WebCore/accessibility/AXLiveRegionManager.h: (): Deleted. * Source/WebCore/accessibility/AXLogger.cpp: (WebCore::operator<<): (WebCore::streamAXCoreObject): * Source/WebCore/accessibility/AXObjectCache.cpp: (WebCore::AXObjectCache::getOrCreateSlow): * Source/WebCore/accessibility/AXObjectCache.h: (WebCore::AccessibilityRemoteToken::AccessibilityRemoteToken): Deleted. * Source/WebCore/accessibility/AXRemoteFrame.h: * Source/WebCore/accessibility/AXSearchManager.cpp: (WebCore::AXSearchManager::findMatchingObjectsAsStream): (WebCore::AXSearchManager::findMatchingObjectsInternalAsStream): (WebCore::AXSearchManager::findMatchingRange): (WebCore::AXSearchManager::matchWithResultsLimit): Deleted. (WebCore::AXSearchManager::findMatchingObjectsInternal): Deleted. * Source/WebCore/accessibility/AXSearchManager.h: (WebCore::AccessibilitySearchCriteriaIPC::AccessibilitySearchCriteriaIPC): (WebCore::AccessibilitySearchCriteriaIPC::toSearchCriteria const): (WebCore::SearchResultEntry::localResult): (WebCore::SearchResultEntry::remoteFrame): (WebCore::SearchResultEntry::isLocalResult const): (WebCore::SearchResultEntry::isRemoteFrame const): (WebCore::SearchResultEntry::objectIfLocalResult const): (WebCore::SearchResultEntry::frameID const): (WebCore::SearchResultEntry::streamIndex const): (WebCore::SearchResultEntry::SearchResultEntry): (WebCore::AccessibilitySearchResultStream::appendLocalResult): (WebCore::AccessibilitySearchResultStream::appendRemoteFrame): (WebCore::AccessibilitySearchResultStream::entries const): (WebCore::AccessibilitySearchResultStream::entryCount const): (WebCore::AccessibilitySearchResultStream::setResultsLimit): (WebCore::AccessibilitySearchResultStream::resultsLimit const): (WebCore::AccessibilitySearchResultStream::nextIndex const): (WebCore::AccessibilitySearchResult::local): (WebCore::AccessibilitySearchResult::remote): (WebCore::AccessibilitySearchResult::isLocal const): (WebCore::AccessibilitySearchResult::isRemote const): (WebCore::AccessibilitySearchResult::objectIfLocalResult const): (WebCore::AccessibilitySearchResult::remoteToken const): (WebCore::AccessibilitySearchResult::AccessibilitySearchResult): (WebCore::AXSearchManager::findMatchingObjects): Deleted. * Source/WebCore/accessibility/AXStitchUtilities.h: * Source/WebCore/accessibility/AccessibilityMockObject.h: * Source/WebCore/accessibility/AccessibilityNodeObject.cpp: * Source/WebCore/accessibility/AccessibilityObject.cpp: (WebCore::AccessibilityObject::findMatchingObjectsWithin): (WebCore::AccessibilityObject::findMatchingObjects): Deleted. * Source/WebCore/accessibility/AccessibilityObject.h: * Source/WebCore/accessibility/AccessibilityRemoteToken.h: Added. (WebCore::AccessibilityRemoteToken::AccessibilityRemoteToken): * Source/WebCore/accessibility/AccessibilityRenderObject.cpp: * Source/WebCore/accessibility/AccessibilityScrollView.cpp: (WebCore::AccessibilityScrollView::detachRemoteParts): * Source/WebCore/accessibility/AccessibilityScrollView.h: * Source/WebCore/accessibility/AccessibilitySearchCriteriaIPC.h: Added. * Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm: (-[WebAccessibilityObjectWrapper accessibilityFindMatchingObjects:]): * Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp: (WebCore::AXIsolatedObject::findMatchingObjects): Deleted. * Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h: * Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp: (WebCore::AXIsolatedTree::applyPendingChangesLocked): * Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h: * Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm: (WebCore::appendPlatformProperties): * Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm: (remoteElementFromToken): (searchResultsToNSArray): (performSearchWithRemoteFrames): (-[WebAccessibilityObjectWrapper _accessibilityHitTestResolvingRemoteFrame:callback:]): (-[WebAccessibilityObjectWrapper accessibilityAttributeValue:forParameter:]): * Source/WebCore/page/ChromeClient.cpp: (WebCore::ChromeClient::performAccessibilitySearchInRemoteFrame): (WebCore::ChromeClient::continueAccessibilitySearchFromChildFrame): * Source/WebCore/page/ChromeClient.h: * Source/WebCore/page/FrameTree.h: * Source/WebKit/Scripts/webkit/messages.py: (headers_for_type): * Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in: * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::performAccessibilitySearchInRemoteFrame): (WebKit::WebPageProxy::continueAccessibilitySearchFromChildFrame): * Source/WebKit/UIProcess/WebPageProxy.h: * Source/WebKit/UIProcess/WebPageProxy.messages.in: * Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp: (WebKit::WebChromeClient::performAccessibilitySearchInRemoteFrame): (WebKit::WebChromeClient::continueAccessibilitySearchFromChildFrame): * Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h: * Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm: (WebKit::tokenFromWrapper): (WebKit::convertSearchResultsToRemoteTokens): (WebKit::WebPage::performAccessibilitySearchInRemoteFrame): (WebKit::WebPage::continueAccessibilitySearchInParentFrame): * Source/WebKit/WebProcess/WebPage/WebPage.h: * Source/WebKit/WebProcess/WebPage/WebPage.messages.in: * Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp: (WTR::AccessibilityUIElement::uiElementsForSearchPredicate): * Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h: * Tools/WebKitTestRunner/InjectedBundle/Bindings/AccessibilityUIElement.idl: * Tools/WebKitTestRunner/InjectedBundle/atspi/AccessibilityUIElementAtspi.cpp: (WTR::AccessibilityUIElementAtspi::uiElementsForSearchPredicate): * Tools/WebKitTestRunner/InjectedBundle/atspi/AccessibilityUIElementAtspi.h: * Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.h: * Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm: (WTR::AccessibilityUIElementIOS::uiElementsForSearchPredicate): * Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.h: * Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm: (WTR::AccessibilityUIElementMac::attributeValue const): (WTR::AccessibilityUIElementMac::allAttributes): (WTR::AccessibilityUIElementMac::setBoolAttributeValue): (WTR::AccessibilityUIElementMac::setValue): (WTR::AccessibilityUIElementMac::isAttributeSupported): (WTR::AccessibilityUIElementMac::isValid const): (WTR::AccessibilityUIElementMac::uiElementsForSearchPredicate): (WTR::AccessibilityUIElementMac::setSelectedTextRange): (WTR::AccessibilityUIElementMac::invokeCustomActionAtIndex): (WTR::AccessibilityUIElementMac::setSelectedTextMarkerRange): (WTR::AccessibilityUIElementMac::setSelectedChild const): (WTR::AccessibilityUIElementMac::setSelectedChildAtIndex const): (WTR::AccessibilityUIElementMac::removeSelectionAtIndex const): (WTR::AccessibilityUIElementMac::takeFocus): (WTR::AccessibilityUIElementMac::resetSelectedTextMarkerRange): * Tools/WebKitTestRunner/InjectedBundle/playstation/AccessibilityUIElementPlayStation.cpp: (WTR::AccessibilityUIElementPlayStation::uiElementsForSearchPredicate): * Tools/WebKitTestRunner/InjectedBundle/playstation/AccessibilityUIElementPlayStation.h: * Tools/WebKitTestRunner/InjectedBundle/win/AccessibilityUIElementWin.cpp: (WTR::AccessibilityUIElementWin::uiElementsForSearchPredicate): * Tools/WebKitTestRunner/InjectedBundle/win/AccessibilityUIElementWin.h: Canonical link: https://commits.webkit.org/308321@main
2562f37 to
c1e1677
Compare
|
Committed 308321@main (c1e1677): https://commits.webkit.org/308321@main Reviewed commits have been landed. Closing PR #58826 and removing active labels. |
c1e1677
2562f37