Bug 204226 - Run AccessibilityController::rootElement on secondary thread to simulate HIServices during LayoutTests.
Summary: Run AccessibilityController::rootElement on secondary thread to simulate HISe...
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: 2019-11-15 08:32 PST by Andres Gonzalez
Modified: 2019-11-18 17:13 PST (History)
10 users (show)

See Also:


Attachments
Patch (10.03 KB, patch)
2019-11-15 08:44 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (14.18 MB, application/zip)
2019-11-15 13:51 PST, EWS Watchlist
no flags Details
Patch (10.01 KB, patch)
2019-11-15 14:47 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (14.79 KB, patch)
2019-11-16 11:34 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (15.53 KB, patch)
2019-11-16 12:06 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (15.65 KB, patch)
2019-11-18 13:25 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2019-11-15 08:32:34 PST
Run AccessibilityController::rootElement on secondary thread to simulate HIServices during LayoutTests.
Comment 1 Andres Gonzalez 2019-11-15 08:44:29 PST
Created attachment 383623 [details]
Patch
Comment 2 chris fleizach 2019-11-15 11:42:58 PST
Comment on attachment 383623 [details]
Patch

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

> Tools/ChangeLog:17
> +        served in the main thread in order to build the AXIsolatedTree.

How do we enable the secondary thread mode?

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:79
> +    void* root = nullptr;

is this really a void* still? can we change type to PlatformUIElement?

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:82
> +        dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);

it looks like there a WTF::BinarySemaphore that might be more platform independent

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:95
> +        m_useAXThread = true;

we don't want to set this for all requests do we?
Comment 3 EWS Watchlist 2019-11-15 13:51:03 PST
Comment on attachment 383623 [details]
Patch

Attachment 383623 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13258982

New failing tests:
animations/no-style-recalc-during-accelerated-animation.html
Comment 4 EWS Watchlist 2019-11-15 13:51:06 PST
Created attachment 383642 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 5 Andres Gonzalez 2019-11-15 14:45:26 PST
(In reply to chris fleizach from comment #2)
> Comment on attachment 383623 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383623&action=review
> 
> > Tools/ChangeLog:17
> > +        served in the main thread in order to build the AXIsolatedTree.
> 
> How do we enable the secondary thread mode?

The first time it runs, it will always be main thread. the second time and on, it would be secondary thread.
> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:79
> > +    void* root = nullptr;
> 
> is this really a void* still? can we change type to PlatformUIElement?

Done.
> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:82
> > +        dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
> 
> it looks like there a WTF::BinarySemaphore that might be more platform
> independent

Done.
> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:95
> > +        m_useAXThread = true;
> 
> we don't want to set this for all requests do we?

It is set to true after the first request is served in the main thread and the isolated tree is generated, so that all subsequent requests are served in the secondary thread.
Comment 6 Andres Gonzalez 2019-11-15 14:47:40 PST
Created attachment 383653 [details]
Patch
Comment 7 chris fleizach 2019-11-15 21:33:10 PST
Comment on attachment 383653 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:89
> +        root = static_cast<PlatformUIElement>(WKAccessibilityRootObject(page));

I think we'll need a way to distinguish from the test (or some other process, since ultimately we want to run every test in this mode and regular mode).

we won't be able to call into regular AccessibilityObject off the main thread (lots of main thread asserts)
Comment 8 Andres Gonzalez 2019-11-16 11:34:12 PST
Created attachment 383701 [details]
Patch
Comment 9 Andres Gonzalez 2019-11-16 11:41:29 PST
Added canUseSecondaryAXThread. It will return true when isolated tree is enabled and the client supports the isolated tree. Accessibility controller checks for this condition in order to spawn the secondary thread.
Comment 10 Andres Gonzalez 2019-11-16 12:06:03 PST
Created attachment 383702 [details]
Patch
Comment 11 chris fleizach 2019-11-18 12:21:57 PST
Comment on attachment 383702 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:717
> +    return clientSupportsIsolatedTree();

will clientSupportsIsolatedTree be true when running tests?

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:92
> +        m_useAXThread = true;

seems like we should only set this true (m_useAXThread) if WKAccessibilityCanUseSecondaryAXThread(page) is true
Comment 12 Andres Gonzalez 2019-11-18 13:25:29 PST
Created attachment 383782 [details]
Patch
Comment 13 Andres Gonzalez 2019-11-18 13:31:45 PST
(In reply to chris fleizach from comment #11)
> Comment on attachment 383702 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383702&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:717
> > +    return clientSupportsIsolatedTree();
> 
> will clientSupportsIsolatedTree be true when running tests?

Yes, the AXClientType during LayoutTests is kAXClientTypeNoActiveRequestFound. So the assumption is no client request found, it is LayoutTests.

> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:92
> > +        m_useAXThread = true;
> 
> seems like we should only set this true (m_useAXThread) if
> WKAccessibilityCanUseSecondaryAXThread(page) is true

Yes, fixed it. thanks.
Comment 14 chris fleizach 2019-11-18 15:14:00 PST
Comment on attachment 383782 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:92
> +            // Set m_useAXThread to true for next request.

if you put this comment above the if, then you can get rid of the braces
Comment 15 WebKit Commit Bot 2019-11-18 17:12:07 PST
Comment on attachment 383782 [details]
Patch

Clearing flags on attachment: 383782

Committed r252609: <https://trac.webkit.org/changeset/252609>
Comment 16 WebKit Commit Bot 2019-11-18 17:12:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2019-11-18 17:13:22 PST
<rdar://problem/57305440>