745429 - don't use QueryInterface() in CAccessibleImage

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

similar to bug 741697
in CAccessibleImage::QueryInterface() you can use static cast this to be a nsAccessibleWrap then call IsImage() on it.
in CAccessibleImage::get_description() you can static cast this  to nsAccessibleWrap* and call GetName() on that, please add a null check while your here.
for CAccessibleImage::get_imageSize() / get_imagePosition() static cast this to nsAccessibleWrap* then call AsImage(), and please add IsDefunct() checks here too.

Comment 1

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #0)

> for CAccessibleImage::get_imageSize() / get_imagePosition() static cast this
> to nsAccessibleWrap* then call AsImage(), and please add IsDefunct() checks
> here too.

get_imageSize uses nsIAccessibleImage, we don't have dexpcomed version of nsIAccessibleImage. It sounds we should do that prior this bug. Agree?

Note, no one class sets eImageAccessible flag. Also we should rename IsImageAccessible to IsImage (i.e. no IsImage method currently). Sounds like we could split that into few bugs.

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #1)
> (In reply to Trevor Saunders (:tbsaunde) from comment #0)
> 
> > for CAccessibleImage::get_imageSize() / get_imagePosition() static cast this
> > to nsAccessibleWrap* then call AsImage(), and please add IsDefunct() checks
> > here too.
> 
> get_imageSize uses nsIAccessibleImage, we don't have dexpcomed version of
> nsIAccessibleImage. It sounds we should do that prior this bug. Agree?

Well, other than returning void from the two methods not sure how much else we need to change there, and I'd think we can do that before or after this bug.

> 
> Note, no one class sets eImageAccessible flag. Also we should rename
> IsImageAccessible to IsImage (i.e. no IsImage method currently). Sounds like

err, yeah, we need to fix that.

> we could split that into few bugs.

yeah, I'll file em.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)

> > get_imageSize uses nsIAccessibleImage, we don't have dexpcomed version of
> > nsIAccessibleImage. It sounds we should do that prior this bug. Agree?
> 
> Well, other than returning void from the two methods not sure how much else
> we need to change there, and I'd think we can do that before or after this
> bug.

until we dexpcom'ed nsIAccessibleImage we can't stop QueryInterface (that's what the bug is about).

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > > get_imageSize uses nsIAccessibleImage, we don't have dexpcomed version of
> > > nsIAccessibleImage. It sounds we should do that prior this bug. Agree?
> > 
> > Well, other than returning void from the two methods not sure how much else
> > we need to change there, and I'd think we can do that before or after this
> > bug.
> 
> until we dexpcom'ed nsIAccessibleImage we can't stop QueryInterface (that's
> what the bug is about).

Well, other than the part were nsHTMLIMageAccessible needs to set eImage bit you could do it now by downcasting to nsHTMLImageAccessible since its the only type to impl that interface and then calling the xpcom method.

On the other hand I wonder how much point there is in an interface with two trivial functions, perhaps we should just  call the nsAccessible methods they call directly.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> Well, other than the part were nsHTMLIMageAccessible needs to set eImage bit
> you could do it now by downcasting to nsHTMLImageAccessible since its the
> only type to impl that interface and then calling the xpcom method.

right. We need to rename nsHTMLImageAccessible to ImageAccessible and move it to generic folder.

> On the other hand I wonder how much point there is in an interface with two
> trivial functions, perhaps we should just  call the nsAccessible methods
> they call directly.

agree

Comment 6

[Mark Capella [:capella]]

So should there be an open bug to de-xpcom nsIAccessibleImage to move this forward? I can't find one ...

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #6)
> So should there be an open bug to de-xpcom nsIAccessibleImage to move this
> forward? I can't find one ...

I think it's not necessary to have it since basically nsIAccessibleImage implementation is a simple call of GetBounds() method. However it should be nice to have simple inlines since this code is shared between xpcom, atk and ia2. It's up to you to file a bug or do that here.

Comment 8

[Mark Capella [:capella]]

Ok, then are we going to De-ns-ify nsHTMLImageAccessible (comment5) beforehand ... s/b a bug for this I can start on?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #8)
> Ok, then are we going to De-ns-ify nsHTMLImageAccessible (comment5)
> beforehand ... s/b a bug for this I can start on?

it'd be nice but not a requirement I think

Comment 10

[Mark Capella [:capella]]

Attachment: Patch (v1)

Initial patch, asking for feedback from mentor...

I had to use reinterpret_cast, as using static_cast causes my compiler to complain of incompatible types.

Let me know if anything really wrong jumps out at you, or if there's something further here you were asking for in the back and forth discussions with Surkov.

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 622756 [details] [diff] [review]
Patch (v1)

Review of attachment 622756 [details] [diff] [review]:
-----------------------------------------------------------------

stealing feedback from Trevor, asking him for review

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +439,1 @@
>          interfacesBits |= 1 << MAI_INTERFACE_IMAGE;

nit: while you are here, please indent it properly

::: accessible/src/atk/nsMaiInterfaceImage.cpp
@@ +55,1 @@
>        return;

same, just these three lines (don't forget to fix type* name issue)

@@ +76,3 @@
>        return;
>  
>      accWrap->AsImage()->GetImageSize(aAccWidth, aAccHeight);

you could align whole body

::: accessible/src/msaa/CAccessibleImage.cpp
@@ +144,3 @@
>  
> +  nsHTMLImageAccessible* imageAcc = acc->AsImage();
> +  PRInt32 width = 0, height = 0;

I'd put empty line between these and no empty line beetween width, heght and GetImageSize call

@@ +144,4 @@
>  
> +  nsHTMLImageAccessible* imageAcc = acc->AsImage();
> +  PRInt32 width = 0, height = 0;
> +

nit: it seems like empty line contains whitespaces

Comment 12

[Mark Capella [:capella]]

Attachment: Patch (v2)

Updated patch with nits addressed, requesting review? from Tbsaunde...

FYi - I fixed up the whole method in nsAccessibleWrap.cpp, but did the whole file in nsMaiInterfaceImage.cpp as it's a short one.

Comment 13

[Trevor Saunders (:tbsaunde)]

> FYi - I fixed up the whole method in nsAccessibleWrap.cpp, but did the whole
> file in nsMaiInterfaceImage.cpp as it's a short one.

honestly I'd just assume you didn't, because now I have to review a bunch of whitespace only changes and sort them from real changes, but *shrug*

Comment 14

[Trevor Saunders (:tbsaunde)]

Comment on attachment 623412 [details] [diff] [review]
Patch (v2)

>-    // Add Interfaces for each nsIAccessible.ext interfaces
>+  // Add Interfaces for each nsIAccessible.ext interfaces

nit, existing comment is sort of funy, no idea what nsIAccessible.ext interface...

>+  // the Component interface are supported by all nsIAccessible

nit, s/are/is/ if you don't mind

>+++ b/accessible/src/msaa/CAccessibleImage.cpp
>@@ -37,55 +37,57 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "CAccessibleImage.h"
> 
> #include "AccessibleImage_i.c"
> 
>+#include "nsAccessibleWrap.h"
>+#include "nsHTMLImageAccessible.h"

just include nsHTMLImageAccessibleWrap.h

> CAccessibleImage::QueryInterface(REFIID iid, void** ppv)

be nice if oyu kept with renaming these classes to ia2Foo, but a follow would be fine too.

>   if (IID_IAccessibleImage == iid) {
>-    nsCOMPtr<nsIAccessibleImage> imageAcc(do_QueryObject(this));
>-    if (!imageAcc)
>-      return E_FAIL;
>+    nsAccessibleWrap* acc = reinterpret_cast<nsAccessibleWrap*>(this);
>+    if (acc->IsDefunct())
>+      return CO_E_OBJNOTCONNECTED;

don't do the first check since it can't fail, and don't do the second because interfaces shouldn't depend on defunctness (I think it slightly breaks com rules, but more importantly is just funny)

>-  nsCOMPtr<nsIAccessible> acc(do_QueryObject(this));
>-  if (!acc)
>-    return E_FAIL;
>+  nsAccessibleWrap* acc = reinterpret_cast<nsAccessibleWrap*>(this);

no, you need to static_cast to nsHTMLImageAccessibleWrap if msvc won't let you go to a super class of it.  msvc might do what you want with reinterpret cast, but I really have no idea, and wouldn't trust it even if I did.

Comment 15

[Mark Capella [:capella]]

Attachment: Patch (v3)

Ok, all nits were addressed, and with this latest version of the patch, I've renamed the class to ia2AccessibleImage. This builds and tests fine on my WIN machine, but I haven't done a TRY push yet. 

When you think this is stable / close to being finished, I'll do the TRY and ensure all other platform builds work properly.

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 624634 [details] [diff] [review]
Patch (v3)

Review of attachment 624634 [details] [diff] [review]:
-----------------------------------------------------------------

r=me (let's flush some of your queue)

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +400,2 @@
>      
> +  // Add Interfaces for each nsIAccessible interface

nit: I'd say to remove this comment at all

@@ +402,2 @@
>  
> +  // the Component interface is supported by all nsIAccessible

nit: the -> The, nsIAccessible -> accessibles. Dot in the end please

@@ +406,5 @@
>    // Add Action interface if the action count is more than zero.
>    if (ActionCount() > 0)
>      interfacesBits |= 1 << MAI_INTERFACE_ACTION;
>  
> +  //nsIAccessibleText

// Text interface.

@@ +420,5 @@
> +  QueryInterface(NS_GET_IID(nsIAccessibleEditableText),
> +                 getter_AddRefs(accessInterfaceEditableText));
> +  if (accessInterfaceEditableText) {
> +    interfacesBits |= 1 << MAI_INTERFACE_EDITABLE_TEXT;
> +  }

please replace nsIAccessibleText/nsIAccessibleEditableText queries on
nsHyperTextAccessible* hyperText = AsHyperText();
if (hyperText && hyperText->IsTextRole()) {
  interfacesBits |= 1 << MAI_INTERFACE_TEXT;
  interfacesBits |= 1 << MAI_INTERFACE_EDITABLE_TEXT;
  if (!nsAccUtils::MustPrune(this))
    interfacesBits |= 1 << MAI_INTERFACE_HYPERTEXT;
}

@@ +425,2 @@
>  
> +  //nsIAccessibleValue

// Value interface and etc

::: accessible/src/msaa/CAccessibleImage.cpp
@@ +59,3 @@
>  {
> +  *ppv = static_cast<IAccessibleImage*>(this);
> +  (reinterpret_cast<IUnknown*>(*ppv))->AddRef();

you return not NULL pointer not depending on REFIID (I think you should keep a check IID_IAccessibleImage)

::: accessible/src/msaa/CAccessibleImage.h
@@ +44,3 @@
>  #include "AccessibleImage.h"
>  
> +class ia2AccessibleImage: public IAccessibleImage

nit: space before ":"

Comment 17

[Trevor Saunders (:tbsaunde)]

Comment on attachment 624634 [details] [diff] [review]
Patch (v3)

>+ia2AccessibleImage::QueryInterface(REFIID iid, void** ppv)
> {
>-  *ppv = NULL;
>+  *ppv = static_cast<IAccessibleImage*>(this);
>+  (reinterpret_cast<IUnknown*>(*ppv))->AddRef();

I'm pretty sure you could use static cast here, but in any case I cn't see a reason we don't just use AddRef() since we never do any funny tricks here.

>+ia2AccessibleImage::get_imagePosition(enum IA2CoordinateType aCoordType,
>+                                      long* aX,
>+                                      long* aY)
> {
> __try {
>   *aX = 0;
>   *aY = 0;
> 
>+  nsHTMLImageAccessibleWrap* acc =
>+    static_cast<nsHTMLImageAccessibleWrap*>(this);
>+  if (acc->IsDefunct())
>+    return CO_E_OBJNOTCONNECTED;
>+
>+  nsHTMLImageAccessible* imageAcc = acc->AsImage();

no point in this.

>-    return E_FAIL;
>+  nsHTMLImageAccessibleWrap* acc =
>+    static_cast<nsHTMLImageAccessibleWrap*>(this);
>+  if (acc->IsDefunct())
>+    return CO_E_OBJNOTCONNECTED;
> 
>-  PRInt32 x = 0, y = 0, width = 0, height = 0;
>+  nsHTMLImageAccessible* imageAcc = acc->AsImage();

same

Comment 18

[Mark Capella [:capella]]

Please clarify "no point in this." Do you mean I could code something like:

nsHTMLImageAccessibleWrap* imageAcc = static_cast<nsHTMLImageAccessibleWrap*>(this)->AsImage();

Comment 19

[Trevor Saunders (:tbsaunde)]

I mean if you have a pointer of type nsHTMLImageAccessible* calling AsImage() on it does nothing useful.

Comment 20

[Mark Capella [:capella]]

Ok, i was trying to keep to the original thought / request was to use AsImage() ... but thats when we were using an nsAccessibleWrap* which changed in comment 14 ... so all I need to do now is for example:

  nsHTMLImageAccessibleWrap* imageAcc =
    static_cast<nsHTMLImageAccessibleWrap*>(this);
  if (acc->IsDefunct())
    return CO_E_OBJNOTCONNECTED;

  PRInt32 width = 0, height = 0;
   nsresult rv = imageAcc->GetImageSize(&width, &height);

Comment 21

[Mark Capella [:capella]]

Attachment: Patch (v4)

All right then! With the review+, this should address all final nits. I think I properly incorporated the last few changes as requested coming in from both Alex and Trev. I do wish to err on the side of caution, so am posting the patch as it now stands.

As usual, it builds and tests clean on my local WIN7 system, and absent any last minute objections, will later be pushed to TRY with results posted, before pushing to inbound.

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 625826 [details] [diff] [review]
Patch (v4)

Review of attachment 625826 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +401,5 @@
>  
> +  if (!nsAccUtils::MustPrune(this)) {  // These interfaces require children
> +    // Hypertext interface.
> +    if (IsHyperText()) {
> +      interfacesBits |= 1 << MAI_INTERFACE_HYPERTEXT;

you expose hypertext interface twice

Comment 23

[Mark Capella [:capella]]

Attachment: Patch (v5)

Cleaned it up ....

Comment 24

[Mark Capella [:capella]]

Push to inbound TRY
https://tbpl.mozilla.org/?tree=Try&rev=eea7a9606f4e

Comment 25

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7a6feee6c13c

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #25)
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7a6feee6c13c

https://hg.mozilla.org/integration/mozilla-inbound/rev/0835ae1e317b

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0835ae1e317b