Bug 247735

Summary: AX: Add logging of the AX trees on a timer for debugging purpose.
Product: WebKit Reporter: Andres Gonzalez <andresg_22>
Component: AccessibilityAssignee: Andres Gonzalez <andresg_22>
Status: NEW ---    
Severity: Normal CC: aboxhall, andresg_22, annulen, apinheiro, cfleizach, dmazzoni, ews-watchlist, gyuyoung.kim, jcraig, jdiggs, ryuan.choi, samuel_white, sergio, tyler_w, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch andresg_22: review?

Description Andres Gonzalez 2022-11-10 08:04:03 PST
Useful to compare live and isolated trees.
Comment 1 Radar WebKit Bug Importer 2022-11-10 08:04:17 PST
<rdar://problem/102194134>
Comment 2 Andres Gonzalez 2022-11-10 08:42:41 PST
Created attachment 463478 [details]
Patch
Comment 3 Tyler Wilcock 2022-11-14 10:38:14 PST
My two concerns with this patch are:

  1. It dumps the isolated tree on the secondary thread (rather than main-thread) unconditionally. This is a problem because I find that most webpages are way too slow to be operable with AXLoggingOptions::OffMainThread enabled. My non-upstreamed timer dump patch forces a tree dump on the main-thread when it has detected an idle state (meaning that the tree dump isn't *strictly* thread-safe, but thread-safe enough for what it needs to do).

  2. It requires something to run AXIsolatedTree::applyPendingChanges to dump the isolated tree, rather than the aforementioned forced tree dump directly based on the timer. My personal workflow is to get pages into a state where they are broken, and then let the forced dump run without any other VO actions to ensure I don't further change the page state or otherwise muddy the logging.

My tree dump patch is obviously not upstreamable, but keeping it around locally[1] and having to rebase it every now and then is not a big deal for the utility it provides.

[1]: I keep it as a git stash, and git stash apply as needed
Comment 4 Andres Gonzalez 2022-11-15 05:37:35 PST
(In reply to Tyler Wilcock from comment #3)
> My two concerns with this patch are:
> 
>   1. It dumps the isolated tree on the secondary thread (rather than
> main-thread) unconditionally. 

That's where the iso tree should be accessed, should not be accessed on the main thread.

> This is a problem because I find that most
> webpages are way too slow to be operable with
> AXLoggingOptions::OffMainThread enabled. 

We should address that by providing finer granularity for what to log, i.e., don't log object wrapper stuff, not just on/off main thread.

> My non-upstreamed timer dump patch
> forces a tree dump on the main-thread when it has detected an idle state
> (meaning that the tree dump isn't *strictly* thread-safe, but thread-safe
> enough for what it needs to do).

We can add that hack to this but it is just an illusion that we can detect any kind of idleness in the iso tree. An actual solution here could be to dispatch the dump request to the secondary thread.

> 
>   2. It requires something to run AXIsolatedTree::applyPendingChanges to
> dump the isolated tree, rather than the aforementioned forced tree dump
> directly based on the timer. 

We care about the state of the iso tree after applying pending changes, so that's where we want to dump the tree.

> My personal workflow is to get pages into a
> state where they are broken, and then let the forced dump run without any
> other VO actions to ensure I don't further change the page state or
> otherwise muddy the logging.

We can make that work with the right approach of dispatching the dump to the secondary thread. This is a step in that direction.