Bug 121959 – [ATK] Normalize checks in entry points for DRT and WKTR

• Component: Accessibility
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

By taking a look to ATK's DRT and WKTR, is easy to check that there is no consistency in the way we do the checks at the entry points for AccessibilityUIElement, since sometimes we check !m_element, other times we check !ATK_IS_OBJECT(m_element) - or an specific ATK interface -, sometimes we check both... and sometimes we just don't check, which leads to some funny messages in stderr such as this one:

  atk_object_ref_accessible_child: assertion `ATK_IS_OBJECT (accessible)' failed

At least, this is a problem with tests such as accessibility/loading-iframe-updates-axtree.html or accessibility/svg-bounds.html, which are spitting that kind of error because the stuff that is needed for the test to pass is not implemented (either in the ATK code or in the DRT/WKTR).

Another manifestation of this issue, again due to tests running whose needs are not [properly] implemented yet, is what you can see in the logs for WK2GTK layout tests:

  07:30:55.934 26675 worker/1 accessibility/canvas-fallback-content-2.html output stderr lines:
  07:30:55.934 26675   invalid cast from `WAIType191' to `AtkValue'
  07:30:55.934 26675   atk_value_get_current_value: assertion `ATK_IS_VALUE (obj)' failed
  07:30:55.934 26675   invalid cast from `WAIType199' to `AtkValue'
  07:30:55.934 26675   atk_value_get_current_value: assertion `ATK_IS_VALUE (obj)' failed
  07:30:55.934 26675   invalid cast from `WAIType191' to `AtkValue'
  [...]

So, I think we should agree on a normalized way to check the entry points in DRT/WKTR, and my proposal is to check that we are in front of an instance of AtkObject implementing, if needed, the interfaces that we need for implementing a given functionality, gracefully exiting otherwise. No more checking for non-null and maybe later asserting for the right interface, since that will be of little help if the object we are receiving is not of the expected type (maybe, because the hierarchy exposed for GTK does not match in that case the one exposed for the Mac). And anyway, we will still detect that there's an issue or a missing implementation somewhere since the expectations won't match.

For instance, consider the following partial diff between the current situation and the proposed one:

  [...]
   int AccessibilityUIElement::columnCount()
   {
  -    if (!m_element)
  +    if (!ATK_IS_TABLE(m_element))
           return 0;
   
  -    ASSERT(ATK_IS_TABLE(m_element));
  -
       return atk_table_get_n_columns(ATK_TABLE(m_element));
   }
   
   int AccessibilityUIElement::childrenCount()
   {
  -    if (!m_element)
  +    if (!ATK_IS_OBJECT(m_element))
           return 0;
   
  -    ASSERT(ATK_IS_OBJECT(m_element));
  -
       return atk_object_get_n_accessible_children(ATK_OBJECT(m_element));
   }
   
   AccessibilityUIElement AccessibilityUIElement::elementAtPoint(int x, int y)
   {
  -    if (!m_element)
  +    if (!ATK_IS_COMPONENT(m_element))
           return 0;
   
       return AccessibilityUIElement(atk_component_ref_accessible_at_point(ATK_COMPONENT(m_element), x, y, ATK_XY_WINDOW));
   }
  [...]


As the ATK_IS_OBJECT, ATK_IS_TABLE... macros already do the null check, we do not need to check that explicitly. Also, we do not need to check for ATK_IS_OBJECT if we are already checking for an ATK interface, since that will always be implemented by an ATK_OBJECT. Last, I would not use ASSERT() here because we do not certainly want to continue if we don't have the proper object. Let's just the results be wrong, so we can realize of something being wrong.

For all these reasons, and also to prevent further confusion when adding code to these files in DRT and WKTR, I propose to write a patch now to normalize the situation. A patch that I've been already working on and testing today and that I expect to upload soon.

[Radar WebKit Bug Importer]

<rdar://problem/15087770>

[Mario Sanchez Prada]

Created attachment 212717 [details]
Patch proposal

I tested this patch both with WK1 and WK2 and actually gets rid of those error messages, while not causing any regression.

Pleas review. Thanks!

[chris fleizach]

Comment on attachment 212717 [details]
Patch proposal

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

> Tools/ChangeLog:5
> +

can you add a description of what and why this patch is needed

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:814
> +    if (!ATK_IS_VALUE(m_element.get()))

I don't know much about this ATK_IS_* API, but it does seem strange to me that an m_element could be
ATK_IS_VALUE or ATK_IS_OBJECT

The difference between ATK_IS_OBJECT and ATK_IS_COMPONENT is also pretty nebulous it seems.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1079
>      if (!ATK_IS_ACTION(m_element.get()))

