Bug 230257

Created attachment 440592 [details] Patch

The API test failures must be unrelated, this patch only affects a11y and when building with atspi enabled, which is disabled by default.

(In reply to Carlos Garcia Campos from comment #1) > Created attachment 440592 [details] > Patch --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h + RefPtr<AXIsolatedObject> nodeForID(AXID) const; nodeForID belongs to the tree not to the isolated object. I think it is a better design to keep it in AXIsolatedTree. Do you have a reason to move it to AXIsolatedObject?

(In reply to Andres Gonzalez from comment #3) > (In reply to Carlos Garcia Campos from comment #1) > > Created attachment 440592 [details] > > Patch > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > + RefPtr<AXIsolatedObject> nodeForID(AXID) const; > > nodeForID belongs to the tree not to the isolated object. I think it is a > better design to keep it in AXIsolatedTree. Do you have a reason to move it > to AXIsolatedObject? Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR actually. Since WTR calls accessibility api from the web process main thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's always expected to be called from the secondary thread (nodeForID only works from the a11y thread). I can't use callOnAXthreadAndWait, because first thing AXIsolatedObject::accessibilityHitTest() is retrieveValueFromMainThread() but main thread is blocked. So, I split it adding AccessibilityObjectAtspi::objectAtPoint() that is always called by the main thread and does the coordinate conversion + accessibilityHitTest(). When called from the main thread (WTR only) it's called directly and the wrapper object returned. When called from the AX thread I need the nodeForID() call to get the wrapper from the isolated object. I agree nodeForID doesn't belong to AXIsolatedObject, so the other solution would be to just make AXIsolatedObject::tree() public.

(In reply to Carlos Garcia Campos from comment #4) > (In reply to Andres Gonzalez from comment #3) > > (In reply to Carlos Garcia Campos from comment #1) > > > Created attachment 440592 [details] > > > Patch > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > > > + RefPtr<AXIsolatedObject> nodeForID(AXID) const; > > > > nodeForID belongs to the tree not to the isolated object. I think it is a > > better design to keep it in AXIsolatedTree. Do you have a reason to move it > > to AXIsolatedObject? > > Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR > actually. Since WTR calls accessibility api from the web process main > thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's > always expected to be called from the secondary thread (nodeForID only works > from the a11y thread). I can't use callOnAXthreadAndWait, because first > thing AXIsolatedObject::accessibilityHitTest() is > retrieveValueFromMainThread() but main thread is blocked. So, I split it > adding AccessibilityObjectAtspi::objectAtPoint() that is always called by > the main thread and does the coordinate conversion + accessibilityHitTest(). > When called from the main thread (WTR only) it's called directly and the > wrapper object returned. When called from the AX thread I need the > nodeForID() call to get the wrapper from the isolated object. Can't you get the wrapper from the isolated object? AXCoreObject::wrapper() should work in all objects, isolated or live.

(In reply to Andres Gonzalez from comment #5) > (In reply to Carlos Garcia Campos from comment #4) > > (In reply to Andres Gonzalez from comment #3) > > > (In reply to Carlos Garcia Campos from comment #1) > > > > Created attachment 440592 [details] > > > > Patch > > > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > > > > > + RefPtr<AXIsolatedObject> nodeForID(AXID) const; > > > > > > nodeForID belongs to the tree not to the isolated object. I think it is a > > > better design to keep it in AXIsolatedTree. Do you have a reason to move it > > > to AXIsolatedObject? > > > > Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR > > actually. Since WTR calls accessibility api from the web process main > > thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's > > always expected to be called from the secondary thread (nodeForID only works > > from the a11y thread). I can't use callOnAXthreadAndWait, because first > > thing AXIsolatedObject::accessibilityHitTest() is > > retrieveValueFromMainThread() but main thread is blocked. So, I split it > > adding AccessibilityObjectAtspi::objectAtPoint() that is always called by > > the main thread and does the coordinate conversion + accessibilityHitTest(). > > When called from the main thread (WTR only) it's called directly and the > > wrapper object returned. When called from the AX thread I need the > > nodeForID() call to get the wrapper from the isolated object. > > Can't you get the wrapper from the isolated object? AXCoreObject::wrapper() > should work in all objects, isolated or live. hmm, I'm not sure it's ok to get the wrapper from the main thread, I assumed that AXIsolatedObject::accessibilityHitTest() returned the AXID because it's safe to send the id from main to AX thread, but not the wrapper?

(In reply to Carlos Garcia Campos from comment #6) > (In reply to Andres Gonzalez from comment #5) > > (In reply to Carlos Garcia Campos from comment #4) > > > (In reply to Andres Gonzalez from comment #3) > > > > (In reply to Carlos Garcia Campos from comment #1) > > > > > Created attachment 440592 [details] > > > > > Patch > > > > > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > > > > > > > + RefPtr<AXIsolatedObject> nodeForID(AXID) const; > > > > > > > > nodeForID belongs to the tree not to the isolated object. I think it is a > > > > better design to keep it in AXIsolatedTree. Do you have a reason to move it > > > > to AXIsolatedObject? > > > > > > Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR > > > actually. Since WTR calls accessibility api from the web process main > > > thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's > > > always expected to be called from the secondary thread (nodeForID only works > > > from the a11y thread). I can't use callOnAXthreadAndWait, because first > > > thing AXIsolatedObject::accessibilityHitTest() is > > > retrieveValueFromMainThread() but main thread is blocked. So, I split it > > > adding AccessibilityObjectAtspi::objectAtPoint() that is always called by > > > the main thread and does the coordinate conversion + accessibilityHitTest(). > > > When called from the main thread (WTR only) it's called directly and the > > > wrapper object returned. When called from the AX thread I need the > > > nodeForID() call to get the wrapper from the isolated object. > > > > Can't you get the wrapper from the isolated object? AXCoreObject::wrapper() > > should work in all objects, isolated or live. > > hmm, I'm not sure it's ok to get the wrapper from the main thread, I assumed > that AXIsolatedObject::accessibilityHitTest() returned the AXID because it's > safe to send the id from main to AX thread, but not the wrapper? Let's go back for a moment to the origin of the problem. As you said, you couldn't use AccessibilityController::executeOnAXThreadAndWait for HitTest because HitTest will dispatch back to the main thread that it would be waiting, resulting in deadlock. The way we worked around this was by: AccessibilityController::executeOnAXThreadAndWait calls AXThread::dispatch that in turn calls axThread.wakeUpRunLoop On Mac wakeUpRunLoop does void AXThread::wakeUpRunLoop() { CFRunLoopSourceSignal(m_threadRunLoopSource.get()); CFRunLoopWakeUp(m_threadRunLoop.get()); } But it is a no-op in GTK. Could we try something equivalent in GTK?

(In reply to Andres Gonzalez from comment #7) > (In reply to Carlos Garcia Campos from comment #6) > > (In reply to Andres Gonzalez from comment #5) > > > (In reply to Carlos Garcia Campos from comment #4) > > > > (In reply to Andres Gonzalez from comment #3) > > > > > (In reply to Carlos Garcia Campos from comment #1) > > > > > > Created attachment 440592 [details] > > > > > > Patch > > > > > > > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > > > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > > > > > > > > > + RefPtr<AXIsolatedObject> nodeForID(AXID) const; > > > > > > > > > > nodeForID belongs to the tree not to the isolated object. I think it is a > > > > > better design to keep it in AXIsolatedTree. Do you have a reason to move it > > > > > to AXIsolatedObject? > > > > > > > > Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR > > > > actually. Since WTR calls accessibility api from the web process main > > > > thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's > > > > always expected to be called from the secondary thread (nodeForID only works > > > > from the a11y thread). I can't use callOnAXthreadAndWait, because first > > > > thing AXIsolatedObject::accessibilityHitTest() is > > > > retrieveValueFromMainThread() but main thread is blocked. So, I split it > > > > adding AccessibilityObjectAtspi::objectAtPoint() that is always called by > > > > the main thread and does the coordinate conversion + accessibilityHitTest(). > > > > When called from the main thread (WTR only) it's called directly and the > > > > wrapper object returned. When called from the AX thread I need the > > > > nodeForID() call to get the wrapper from the isolated object. > > > > > > Can't you get the wrapper from the isolated object? AXCoreObject::wrapper() > > > should work in all objects, isolated or live. > > > > hmm, I'm not sure it's ok to get the wrapper from the main thread, I assumed > > that AXIsolatedObject::accessibilityHitTest() returned the AXID because it's > > safe to send the id from main to AX thread, but not the wrapper? > > Let's go back for a moment to the origin of the problem. As you said, you > couldn't use AccessibilityController::executeOnAXThreadAndWait for HitTest > because HitTest will dispatch back to the main thread that it would be > waiting, resulting in deadlock. The way we worked around this was by: > > AccessibilityController::executeOnAXThreadAndWait calls > AXThread::dispatch that in turn calls > axThread.wakeUpRunLoop > > On Mac wakeUpRunLoop does > > void AXThread::wakeUpRunLoop() > { > CFRunLoopSourceSignal(m_threadRunLoopSource.get()); > CFRunLoopWakeUp(m_threadRunLoop.get()); > } > > But it is a no-op in GTK. Could we try something equivalent in GTK? The issue is not unique to HitTest, but you would have the same problem for any call into the API that needs to dispatch to the main thread. So we need to find a general solution.

(In reply to Andres Gonzalez from comment #7) > (In reply to Carlos Garcia Campos from comment #6) > > (In reply to Andres Gonzalez from comment #5) > > > (In reply to Carlos Garcia Campos from comment #4) > > > > (In reply to Andres Gonzalez from comment #3) > > > > > (In reply to Carlos Garcia Campos from comment #1) > > > > > > Created attachment 440592 [details] > > > > > > Patch > > > > > > > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > > > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > > > > > > > > > + RefPtr<AXIsolatedObject> nodeForID(AXID) const; > > > > > > > > > > nodeForID belongs to the tree not to the isolated object. I think it is a > > > > > better design to keep it in AXIsolatedTree. Do you have a reason to move it > > > > > to AXIsolatedObject? > > > > > > > > Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR > > > > actually. Since WTR calls accessibility api from the web process main > > > > thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's > > > > always expected to be called from the secondary thread (nodeForID only works > > > > from the a11y thread). I can't use callOnAXthreadAndWait, because first > > > > thing AXIsolatedObject::accessibilityHitTest() is > > > > retrieveValueFromMainThread() but main thread is blocked. So, I split it > > > > adding AccessibilityObjectAtspi::objectAtPoint() that is always called by > > > > the main thread and does the coordinate conversion + accessibilityHitTest(). > > > > When called from the main thread (WTR only) it's called directly and the > > > > wrapper object returned. When called from the AX thread I need the > > > > nodeForID() call to get the wrapper from the isolated object. > > > > > > Can't you get the wrapper from the isolated object? AXCoreObject::wrapper() > > > should work in all objects, isolated or live. > > > > hmm, I'm not sure it's ok to get the wrapper from the main thread, I assumed > > that AXIsolatedObject::accessibilityHitTest() returned the AXID because it's > > safe to send the id from main to AX thread, but not the wrapper? > > Let's go back for a moment to the origin of the problem. As you said, you > couldn't use AccessibilityController::executeOnAXThreadAndWait for HitTest > because HitTest will dispatch back to the main thread that it would be > waiting, resulting in deadlock. The way we worked around this was by: > > AccessibilityController::executeOnAXThreadAndWait calls > AXThread::dispatch that in turn calls > axThread.wakeUpRunLoop > > On Mac wakeUpRunLoop does > > void AXThread::wakeUpRunLoop() > { > CFRunLoopSourceSignal(m_threadRunLoopSource.get()); > CFRunLoopWakeUp(m_threadRunLoop.get()); > } > > But it is a no-op in GTK. Could we try something equivalent in GTK? hmm, I don't understand how waking up the AX thread helps.

The AX thread run loop I meant.

(In reply to Andres Gonzalez from comment #8) > (In reply to Andres Gonzalez from comment #7) > > (In reply to Carlos Garcia Campos from comment #6) > > > (In reply to Andres Gonzalez from comment #5) > > > > (In reply to Carlos Garcia Campos from comment #4) > > > > > (In reply to Andres Gonzalez from comment #3) > > > > > > (In reply to Carlos Garcia Campos from comment #1) > > > > > > > Created attachment 440592 [details] > > > > > > > Patch > > > > > > > > > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > > > > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h > > > > > > > > > > > > + RefPtr<AXIsolatedObject> nodeForID(AXID) const; > > > > > > > > > > > > nodeForID belongs to the tree not to the isolated object. I think it is a > > > > > > better design to keep it in AXIsolatedTree. Do you have a reason to move it > > > > > > to AXIsolatedObject? > > > > > > > > > > Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR > > > > > actually. Since WTR calls accessibility api from the web process main > > > > > thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's > > > > > always expected to be called from the secondary thread (nodeForID only works > > > > > from the a11y thread). I can't use callOnAXthreadAndWait, because first > > > > > thing AXIsolatedObject::accessibilityHitTest() is > > > > > retrieveValueFromMainThread() but main thread is blocked. So, I split it > > > > > adding AccessibilityObjectAtspi::objectAtPoint() that is always called by > > > > > the main thread and does the coordinate conversion + accessibilityHitTest(). > > > > > When called from the main thread (WTR only) it's called directly and the > > > > > wrapper object returned. When called from the AX thread I need the > > > > > nodeForID() call to get the wrapper from the isolated object. > > > > > > > > Can't you get the wrapper from the isolated object? AXCoreObject::wrapper() > > > > should work in all objects, isolated or live. > > > > > > hmm, I'm not sure it's ok to get the wrapper from the main thread, I assumed > > > that AXIsolatedObject::accessibilityHitTest() returned the AXID because it's > > > safe to send the id from main to AX thread, but not the wrapper? > > > > Let's go back for a moment to the origin of the problem. As you said, you > > couldn't use AccessibilityController::executeOnAXThreadAndWait for HitTest > > because HitTest will dispatch back to the main thread that it would be > > waiting, resulting in deadlock. The way we worked around this was by: > > > > AccessibilityController::executeOnAXThreadAndWait calls > > AXThread::dispatch that in turn calls > > axThread.wakeUpRunLoop > > > > On Mac wakeUpRunLoop does > > > > void AXThread::wakeUpRunLoop() > > { > > CFRunLoopSourceSignal(m_threadRunLoopSource.get()); > > CFRunLoopWakeUp(m_threadRunLoop.get()); > > } > > > > But it is a no-op in GTK. Could we try something equivalent in GTK? > > The issue is not unique to HitTest, but you would have the same problem for > any call into the API that needs to dispatch to the main thread. So we need > to find a general solution. I managed to handle all other cases by just splitting the methods, a common one that always runs in the main thread (or simply calls retrieveValueFromMainThread) and the one that is always expected to be called from the AX thread. For requests coming from the ATSPI service I use the method that is expected to be called from the AX thread and calls the common one. From WTR I call the common one directly from the main thread, since retrieveValueFromMainThread just calls the lambda if already in the main thread. That's the advantage of using the WebCore wrappers directly from WTR, without any a11y toolkit in the middle. There are a few cases like children() that I always run from the AX thread in WTR because those don't schedule a main thread task. I have the WTR changes in a local commit that I plan to land after landing the text interface support, that's why you still see stubs for everything when building with atspi.

Created attachment 441055 [details] Patch Removed the cross-platform changes in the end.

(In reply to Carlos Garcia Campos from comment #12) > Created attachment 441055 [details] > Patch > > Removed the cross-platform changes in the end. Sorry that is taking me long to get back. So you are not doing layout tests for HitTest? I have to look at the question concerning the runloop wakeup. The intention at least was to wakeup the main thread to avoid the deadlock, but there may be a bug. Concerning LayoutTests calling different functions than the actual AT client, we would like to avoid that as a way to go in the backdoor. I think it would be good that the LayoutTests JS uses the same entry points into accessibility as a real client would.

(In reply to Andres Gonzalez from comment #13) > (In reply to Carlos Garcia Campos from comment #12) > > Created attachment 441055 [details] > > Patch > > > > Removed the cross-platform changes in the end. > > Sorry that is taking me long to get back. No problem! > So you are not doing layout tests > for HitTest? Yes, but the WTR support is not upstream yet, and ATSPI implementation is disabled by default because it's still work in progress, so no bots are running tests with ATSPI. > I have to look at the question concerning the runloop wakeup. The intention > at least was to wakeup the main thread to avoid the deadlock, but there may > be a bug. But the main loop is blocked in a semaphore, or weaking up the main thread loop in the AX thread makes the code scheduled for the main thread to run in the AX thread? > Concerning LayoutTests calling different functions than the actual AT > client, we would like to avoid that as a way to go in the backdoor. I think > it would be good that the LayoutTests JS uses the same entry points into > accessibility as a real client would. They will end up running the same code. In any case, I think it will be easier to discuss the WTR implementation once I submit the patch for it. I plan to upstream the text implementation after the component one, and then add the initial WTR support.

Committed r284367 (243152@main): <https://commits.webkit.org/243152@main>