Bug 203408 – Create base class common to AccessibilityObject and AXIsolatedTreeNode.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andres Gonzalez]

Create base class common to AccessibilityObject and AXIsolatedTreeNode.

[Andres Gonzalez]

Created attachment 381926 [details]
Created AccessibilityObjectBase, a common base class for both AccessibilityObject and AXIsolatedTreeNode.

[chris fleizach]

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

[chris fleizach]

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

[Andres Gonzalez]

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

[Andres Gonzalez]

Created attachment 381969 [details]
Addresses comment in previous patch.

[Andres Gonzalez]

Created attachment 382144 [details]
Patch

[Andres Gonzalez]

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.

[Andres Gonzalez]

Created attachment 382296 [details]
Patch

[Andres Gonzalez]

Created attachment 382297 [details]
Patch

[Andres Gonzalez]

Created attachment 382298 [details]
Patch

[Andres Gonzalez]

Created attachment 382302 [details]
Patch

[Andres Gonzalez]

Created attachment 382310 [details]
Patch

[Andres Gonzalez]

Created attachment 382323 [details]
Patch

[EWS]

Comment on attachment 382323 [details]
Patch

Rejecting attachment 382323 [details] from commit-queue.

[e-mail] 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.

[WebKit Commit Bot]

Comment on attachment 382323 [details]
Patch

Clearing flags on attachment: 382323

Committed r251798: <https://trac.webkit.org/changeset/251798>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56756913>

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Build fix attempt landed in https://trac.webkit.org/changeset/251835.

[chris fleizach]

(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