ditto about the naming. maybe this is right, but it does seem strange that you can check that the element is ATK_IS_ACTION

[Mario Sanchez Prada]

(In reply to comment #3)
> (From update of attachment 212717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212717&action=review
> 
> > Tools/ChangeLog:5
> > +
> 
> can you add a description of what and why this patch is needed

Strange. I had written one already but for it seems I just did it as a commit message and not in the proper ChangeLog, since the description is here:

https://bug-121959-attachments.webkit.org/attachment.cgi?id=212717

Anyway, I'll put it in the right place and submit the patch again.

> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:814
> > +    if (!ATK_IS_VALUE(m_element.get()))
> 
> I don't know much about this ATK_IS_* API, but it does seem strange to me that an m_element could be
> ATK_IS_VALUE or ATK_IS_OBJECT
> 
> The difference between ATK_IS_OBJECT and ATK_IS_COMPONENT is also pretty nebulous it seems.

Yes, I agree it's not very clear but it makes sense after all.

TL;DR:
------
AtkObject is an abstract class, while AtkComponent, AtkValue, AtkDocument... are interfaces that have to be implemented by instances of AtkObject, or derived class. Thus, checking ATK_IS_IMAGE() or ATK_COMPONENT() effectively implies in our case ATK_IS_OBJECT(), since those interfaces are only implemented by AtkObject.

Long version:
-------------
Welcome to GObject! :)

In a nutshell, AtkObject is an abstract class which I derived our concrete implementation WebKitAccessible, which is what you can see in WebKitAccessibleWrapperAtk, and what we use as our base for implementing accessible objects in ATK.

Then other things such as AtkValue, AtkComponent, AtkAction... are just interfaces that are implemented by subclasses of WebKitAccessible, we generate those classes/types dynamically according to tha nature of each wrapped AccessibilityObject, and we of course reuse those dynamically generated types when we found objects requiring to implement exactly the same interfaces that another one found earlier.

You can see a trace of those dynamically generated types in the description of this bug: "WAIType191" & "WAIType199". Each of those classes will be subclasses of WebKitAccessible (thus subclasses of AtkObject) implementing a different sets of interfaces.

So far so good, but this is plain C and so all this object oriented thang is not provided by the language, so we have GObject which is a library that provides us a whole system to implement OOP in C and that, besides forcing us to work in a certain way when writing the code (e.g. implementing something similar to C++ vtables ourselves),  includes macros such as ATK_OBJECT (just a cast to AtkObject*) or ATK_IS_OBJECT (to check if a pointer references a real instance of AtkObject), or WEBKIT_ACCESSIBLE and WEBKIT_IS_ACCESSIBLE (same thing but for objects from that subclass).

And turns out that, as we do neither things such as polymorphism in C, we ended up using those macros all the time, depending on which API we want to use: for instance, if we have an instance of WebKitAccessible and want to use some of the AtkObject API over it, we need to cast it first, so we make sure we only use the region of memory related to AtkObject among the whole lot of memory used for a WebKitAccessible:

  atk_object_get_name(ATK_OBJECT(webkitAccessible))

So, for the interfaces is exactly the same: we use macros such as ATK_IMAGE or ATK_COMPONENT whenever we want to use the API of a given interface over a specific AtkObject or, in the case of WebKitGTK, of WebKitAccessible:

  atk_table_get_n_columns(ATK_TABLE(webkitAccessible))

In a similar fashion, when we want to check if a pointer references a valid AtkObject we use ATK_IS_OBJECT(object), and when we want to check if a given AtkObject implements one interface such as AtkComponent, we do ATK_IS_COMPONENT(object). And as I mentioned in the TL;DR section above, as those interfaces are always implemented by AtkObject's only, they effectively imply the ATK_IS_OBJECT(object) check, thus it's not needed.

So, wrapping up: AtkObject is an abstrac class, while the rest of things (e.g. AtkImage, AtkDocument...) are interfaces. That is why something can be ATK_OBJECT and ATK_VALUE at the same time.

Hope is clearer now :)

> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1079
> >      if (!ATK_IS_ACTION(m_element.get()))
> 
> ditto about the naming. maybe this is right, but it does seem strange that you can check that the element is ATK_IS_ACTION

In this specific case, I'm checking "if it's an AtkObject implementing the AtkAction interface". This will fail either if one of those is not true, or if it's just null.

Attaching a new patch with the right changelog soon...

[chris fleizach]

