Created attachment 387537 [details] Patch

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 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?

Created attachment 387550 [details] Patch

(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.

(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().

(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

(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);.

Created attachment 387658 [details] Patch

In latest patch, added the change to guaranty that AXObjectCache::generateIsolatedTree runs in the main thread.

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

Created attachment 387685 [details] Patch

(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.

The commit-queue encountered the following flaky tests while processing attachment 387685 [details]: The commit-queue is continuing to process your patch.

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 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

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 on attachment 387685 [details] Patch Clearing flags on attachment: 387685 Committed r254566: <https://trac.webkit.org/changeset/254566>

All reviewed patches have been landed. Closing bug.

<rdar://problem/58604083>