Bug 203408

Summary: Create base class common to AccessibilityObject and AXIsolatedTreeNode.
Product: WebKit Reporter: Andres Gonzalez <andresg_22>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, commit-queue, dmazzoni, eric.carlson, ews-feeder, ews-watchlist, glenn, hi, jcraig, jdiggs, jer.noble, joepeck, mifenton, philipj, rniwa, samuel_white, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Created AccessibilityObjectBase, a common base class for both AccessibilityObject and AXIsolatedTreeNode.
none
Addresses comment in previous patch.
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Andres Gonzalez 2019-10-25 06:51:00 PDT
Create base class common to AccessibilityObject and AXIsolatedTreeNode.
Comment 1 Andres Gonzalez 2019-10-25 08:22:58 PDT
Created attachment 381926 [details]
Created AccessibilityObjectBase, a common base class for both AccessibilityObject and AXIsolatedTreeNode.
Comment 2 chris fleizach 2019-10-25 09:25:45 PDT
Comment on attachment 381926 [details]
Created AccessibilityObjectBase, a common base class for both AccessibilityObject and AXIsolatedTreeNode.

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

> Source/WebCore/accessibility/AXObjectCache.cpp:336
>      for (const auto& child : axRenderImage->children()) {

the method signature

> Source/WebCore/accessibility/AXObjectCache.cpp:2973
> +    tree->setRoot(root);

why do we want to store the actual root instead of the rootID?

> Source/WebCore/accessibility/AccessibilityTableRow.cpp:147
> +    return downcast<AccessibilityTableCell>(cell);

this downcast seems unnecessary since we're returning a AccessibilityObjectBase

> Source/WebCore/accessibility/ios/AXObjectCacheIOS.mm:44
> +    return wrapper;

if we're detaching a wrapper I don't think we should be returning it, because it's essentially deallocated when it comes back after this method

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:121
> +    m_readerThreadNodeMap.add(root->objectID(), WTFMove(root));

I'm concerned about moving the root into the reader thread here. I think that's already supposed to have happened
Comment 3 chris fleizach 2019-10-25 09:34:41 PDT
So I think there are too many different kinds of changes rolled up in here

1) rename AXObjectInterface > AXObjectBase
2) Adopt all AXObjectBase methods in AXIsolatedTree
3) move virtual > override
4) rename matchedParent > ancestorMatch and rewrite function
   - I don't think we should add an "ax" namespace because ax isn't well known
   - we should probably have a method in WebCore namespace that's like "accessibilityAncestorMatched"
5) Lots of little cleanup for auto or -> to . syntax
6) rewrite detachWrapper
7) storing root node instead of the root node id
8) rename axObjectID -> objectID()

I'm also worried about having two AXObjectWrappers now. It will double the number of ObjC classes initialized, but I'm also concerned that there are some cases where we will reference once wrapper in once place and then another wrapper in another place and things won't work correctly.

-------

