don't use QueryInterface() in CAccessibleImage

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: capella)

Tracking

unspecified
mozilla15
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
(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.
Summary: don't use QueryInterface() in CAccessibleComponent → don't use QueryInterface() in CAccessibleImage
(Reporter)

Comment 2

5 years ago
(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

5 years ago
(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).
Blocks: 574336
No longer blocks: 389800
(Reporter)

Comment 4

5 years ago
(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

5 years ago
(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
(Assignee)

Updated

5 years ago
Depends on: 740747
(Assignee)

Comment 6

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

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

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

Comment 9

5 years ago
(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
(Assignee)

Comment 10

5 years ago
Created attachment 622756 [details] [diff] [review]
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.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #622756 - Flags: feedback?(trev.saunders)

Comment 11

5 years ago
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
Attachment #622756 - Flags: review?(trev.saunders)
Attachment #622756 - Flags: feedback?(trev.saunders)
Attachment #622756 - Flags: feedback+
(Assignee)

Comment 12

5 years ago
Created attachment 623412 [details] [diff] [review]
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.
Attachment #622756 - Attachment is obsolete: true
Attachment #622756 - Flags: review?(trev.saunders)
Attachment #623412 - Flags: review?(trev.saunders)
(Reporter)

Comment 13

5 years ago
> 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*
(Reporter)

Comment 14

5 years ago
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.
Attachment #623412 - Flags: review?(trev.saunders)
(Assignee)

Comment 15

5 years ago
Created attachment 624634 [details] [diff] [review]
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.
Attachment #623412 - Attachment is obsolete: true

Comment 16

5 years ago
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 ":"
Attachment #624634 - Flags: review+
(Reporter)

Comment 17

5 years ago
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
(Assignee)

Comment 18

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

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

Comment 19

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

Comment 20

5 years ago
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);
(Assignee)

Comment 21

5 years ago
Created attachment 625826 [details] [diff] [review]
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.
Attachment #624634 - Attachment is obsolete: true

Comment 22

5 years ago
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
(Assignee)

Comment 23

5 years ago
Created attachment 625963 [details] [diff] [review]
Patch (v5)

Cleaned it up ....
Attachment #625826 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
Push to inbound TRY
https://tbpl.mozilla.org/?tree=Try&rev=eea7a9606f4e
(Assignee)

Comment 25

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7a6feee6c13c
Target Milestone: --- → mozilla15

Comment 26

5 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/0835ae1e317b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.