Thanks for the explanation!

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 212717 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=212717&action=review
> > 
> > > Tools/ChangeLog:5
> > > +
> > 
> > can you add a description of what and why this patch is needed
> 
> Strange. I had written one already but for it seems I just did it as a commit message and not in the proper ChangeLog, since the description is here:
> 
> https://bug-121959-attachments.webkit.org/attachment.cgi?id=212717
> 
> Anyway, I'll put it in the right place and submit the patch again.
> 
> > > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:814
> > > +    if (!ATK_IS_VALUE(m_element.get()))
> > 
> > I don't know much about this ATK_IS_* API, but it does seem strange to me that an m_element could be
> > ATK_IS_VALUE or ATK_IS_OBJECT
> > 
> > The difference between ATK_IS_OBJECT and ATK_IS_COMPONENT is also pretty nebulous it seems.
> 
> Yes, I agree it's not very clear but it makes sense after all.
> 
> TL;DR:
> ------
> AtkObject is an abstract class, while AtkComponent, AtkValue, AtkDocument... are interfaces that have to be implemented by instances of AtkObject, or derived class. Thus, checking ATK_IS_IMAGE() or ATK_COMPONENT() effectively implies in our case ATK_IS_OBJECT(), since those interfaces are only implemented by AtkObject.
> 
> Long version:
> -------------
> Welcome to GObject! :)
> 
> In a nutshell, AtkObject is an abstract class which I derived our concrete implementation WebKitAccessible, which is what you can see in WebKitAccessibleWrapperAtk, and what we use as our base for implementing accessible objects in ATK.
> 
> Then other things such as AtkValue, AtkComponent, AtkAction... are just interfaces that are implemented by subclasses of WebKitAccessible, we generate those classes/types dynamically according to tha nature of each wrapped AccessibilityObject, and we of course reuse those dynamically generated types when we found objects requiring to implement exactly the same interfaces that another one found earlier.
> 
> You can see a trace of those dynamically generated types in the description of this bug: "WAIType191" & "WAIType199". Each of those classes will be subclasses of WebKitAccessible (thus subclasses of AtkObject) implementing a different sets of interfaces.
> 
> So far so good, but this is plain C and so all this object oriented thang is not provided by the language, so we have GObject which is a library that provides us a whole system to implement OOP in C and that, besides forcing us to work in a certain way when writing the code (e.g. implementing something similar to C++ vtables ourselves),  includes macros such as ATK_OBJECT (just a cast to AtkObject*) or ATK_IS_OBJECT (to check if a pointer references a real instance of AtkObject), or WEBKIT_ACCESSIBLE and WEBKIT_IS_ACCESSIBLE (same thing but for objects from that subclass).
> 
> And turns out that, as we do neither things such as polymorphism in C, we ended up using those macros all the time, depending on which API we want to use: for instance, if we have an instance of WebKitAccessible and want to use some of the AtkObject API over it, we need to cast it first, so we make sure we only use the region of memory related to AtkObject among the whole lot of memory used for a WebKitAccessible:
> 
>   atk_object_get_name(ATK_OBJECT(webkitAccessible))
> 
> So, for the interfaces is exactly the same: we use macros such as ATK_IMAGE or ATK_COMPONENT whenever we want to use the API of a given interface over a specific AtkObject or, in the case of WebKitGTK, of WebKitAccessible:
> 
>   atk_table_get_n_columns(ATK_TABLE(webkitAccessible))
> 
> In a similar fashion, when we want to check if a pointer references a valid AtkObject we use ATK_IS_OBJECT(object), and when we want to check if a given AtkObject implements one interface such as AtkComponent, we do ATK_IS_COMPONENT(object). And as I mentioned in the TL;DR section above, as those interfaces are always implemented by AtkObject's only, they effectively imply the ATK_IS_OBJECT(object) check, thus it's not needed.
> 
> So, wrapping up: AtkObject is an abstrac class, while the rest of things (e.g. AtkImage, AtkDocument...) are interfaces. That is why something can be ATK_OBJECT and ATK_VALUE at the same time.
> 
> Hope is clearer now :)
> 
> > > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1079
> > >      if (!ATK_IS_ACTION(m_element.get()))
> > 
> > ditto about the naming. maybe this is right, but it does seem strange that you can check that the element is ATK_IS_ACTION
> 
> In this specific case, I'm checking "if it's an AtkObject implementing the AtkAction interface". This will fail either if one of those is not true, or if it's just null.
> 
> Attaching a new patch with the right changelog soon...

[Mario Sanchez Prada]

Created attachment 212740 [details]
Patch proposal

(In reply to comment #5)
> Thanks for the explanation!
> 
I can't believe you have read it :). Thanks for appreciating the explanation. One day I'll need something similar about Objective-C, but for now all I know about it is basically what Wikipedia tells about it in this site

  http://en.wikipedia.org/wiki/Objective-C

Anyway, now attaching a new patch with the right ChangeLog...

[Mario Sanchez Prada]

Committed r156531: <http://trac.webkit.org/changeset/156531>