So I think we should try to break up this patch into smaller patches that separates what each patch does. It will help with reviewing and make it more clear why this change was associated with a specific bug
Comment 4 Andres Gonzalez 2019-10-25 14:55:35 PDT
(In reply to chris fleizach from comment #2)
> Comment on attachment 381926 [details]
> Created AccessibilityObjectBase, a common base class for both
> AccessibilityObject and AXIsolatedTreeNode.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381926&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:336
> >      for (const auto& child : axRenderImage->children()) {
> 
> the method signature

It is fine to return an AccessibilityObject here, no need to return the base class. Although we could change the method signature to return an AccessibilityImageMapLink. is that what you mean?

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:2973
> > +    tree->setRoot(root);
> 
> why do we want to store the actual root instead of the rootID?

The root needs to be set during the creation of the tree because:

- (id)isolatedTreeRootObject
{
...
            auto tree = cache->generateIsolatedAccessibilityTree();
...
            if (auto rootNode = tree->rootNode())
                return rootNode->wrapper();
...
    return nil;
}

and tree->rootNode() will be nil because it won't find any element in the HashMap for the root ID. If this returns nil, everything else fails.

> 
> > Source/WebCore/accessibility/AccessibilityTableRow.cpp:147
> > +    return downcast<AccessibilityTableCell>(cell);
> 
> this downcast seems unnecessary since we're returning a
> AccessibilityObjectBase

Yes, fixed.

> 
> > Source/WebCore/accessibility/ios/AXObjectCacheIOS.mm:44
> > +    return wrapper;
> 
> if we're detaching a wrapper I don't think we should be returning it,
> because it's essentially deallocated when it comes back after this method

Not if the caller assigns it to a variable, strong reference. I did this to allow for swapping the underlying accessible object. For instance:

AXIsolatedTreeNode::f() {
    // cannot fulfill request, have to go back to main thread.
    AccessibilityObject* axObject = get corresponding live object from the ID;
    auto* wrapper = cache->detachWrapper(*this);
    axObject->attachWrapper(wrapper);
    // send to main thread axObject->f()
}

But I'm not using it yet in this patch. So I could revert that if you wish.

> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:121
> > +    m_readerThreadNodeMap.add(root->objectID(), WTFMove(root));
> 
> I'm concerned about moving the root into the reader thread here. I think
> that's already supposed to have happened

It hasn't happened on generating the tree when the root needs to be set. So that's why we now do setRoot instead of just setRootID, as explained above.
Comment 5 Andres Gonzalez 2019-10-25 15:08:30 PDT
Created attachment 381969 [details]
Addresses comment in previous patch.
Comment 6 Andres Gonzalez 2019-10-28 18:03:07 PDT
Created attachment 382144 [details]
Patch
Comment 7 Andres Gonzalez 2019-10-28 18:08:09 PDT
This new patch is a subset of the previous patch with only the changes related to making AXCoreObject a common base class for AccessibilityObject and AXIsolatedTreeNode. Builds and passes all tests in both MacOS and iOS. No changes in AXIsolatedTreeNode other than the strictly necessary to build.
Comment 8 Andres Gonzalez 2019-10-30 05:05:30 PDT
Created attachment 382296 [details]
Patch
Comment 9 Andres Gonzalez 2019-10-30 05:35:46 PDT
Created attachment 382297 [details]
Patch
Comment 10 Andres Gonzalez 2019-10-30 05:49:48 PDT
Created attachment 382298 [details]
Patch
Comment 11 Andres Gonzalez 2019-10-30 06:30:04 PDT
Created attachment 382302 [details]
Patch
Comment 12 Andres Gonzalez 2019-10-30 07:38:26 PDT
Created attachment 382310 [details]
Patch
Comment 13 Andres Gonzalez 2019-10-30 09:34:24 PDT
Created attachment 382323 [details]
Patch
Comment 14 EWS 2019-10-30 12:38:47 PDT
Comment on attachment 382323 [details]
Patch

Rejecting attachment 382323 [details] from commit-queue.

andresg_22@apple.com does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 15 WebKit Commit Bot 2019-10-30 13:44:43 PDT
Comment on attachment 382323 [details]
Patch

Clearing flags on attachment: 382323

Committed r251798: <https://trac.webkit.org/changeset/251798>
Comment 16 WebKit Commit Bot 2019-10-30 13:44:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2019-10-30 13:45:18 PDT
<rdar://problem/56756913>
Comment 18 Ryosuke Niwa 2019-10-31 00:56:23 PDT
This patch appears to have broken Windows builds:
e.g.

https://build.webkit.org/builders/Apple%20Win%2010%20Debug%20%28Build%29/builds/7858/steps/compile-webkit/logs/stdio

  JSInternals.cpp
c:\cygwin\home\buildbot\worker\win10-debug\build\source\webkitlegacy\win\accessiblebase.cpp(361): error C2220: warning treated as error - no 'object' file generated [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\WebKitLegacy\WebKitLegacy.vcxproj]
c:\cygwin\home\buildbot\worker\win10-debug\build\source\webkitlegacy\win\accessiblebase.cpp(361): warning C4541: 'dynamic_cast' used on polymorphic type 'WebCore::AXCoreObject' with /GR-; unpredictable behavior may result [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\WebKitLegacy\WebKitLegacy.vcxproj]
c:\cygwin\home\buildbot\worker\win10-debug\build\source\webkitlegacy\win\accessiblebase.cpp(1048): warning C4541: 'dynamic_cast' used on polymorphic type 'WebCore::AXCoreObject' with /GR-; unpredictable behavior may result [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\WebKitLegacy\WebKitLegacy.vcxproj]
  WebDesktopNotificationsDelegate.cpp

We disable RTTI so I'm not certain dynamic_cast even works in WebKit...
Comment 19 Ryosuke Niwa 2019-10-31 00:59:58 PDT
Also, please be careful with isolated tree work. Most of objects like Node, VisiblePosition, etc... are NOT thread safe. Simply storing a Ref or RefPtr on them in non-main thread would corrupt them.
Comment 20 Ryosuke Niwa 2019-10-31 01:06:11 PDT
Build fix attempt landed in https://trac.webkit.org/changeset/251835.
Comment 21 chris fleizach 2019-10-31 01:15:29 PDT
(In reply to Ryosuke Niwa from comment #19)
> Also, please be careful with isolated tree work. Most of objects like Node,
> VisiblePosition, etc... are NOT thread safe. Simply storing a Ref or RefPtr
> on them in non-main thread would corrupt them.

Yep we're very aware of this limitation