Bug 206177 - Implementation of AXIsolatedObject::press().
Summary: Implementation of AXIsolatedObject::press().
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-13 08:46 PST by Andres Gonzalez
Modified: 2020-01-15 06:43 PST (History)
10 users (show)

See Also:


Attachments
Patch (20.85 KB, patch)
2020-01-13 08:58 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (20.67 KB, patch)
2020-01-13 11:46 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (22.64 KB, patch)
2020-01-14 08:13 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (22.63 KB, patch)
2020-01-14 12:04 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-01 for mac-mojave (3.15 MB, application/zip)
2020-01-14 17:03 PST, WebKit Commit Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2020-01-13 08:46:32 PST
Implementation of AXIsolatedObject::press().
Comment 1 Andres Gonzalez 2020-01-13 08:58:43 PST
Created attachment 387537 [details]
Patch
Comment 2 chris fleizach 2020-01-13 09:08:17 PST
Comment on attachment 387537 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387537&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:239
> +        _AXUIElementUseSecondaryAXThread(false);

do we really want to stop using the secondary thread just because a page goes away... this one is global, so I would expect other open pages and new pages should use the other thread

> Source/WebCore/accessibility/AXObjectCache.cpp:849
> +    if (m_pageID) {

is m_pageID a pointer? do we need to check its nullability?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:94
> +    auto optionalTree = treePageCache().take(pageID);

is this optionalTree different from the tree being operated on?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:95
> +    if (optionalTree) {

if (auto optionalTree = treePageCache().take(pageID)) {

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:409
> +    for (const auto& childID : m_childrenIDs)

how are we sure of which thread this will operate on?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:850
> +    if (auto* object = associatedAXObject())

can we assert that this only happens on the main thread

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:1504
>      ASSERT_NOT_REACHED();

is it possible the associated object would ever be nil? in that case the assert not reached shouldn't be there
Comment 3 chris fleizach 2020-01-13 09:10:07 PST
Comment on attachment 387537 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387537&action=review

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:186
> +            axObjectCache()->detachWrapper(object.get(), AccessibilityDetachmentType::ElementDestroyed);

can we cache the axObjectCache() object so we don't call it for every instance in the loop

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:188
> +            object->setObjectID(InvalidAXID);

can these three function calls be put into one method inside the isolated object?
Comment 4 Andres Gonzalez 2020-01-13 11:46:07 PST
Created attachment 387550 [details]
Patch
Comment 5 Andres Gonzalez 2020-01-13 12:48:08 PST
(In reply to chris fleizach from comment #2)
> Comment on attachment 387537 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387537&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:239
> > +        _AXUIElementUseSecondaryAXThread(false);
> 
> do we really want to stop using the secondary thread just because a page
> goes away... this one is global, so I would expect other open pages and new
> pages should use the other thread

When a page goes away, say by activating a link, the current AXObjectCache is destroyed. So when the new one is built, we get the following call stack, that triggers the generation of the new isolated tree and that needs to run in the main thread:

(lldb) bt
* thread #19, name = 'com.apple.accessibility.secondary', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
  * frame #0: 0x00000003b0a0b5de JavaScriptCore`::WTFCrash() at Assertions.cpp:305:35
    frame #1: 0x0000000398009d9b WebCore`WTFCrashWithInfo((null)=765, (null)="./accessibility/AXObjectCache.cpp", (null)="WebCore::AXCoreObject *WebCore::AXObjectCache::isolatedTreeRootObject()", (null)=1132) at Assertions.h:618:5
    frame #2: 0x0000000399fcc0ef WebCore`WebCore::AXObjectCache::isolatedTreeRootObject(this=0x00000003b9edc900) at AXObjectCache.cpp:765:5
    frame #3: 0x0000000399fcbf49 WebCore`WebCore::AXObjectCache::rootObject(this=0x00000003b9edc900) at AXObjectCache.cpp:741:16
    frame #4: 0x0000000110c43ad7 WebKit`::-[WKAccessibilityWebPageObjectBase accessibilityRootObjectWrapper](self=0x00007fd1f6e2f7b0, _cmd="accessibilityRootObjectWrapper") at WKAccessibilityWebPageObjectBase.mm:95:50
    frame #5: 0x0000000110c43d34 WebKit`::-[WKAccessibilityWebPageObjectBase accessibilityFocusedUIElement](self=0x00007fd1f6e2f7b0, _cmd="accessibilityFocusedUIElement") at WKAccessibilityWebPageObjectBase.mm:132:13
    frame #6: 0x0000000110c59925 WebKit`WebKit::NSApplicationAccessibilityFocusedUIElement((null)=0x00007fd1f6d07a10, (null)="accessibilityFocusedUIElement") at WebProcessCocoa.mm:153:12
    frame #7: 0x00007fff2f7037f7 AppKit`NSAccessibilityGetObjectForAttributeUsingLegacyAPI + 371

Perhaps we need to dispatch this call to the main thread instead of turning the AX thread calls off globally?

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:849
> > +    if (m_pageID) {
> 
> is m_pageID a pointer? do we need to check its nullability?

It is an Optional, so we need to check.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:94
> > +    auto optionalTree = treePageCache().take(pageID);
> 
> is this optionalTree different from the tree being operated on?

It is an Optional<Ref<AXIsolatedTree>>, so to access a member you have to do something ugly like (*tree)->treeMember. So I thought it would be more readable to have two separate variables, one for the Optional and another for the Ref.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:95
> > +    if (optionalTree) {
> 
> if (auto optionalTree = treePageCache().take(pageID)) {

Done.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:409
> > +    for (const auto& childID : m_childrenIDs)
> 
> how are we sure of which thread this will operate on?

Added ASSERT(isMainThread()).
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:850
> > +    if (auto* object = associatedAXObject())
> 
> can we assert that this only happens on the main thread

It already does because AXIsolatedObject::axObjectCache asserts. I.e., all calls to associatedAXObject will assert is main thread. Perhaps I should add a comment to make it clearer.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:1504
> >      ASSERT_NOT_REACHED();
> 
> is it possible the associated object would ever be nil? in that case the
> assert not reached shouldn't be there

It is possible, when something goes wrong. So I was considering to add it everywhere that we need the associated object. But we are not there yet.
Comment 6 Andres Gonzalez 2020-01-13 12:51:59 PST
(In reply to chris fleizach from comment #3)
> Comment on attachment 387537 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387537&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:186
> > +            axObjectCache()->detachWrapper(object.get(), AccessibilityDetachmentType::ElementDestroyed);
> 
> can we cache the axObjectCache() object so we don't call it for every
> instance in the loop

It is cached, AXIsolatedTree::axObjectCache is inline { return m_axObjectCache; }.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:188
> > +            object->setObjectID(InvalidAXID);
> 
> can these three function calls be put into one method inside the isolated
> object?

Done, added AXIsolatedObject::defunct().
Comment 7 chris fleizach 2020-01-13 15:31:27 PST
(In reply to Andres Gonzalez from comment #5)
> (In reply to chris fleizach from comment #2)
> > Comment on attachment 387537 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=387537&action=review
> > 
> > > Source/WebCore/accessibility/AXObjectCache.cpp:239
> > > +        _AXUIElementUseSecondaryAXThread(false);
> > 
> > do we really want to stop using the secondary thread just because a page
> > goes away... this one is global, so I would expect other open pages and new
> > pages should use the other thread
> 
> When a page goes away, say by activating a link, the current AXObjectCache
> is destroyed. So when the new one is built, we get the following call stack,
> that triggers the generation of the new isolated tree and that needs to run
> in the main thread:
> 
> (lldb) bt
> * thread #19, name = 'com.apple.accessibility.secondary', stop reason =
> EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
>   * frame #0: 0x00000003b0a0b5de JavaScriptCore`::WTFCrash() at
> Assertions.cpp:305:35
>     frame #1: 0x0000000398009d9b WebCore`WTFCrashWithInfo((null)=765,
> (null)="./accessibility/AXObjectCache.cpp", (null)="WebCore::AXCoreObject
> *WebCore::AXObjectCache::isolatedTreeRootObject()", (null)=1132) at
> Assertions.h:618:5
>     frame #2: 0x0000000399fcc0ef
> WebCore`WebCore::AXObjectCache::
> isolatedTreeRootObject(this=0x00000003b9edc900) at AXObjectCache.cpp:765:5
>     frame #3: 0x0000000399fcbf49
> WebCore`WebCore::AXObjectCache::rootObject(this=0x00000003b9edc900) at
> AXObjectCache.cpp:741:16
>     frame #4: 0x0000000110c43ad7 WebKit`::-[WKAccessibilityWebPageObjectBase
> accessibilityRootObjectWrapper](self=0x00007fd1f6e2f7b0,
> _cmd="accessibilityRootObjectWrapper") at
> WKAccessibilityWebPageObjectBase.mm:95:50
>     frame #5: 0x0000000110c43d34 WebKit`::-[WKAccessibilityWebPageObjectBase
> accessibilityFocusedUIElement](self=0x00007fd1f6e2f7b0,
> _cmd="accessibilityFocusedUIElement") at
> WKAccessibilityWebPageObjectBase.mm:132:13
>     frame #6: 0x0000000110c59925
> WebKit`WebKit::
> NSApplicationAccessibilityFocusedUIElement((null)=0x00007fd1f6d07a10,
> (null)="accessibilityFocusedUIElement") at WebProcessCocoa.mm:153:12
>     frame #7: 0x00007fff2f7037f7
> AppKit`NSAccessibilityGetObjectForAttributeUsingLegacyAPI + 371
> 
> Perhaps we need to dispatch this call to the main thread instead of turning
> the AX thread calls off globally?

Hmm. so we still can service the focused element off the main thread. we just need to know which element ID is focused

However, in this case shouldn't we hit

    if (!m_pageID)
        return nullptr;

and return nil?

if we still have the page_id, we'd presumably hit

    // Should not get here, couldn't create the IsolatedTree.
    ASSERT_NOT_REACHED();

which appears to be a valid case at this time. We're asking for focused element, but there's no page at all, so expected that we return nil
Comment 8 Andres Gonzalez 2020-01-13 17:01:54 PST
(In reply to chris fleizach from comment #7)
> (In reply to Andres Gonzalez from comment #5)
> > (In reply to chris fleizach from comment #2)
> > > Comment on attachment 387537 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=387537&action=review
> > > 
> > > > Source/WebCore/accessibility/AXObjectCache.cpp:239
> > > > +        _AXUIElementUseSecondaryAXThread(false);
> > > 
> > > do we really want to stop using the secondary thread just because a page
> > > goes away... this one is global, so I would expect other open pages and new
> > > pages should use the other thread
> > 
> > When a page goes away, say by activating a link, the current AXObjectCache
> > is destroyed. So when the new one is built, we get the following call stack,
> > that triggers the generation of the new isolated tree and that needs to run
> > in the main thread:
> > 
> > (lldb) bt
> > * thread #19, name = 'com.apple.accessibility.secondary', stop reason =
> > EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
> >   * frame #0: 0x00000003b0a0b5de JavaScriptCore`::WTFCrash() at
> > Assertions.cpp:305:35
> >     frame #1: 0x0000000398009d9b WebCore`WTFCrashWithInfo((null)=765,
> > (null)="./accessibility/AXObjectCache.cpp", (null)="WebCore::AXCoreObject
> > *WebCore::AXObjectCache::isolatedTreeRootObject()", (null)=1132) at
> > Assertions.h:618:5
> >     frame #2: 0x0000000399fcc0ef
> > WebCore`WebCore::AXObjectCache::
> > isolatedTreeRootObject(this=0x00000003b9edc900) at AXObjectCache.cpp:765:5
> >     frame #3: 0x0000000399fcbf49
> > WebCore`WebCore::AXObjectCache::rootObject(this=0x00000003b9edc900) at
> > AXObjectCache.cpp:741:16
> >     frame #4: 0x0000000110c43ad7 WebKit`::-[WKAccessibilityWebPageObjectBase
> > accessibilityRootObjectWrapper](self=0x00007fd1f6e2f7b0,
> > _cmd="accessibilityRootObjectWrapper") at
> > WKAccessibilityWebPageObjectBase.mm:95:50
> >     frame #5: 0x0000000110c43d34 WebKit`::-[WKAccessibilityWebPageObjectBase
> > accessibilityFocusedUIElement](self=0x00007fd1f6e2f7b0,
> > _cmd="accessibilityFocusedUIElement") at
> > WKAccessibilityWebPageObjectBase.mm:132:13
> >     frame #6: 0x0000000110c59925
> > WebKit`WebKit::
> > NSApplicationAccessibilityFocusedUIElement((null)=0x00007fd1f6d07a10,
> > (null)="accessibilityFocusedUIElement") at WebProcessCocoa.mm:153:12
> >     frame #7: 0x00007fff2f7037f7
> > AppKit`NSAccessibilityGetObjectForAttributeUsingLegacyAPI + 371
> > 
> > Perhaps we need to dispatch this call to the main thread instead of turning
> > the AX thread calls off globally?
> 
> Hmm. so we still can service the focused element off the main thread. we
> just need to know which element ID is focused
> 
> However, in this case shouldn't we hit
> 
>     if (!m_pageID)
>         return nullptr;
> 
> and return nil?
> 
> if we still have the page_id, we'd presumably hit
> 
>     // Should not get here, couldn't create the IsolatedTree.
>     ASSERT_NOT_REACHED();
> 
> which appears to be a valid case at this time. We're asking for focused
> element, but there's no page at all, so expected that we return nil

There is already a page at this point. This is the point where we generate the isolated tree the first time. But since this is the second time that the tree needs to be generated, the AX thread flag is already on, so the call comes on the AX thread.

I think the right solution is to dispatch this call to the main thread. I'll make that change in a separate patch/bug and undo the remove the call to _AXUIElementUseSecondaryAXThread(false);.
Comment 9 Andres Gonzalez 2020-01-14 08:13:25 PST
Created attachment 387658 [details]
Patch
Comment 10 Andres Gonzalez 2020-01-14 08:19:50 PST
In latest patch, added the change to guaranty that AXObjectCache::generateIsolatedTree runs in the main thread.
Comment 11 chris fleizach 2020-01-14 08:44:27 PST
Comment on attachment 387658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387658&action=review

> Source/WebCore/ChangeLog:11
> +        - AXIsolatedTree::applyPendingChanges now also properly detach isolated

... also properly detaches ...

> Source/WebCore/accessibility/AXObjectCache.cpp:850
> +    if (m_pageID) {

should we make m_pageID not be optional, so we don't need to check this every time?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:427
> +void AXIsolatedObject::defunct()

can we call this disconnect?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:429
> +    tree()->axObjectCache()->detachWrapper(this, AccessibilityDetachmentType::ElementDestroyed);

should we assert this is main thread?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h:77
> +        // Should always run in the main thread.

comment probably not necessary, since the ASSERT(isMainThread) says the same thing in code
Comment 12 Andres Gonzalez 2020-01-14 12:04:10 PST
Created attachment 387685 [details]
Patch
Comment 13 Andres Gonzalez 2020-01-14 12:10:38 PST
(In reply to chris fleizach from comment #11)
> Comment on attachment 387658 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387658&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        - AXIsolatedTree::applyPendingChanges now also properly detach isolated
> 
> ... also properly detaches ...

Fixed.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:850
> > +    if (m_pageID) {
> 
> should we make m_pageID not be optional, so we don't need to check this
> every time?

I don't think the page ID is guarantied to be not null. we get the page ID from Optional<PageIdentifier> Document::pageID(), so I don't think we can avoid checking whether the page ID is null or not.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:427
> > +void AXIsolatedObject::defunct()
> 
> can we call this disconnect?

Done.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:429
> > +    tree()->axObjectCache()->detachWrapper(this, AccessibilityDetachmentType::ElementDestroyed);
> 
> should we assert this is main thread?

Done.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h:77
> > +        // Should always run in the main thread.
> 
> comment probably not necessary, since the ASSERT(isMainThread) says the same
> thing in code

Fixed. Thanks.
Comment 14 WebKit Commit Bot 2020-01-14 16:46:03 PST
The commit-queue encountered the following flaky tests while processing attachment 387685 [details]:

The commit-queue is continuing to process your patch.
Comment 15 WebKit Commit Bot 2020-01-14 16:46:30 PST
The commit-queue encountered the following flaky tests while processing attachment 387685 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.
Comment 16 WebKit Commit Bot 2020-01-14 17:03:45 PST
Comment on attachment 387685 [details]
Patch

Rejecting attachment 387685 [details] from commit-queue.

New failing tests:
media/track/track-cues-sorted-before-dispatch.html
Full output: https://webkit-queues.webkit.org/results/13304466
Comment 17 WebKit Commit Bot 2020-01-14 17:03:47 PST
Created attachment 387735 [details]
Archive of layout-test-results from webkit-cq-01 for mac-mojave

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-01  Port: mac-mojave  Platform: Mac OS X 10.14.6
Comment 18 WebKit Commit Bot 2020-01-15 06:42:48 PST
Comment on attachment 387685 [details]
Patch

Clearing flags on attachment: 387685

Committed r254566: <https://trac.webkit.org/changeset/254566>
Comment 19 WebKit Commit Bot 2020-01-15 06:42:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2020-01-15 06:43:20 PST
<rdar://problem/58604083>