Last Comment Bug 539683 - defunct object methods should return CO_E_OBJECTNOTCONNECTED
: defunct object methods should return CO_E_OBJECTNOTCONNECTED
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All Windows XP
: -- normal (vote)
: mozilla14
Assigned To: alexander :surkov
:
Mentors:
http://msdn.microsoft.com/en-us/libra...
Depends on:
Blocks: cleana11y 706026 707488
  Show dependency treegraph
 
Reported: 2010-01-14 06:25 PST by alexander :surkov
Modified: 2012-05-04 04:54 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1 (88.18 KB, patch)
2011-08-12 22:07 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review-
Details | Diff | Review
patch2 (41.09 KB, patch)
2012-04-09 01:23 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Review
patch3 (44.62 KB, patch)
2012-04-09 05:46 PDT, alexander :surkov
no flags Details | Diff | Review

Description alexander :surkov 2010-01-14 06:25:37 PST
MSDN: When a client calls an IAccessible property to get information about an object, the proxy object must check whether the accessible object is still available. If it is, the proxy object forwards the client's request to the accessible object. If the accessible object is destroyed (for example, when a dialog box with custom controls is closed), the proxy object returns an error. To indicate that the object has been destroyed, it is recommended that servers return the error code CO_E_OBJECTNOTCONNECTED because this error is returned by COM after a server calls CoDisconnectObject.
Comment 1 Trevor Saunders (:tbsaunde) 2011-08-12 22:07:31 PDT
Created attachment 552834 [details] [diff] [review]
patch 1
Comment 2 alexander :surkov 2011-08-12 23:58:32 PDT
Comment on attachment 552834 [details] [diff] [review]
patch 1

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

About QueryInterface ppv check: you can not check it in CAccessible classes if you check it in wrap classes

no IsDefunct check for nsHyperTextAccessibleWrap::GetModifiedText

::: accessible/src/msaa/CAccessibleAction.cpp
@@ +49,5 @@
>  STDMETHODIMP
>  CAccessibleAction::QueryInterface(REFIID iid, void** ppv)
>  {
> +  if (!ppv)
> +    return E_INVALIDARG;

no need to check

@@ +212,5 @@
>  CAccessibleAction::get_localizedName(long aActionIndex, BSTR *aLocalizedName)
>  {
>  __try {
> +  if (!aLocalizedName)
> +    return E_INVALIDARG;

maybe E_NOTIMPL is enough for either case, otherwise you should add objdisconnected and fix other not implemented methods to be consistent

::: accessible/src/msaa/CAccessibleComponent.cpp
@@ +64,5 @@
>  STDMETHODIMP
>  CAccessibleComponent::QueryInterface(REFIID iid, void** ppv)
>  {
> +  if (!ppv)
> +    return E_INVALIDARG;

no need to check

@@ +137,5 @@
>  CAccessibleComponent::get_foreground(IA2Color *aForeground)
>  {
>  __try {
> +  if (!aForeground)
> +    return E_INVALIDARG;

no need because GetARGBValueFromCSSProperty takes care

@@ +150,5 @@
>  CAccessibleComponent::get_background(IA2Color *aBackground)
>  {
>  __try {
> +  if (!aBackground)
> +    return E_INVALIDARG;

same

::: accessible/src/msaa/CAccessibleEditableText.cpp
@@ +154,5 @@
>                                       BSTR *aText)
>  {
>  __try {
> +  if (!aText)
> +    return E_INVALIDARG;

same

::: accessible/src/msaa/CAccessibleHypertext.cpp
@@ +49,5 @@
>  STDMETHODIMP
>  CAccessibleHypertext::QueryInterface(REFIID iid, void** ppv)
>  {
> +  if (!ppv)
> +    return E_INVALIDARG;

no need to check

::: accessible/src/msaa/CAccessibleTable.cpp
@@ +82,5 @@
>  // IAccessibleTable
>  
>  STDMETHODIMP
>  CAccessibleTable::get_accessibleAt(long aRow, long aColumn,
>                                     IUnknown **aAccessible)

why are other methods not touched too? Is ongoing table iface reorg makes this sort of fruitless? If so then why do you fix this method?

::: accessible/src/msaa/CAccessibleTableCell.cpp
@@ +79,5 @@
>  CAccessibleTableCell::get_table(IUnknown **table)
>  {
>  __try {
> +  if (!table)
> +    return E_INVALIDARG;

null out table

@@ +85,3 @@
>    nsCOMPtr<nsIAccessibleTableCell> tableCell(do_QueryObject(this));
>    NS_ASSERTION(tableCell, TABLECELL_INTERFACE_UNSUPPORTED_MSG);
> +  if (!tableCell || tableCell->IsDefunct()j

j instead ;

@@ +144,5 @@
>                                              long *nColumnHeaderCells)
>  {
>  __try {
> +  if (!cellAccessibles || !nColumnHeaderCells)
> +    return E_INVALIDARG;

null out cellAccessibles?

@@ +229,5 @@
>                                           long *nRowHeaderCells)
>  {
>  __try {
> +  if (!cellAccessibles || !nRowHeaderCells)
> +    return E_INVALIDARG;

null out cellAccessibles

::: accessible/src/msaa/CAccessibleText.cpp
@@ +50,5 @@
>  STDMETHODIMP
>  CAccessibleText::QueryInterface(REFIID iid, void** ppv)
>  {
> +  if (!ppv)
> +    return E_INVALIDARG;

no need to check

@@ +154,5 @@
>                                        long *aX, long *aY,
>                                        long *aWidth, long *aHeight)
>  {
>  __try {
> +  if (!aX || !aY || a!Height || !aWidth)

a!Height is funny

@@ +254,4 @@
>    *aStartOffset = 0;
>    *aEndOffset = 0;
>  
>    nsRefPtr<nsHyperTextAccessible> textAcc(do_QueryObject(this));

forgot to add IsDefunct check

@@ +461,5 @@
>  
>  } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
>    return E_FAIL;
>  }
>  

CAccessibleText::setCaretOffset is missed

::: accessible/src/msaa/CAccessibleValue.cpp
@@ +80,4 @@
>    VariantInit(aCurrentValue);
>  
> +  if (IsDefunct())
> +    return CO_E_OBJECTNOTCONNECTED;

compilation error, move it below to call on valueAcc

@@ +103,5 @@
>  CAccessibleValue::setCurrentValue(VARIANT aValue)
>  {
>  __try {
> +  if (IsDefunct())
> +    return CO_E_OBJECTNOTCONNECTED;

same

@@ +130,4 @@
>    VariantInit(aMaximumValue);
>  
> +  if (IsDefunct())
> +    return CO_E_OBJECTNOTCONNECTED;

same

@@ +159,4 @@
>    VariantInit(aMinimumValue);
>  
> +  if (IsDefunct())
> +    return CO_E_OBJECTNOTCONNECTED;

same

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +222,1 @@
>      /* [out] */ BSTR __RPC_FAR *aNodeName,

add ppv check to nsAccessNodeWrap::QueryInterface and nsAccessNodeWrap::QueryService

@@ +234,2 @@
>    *aNodeName = nsnull;
>    *aNodeValue = nsnull;

NULL out other values please and use NULL instead nsnull

@@ +285,1 @@
>    *aNumAttribs = 0;

null out other values too

@@ +320,5 @@
>  __try {
> +  if (!aAttribNames || !aNameSpaceID || !aAttribValues)
> +    return E_INVALIDARG;
> +
> +  *aAttribNames = *aAttribValues = nsnull;

NULL not nsnull

@@ +360,5 @@
>  /* To do: use media type if not null */
>  STDMETHODIMP nsAccessNodeWrap::get_computedStyle( 
>      /* [in] */ unsigned short aMaxStyleProperties,
>      /* [in] */ boolean aUseAlternateView,
> +    /*j[length_is][size_is][out] */ BSTR __RPC_FAR *aStyleProperties,

new j is not neccessary in comment

@@ +409,5 @@
>      /* [length_is][size_is][out] */ BSTR __RPC_FAR *aStyleValues)
>  {
>  __try {
> +  if (!aStyleProperties || !aStyleValues)
> +    return E_INVALIDARG;

NULL them please

@@ +494,5 @@
>  __try {
> +  if (!aNode)
> +    return E_INVALIDARG;
> +
> +  *aNode = nsnull;

nit: NULL instead nsnull

@@ +512,5 @@
>  __try {
> +  if (!aNode)
> +    return E_INVALIDARG;
> +
> +  *aNode = nsnull;

nit: NULL instead nsnull

@@ +530,5 @@
>  __try {
> +  if (!aNode)
> +    return E_INVALIDARG;
> +
> +  *aNode = nsnull;

too

@@ +548,5 @@
>  __try {
> +  if (!aNode)
> +    return E_INVALIDARG;
> +
> +  *aNode = nsnull;

too

@@ +566,5 @@
>  __try {
> +  if (!aNode)
> +    return E_INVALIDARG;
> +
> +  *aNode = nsnull;

too

@@ +590,1 @@
>    *aNode = nsnull;

too

@@ +609,1 @@
>    *aInnerHTML = nsnull;

too

@@ +665,5 @@
>  __try {
> +  if (!localInterface)
> +    return E_INVALIDARG;
> +
> +  *localInterface = nsnull;

too

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +202,5 @@
>  
>  STDMETHODIMP nsAccessibleWrap::get_accParent( IDispatch __RPC_FAR *__RPC_FAR *ppdispParent)
>  {
>  __try {
>    *ppdispParent = NULL;

arg check

@@ +296,3 @@
>    *pszName = NULL;
>    nsAccessible *xpAccessible = GetXPAccessibleFor(varChild);
> +  if (!xpAccessible || xpAccessible)

always true

@@ +332,3 @@
>    *pszValue = NULL;
>    nsAccessible *xpAccessible = GetXPAccessibleFor(varChild);
> +  if (!xpAccessible || xpAccessible->IsDefunct())

you should check on this accessible rather than on obtained one

@@ +336,4 @@
>  
> +  nsAutoString value;
> +  if (NS_FAILED(xpAccessible->GetValue(value)))
> +    return E_FAIL;

return GetHRESULT(rv);

@@ +363,5 @@
>    *pszDescription = NULL;
>  
>    nsAccessible *xpAccessible = GetXPAccessibleFor(varChild);
>    if (!xpAccessible || xpAccessible->IsDefunct())
> +    return CO_E_OBJECTNOTCONNECTED;

IsDefunct() on this

@@ +389,4 @@
>  
>    nsAccessible *xpAccessible = GetXPAccessibleFor(varChild);
> +  if (!xpAccessible || xpAccessible->IsDefunct())
> +    return CO_E_OBJECTNOTCONNECTED;

too

@@ +466,5 @@
>    pvarState->lVal = 0;
>  
>    nsAccessible *xpAccessible = GetXPAccessibleFor(varChild);
> +  if (!xpAccessible || xpAccessible->IsDefunct())
> +    return CO_E_OBJECTNOTCONNECTED;

IsDefunct() on this

@@ +496,1 @@
>    *pszHelp = NULL;

add IsDefunct check

@@ +513,4 @@
>    *pszHelpFile = NULL;
>    *pidTopic = 0;
>    return S_FALSE;
>  

IsDefunct check?

@@ +529,5 @@
>    *pszKeyboardShortcut = NULL;
>  
>    nsAccessible* acc = GetXPAccessibleFor(varChild);
>    if (!acc || acc->IsDefunct())
> +    return CO_E_OBJECTNOTCONNECTED;

IsDefunct() on this

@@ +778,4 @@
>    *pszDefaultAction = NULL;
>    nsAccessible *xpAccessible = GetXPAccessibleFor(varChild);
> +  if (!xpAccessible || xpAccessible->IsDefunct())
> +    return CO_E_OBJECTNOTCONNECTED;

IsDefunct() on this

@@ +799,5 @@
>  __try {
>    // currently only handle focus and selection
>    nsAccessible *xpAccessible = GetXPAccessibleFor(varChild);
> +  if (!xpAccessible || xpAccessible->IsDefunct())
> +    return CO_E_OBJECTNOTCONNECTED;

IsDefunct() on this

@@ +834,5 @@
>        /* [optional][in] */ VARIANT varChild)
>  {
>  __try {
> +  if (!pxLeft || !pyTop || !pcxWidth || !pcyHeight)
> +    return E_INVALIDARG;

null them out

@@ +840,4 @@
>    nsAccessible *xpAccessible = GetXPAccessibleFor(varChild);
>  
> + if (!xpAccessible || xpAccessible->IsDefunct())
> +   return CO_E_OBJECTNOTCONNECTED;

IsDefunct() on this

@@ +868,3 @@
>    nsAccessible *xpAccessibleStart = GetXPAccessibleFor(varStart);
> +  if (!xpAccessibleStart || xpAccessibleStart->IsDefunct())
> +    return CO_E_OBJECTNOTCONNECTED;

IsDefunct() on this

@@ +871,2 @@
>  
>    VariantInit(pvarEndUpAt);

move it after null check like you have everywhere

@@ +1009,5 @@
>  {
>  __try {
>    nsAccessible *xpAccessible = GetXPAccessibleFor(varChild);
> +  if (!xpAccessible || xpAccessible->IsDefunct())
> +    return CO_E_OBJECTNOTCONNECTED;

IsDefunct() on this

@@ +1110,1 @@
>    *ppenum = nsnull;

nit: NULL

@@ +1289,3 @@
>    PRInt32 groupLevel = 0;
>    PRInt32 similarItemsInGroup = 0;
>    PRInt32 positionInGroup = 0;

IsDefunct?

@@ +1624,5 @@
>                           VARIANT *pVarResult, EXCEPINFO *pExcepInfo,
>                           UINT *puArgErr)
>  {
> +  if (!pDispParams || !pVarResult || !pExcepInfo || !puArgErr)
> +    return E_INVALIDARG;

pVarResult, pExcepInfo and puArgErr can be NULL, see http://msdn.microsoft.com/en-us/library/964ade8e-9d8a-4d32-bd47-aa678912a54d%28VS.85%29

::: accessible/src/msaa/nsApplicationAccessibleWrap.cpp
@@ +73,5 @@
>  nsApplicationAccessibleWrap::get_appName(BSTR *aName)
>  {
>  __try {
> +    if (!aName)
> +        return E_INVALIDARG;

wrong indentation

@@ +79,4 @@
>    *aName = NULL;
>  
> +  if (IsDefunct())
> +      return CO_E_OBJECTNOTCONNECTED;

same

@@ +100,5 @@
>  nsApplicationAccessibleWrap::get_appVersion(BSTR *aVersion)
>  {
>  __try {
> +    if (!aVersion)
> +        return E_INVALIDARG;

same

@@ +106,4 @@
>    *aVersion = NULL;
>  
> +  if (IsDefunct())
> +      return CO_E_OBJECTNOTCONNECTED;

same

@@ +133,5 @@
> +
> +    *aName = NULL;
> +
> +    if (IsDefunct())
> +        return CO_E_OBJECTNOTCONNECTED;

same for whole block

@@ +155,5 @@
>  nsApplicationAccessibleWrap::get_toolkitVersion(BSTR *aVersion)
>  {
>  __try {
> +    if (!aVersion)
> +        return E_INVALIDARG;

same

@@ +161,4 @@
>    *aVersion = NULL;
>  
> +  if (IsDefunct())
> +      return CO_E_OBJECTNOTCONNECTED;

same

::: accessible/src/msaa/nsDocAccessibleWrap.cpp
@@ +99,4 @@
>      
>    (reinterpret_cast<IUnknown*>(*ppv))->AddRef();
>    return S_OK;
>  }

add ppv check for QueryInterface

@@ +206,5 @@
>    /* [out] */ BSTR __RPC_FAR *aNameSpaceURI)
>  {
>  __try {
> +  if (aNameSpaceID < 0 || !aNameSpaceURI)
> +    return E_INVALIDARG;  // -1 is kNameSpaceID_Unknown

no need to check aNameSpaceID < 0, it doesn't make sense or it's inconsistent with values greater than available namespace count

::: accessible/src/msaa/nsTextAccessibleWrap.cpp
@@ +80,4 @@
>    (reinterpret_cast<IUnknown*>(*ppv))->AddRef(); 
>    return S_OK;
>  }
>  

ppv check for QueryInterface
Comment 3 Trevor Saunders (:tbsaunde) 2011-08-15 04:45:20 PDT
> ::: accessible/src/msaa/CAccessibleTable.cpp
> @@ +82,5 @@
> >  // IAccessibleTable
> >  
> >  STDMETHODIMP
> >  CAccessibleTable::get_accessibleAt(long aRow, long aColumn,
> >                                     IUnknown **aAccessible)
> 
> why are other methods not touched too? Is ongoing table iface reorg makes
> this sort of fruitless? If so then why do you fix this method?

I realized the interface not having IsDefunct() issue  I emailed you about   whenfixing this method, and didn't unfix

> 
> ::: accessible/src/msaa/CAccessibleTableCell.cpp
> @@ +79,5 @@
> >  CAccessibleTableCell::get_table(IUnknown **table)
> >  {
> >  __try {
> > +  if (!table)
> > +    return E_INVALIDARG;
> 
> null out table

ok, note that I actually need to remove the IsDefunct() checks for this to build,  same issue as for tables.

> @@ +103,5 @@
> >  CAccessibleValue::setCurrentValue(VARIANT aValue)
> >  {
> >  __try {
> > +  if (IsDefunct())
> > +    return CO_E_OBJECTNOTCONNECTED;
> 
> same

actually, that doesn't work, because its only a nsIAccessiblevalue*, and there's no concrete type that works here that I can see.

> @@ +285,1 @@
> >    *aNumAttribs = 0;
> 
> null out other values too

I'm not sure they're actually pointers, but zero sure.
Comment 4 Trevor Saunders (:tbsaunde) 2011-08-15 04:47:50 PDT
BTW, DO WE WANT iSdEFUNCT() ON isIPLEdomnODE METHODS, OR JUST CHECK McONTENT DIRECTLY?
Comment 5 alexander :surkov 2011-08-16 02:44:26 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > ::: accessible/src/msaa/CAccessibleTable.cpp
> > @@ +82,5 @@
> > >  // IAccessibleTable
> > >  
> > >  STDMETHODIMP
> > >  CAccessibleTable::get_accessibleAt(long aRow, long aColumn,
> > >                                     IUnknown **aAccessible)
> > 
> > why are other methods not touched too? Is ongoing table iface reorg makes
> > this sort of fruitless? If so then why do you fix this method?
> 
> I realized the interface not having IsDefunct() issue  I emailed you about  
> whenfixing this method, and didn't unfix

I'm not sure I understand you right. But why do you deal with CAccessibleTable differently from CAccessibleTableCell?

> > ::: accessible/src/msaa/CAccessibleTableCell.cpp
> > @@ +79,5 @@
> > >  CAccessibleTableCell::get_table(IUnknown **table)
> > null out table
> 
> ok, note that I actually need to remove the IsDefunct() checks for this to
> build,  same issue as for tables.

Why do you need to remove these checks, still don't get it, and what about "null out table" comment?

> > @@ +103,5 @@
> > >  CAccessibleValue::setCurrentValue(VARIANT aValue)
> > >  {
> > >  __try {
> > > +  if (IsDefunct())
> > > +    return CO_E_OBJECTNOTCONNECTED;
> > 
> > same
> 
> actually, that doesn't work, because its only a nsIAccessiblevalue*, and
> there's no concrete type that works here that I can see.

you should query nsAccessible

> 
> > @@ +285,1 @@
> > >    *aNumAttribs = 0;
> > 
> > null out other values too
> 
> I'm not sure they're actually pointers, but zero sure.

I used null term wide, sorry, of course for integer pointers you should use 0, for objects NULL

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> BTW, DO WE WANT iSdEFUNCT() ON isIPLEdomnODE METHODS, OR JUST CHECK McONTENT
> DIRECTLY?

IsDefunct to be on safe side, SimpleDOMNode can be nsAccessible object.
Comment 6 Trevor Saunders (:tbsaunde) 2011-08-16 03:30:44 PDT
(In reply to alexander surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > > ::: accessible/src/msaa/CAccessibleTable.cpp
> > > @@ +82,5 @@
> > > >  // IAccessibleTable
> > > >  
> > > >  STDMETHODIMP
> > > >  CAccessibleTable::get_accessibleAt(long aRow, long aColumn,
> > > >                                     IUnknown **aAccessible)
> > > 
> > > why are other methods not touched too? Is ongoing table iface reorg makes
> > > this sort of fruitless? If so then why do you fix this method?
> > 
> > I realized the interface not having IsDefunct() issue  I emailed you about  
> > whenfixing this method, and didn't unfix
> 
> I'm not sure I understand you right. But why do you deal with
> CAccessibleTable differently from CAccessibleTableCell?

we can't actually check IsDefunct() for CAccessibleTableCell either, it was a mistake that I tried in the patch.

> > > ::: accessible/src/msaa/CAccessibleTableCell.cpp
> > > @@ +79,5 @@
> > > >  CAccessibleTableCell::get_table(IUnknown **table)
> > > null out table
> > 
> > ok, note that I actually need to remove the IsDefunct() checks for this to
> > build,  same issue as for tables.
> 
> Why do you need to remove these checks, still don't get it, and what about
> "null out table" comment?

sure, I'l NULL out the table pointer.  I mean we don't have a concrete object that we can qi ourselves to and both have the methods we need and have IsDefunct(), I mean you have nsCOMPtr<nsIAccessibleTableCell> foo; you can't do foo->IsDefunct() because you only new foo implements nsIAccessibleTableCell, not NsAccessible.

> > > @@ +103,5 @@
> > > >  CAccessibleValue::setCurrentValue(VARIANT aValue)
> > > >  {
> > > >  __try {
> > > > +  if (IsDefunct())
> > > > +    return CO_E_OBJECTNOTCONNECTED;
> > > 
> > > same
> > 
> > actually, that doesn't work, because its only a nsIAccessiblevalue*, and
> > there's no concrete type that works here that I can see.
> 
> you should query nsAccessible

but not all nsAccessibles actually implement nsIAccessibleValue, see nsAccessible's QI.

> > 
> > > @@ +285,1 @@
> > > >    *aNumAttribs = 0;
> > > 
> > > null out other values too
> > 
> > I'm not sure they're actually pointers, but zero sure.
> 
> I used null term wide, sorry, of course for integer pointers you should use
> 0, for objects NULL

np :)

> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > BTW, DO WE WANT iSdEFUNCT() ON isIPLEdomnODE METHODS, OR JUST CHECK McONTENT
> > DIRECTLY?
> 
> IsDefunct to be on safe side, SimpleDOMNode can be nsAccessible object.

ok
Comment 7 alexander :surkov 2011-08-16 05:07:28 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> we can't actually check IsDefunct() for CAccessibleTableCell either, it was
> a mistake that I tried in the patch.

> sure, I'l NULL out the table pointer.  I mean we don't have a concrete
> object that we can qi ourselves to and both have the methods we need and
> have IsDefunct(), I mean you have nsCOMPtr<nsIAccessibleTableCell> foo; you
> can't do foo->IsDefunct() because you only new foo implements
> nsIAccessibleTableCell, not NsAccessible.

do two query interface, we want to be on safe side, then when you finish your table reorg thing you can replace one query interface on AsTable() call.

> but not all nsAccessibles actually implement nsIAccessibleValue, see
> nsAccessible's QI.

same
Comment 8 Trevor Saunders (:tbsaunde) 2011-08-16 17:53:19 PDT
(In reply to alexander surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> 
> > we can't actually check IsDefunct() for CAccessibleTableCell either, it was
> > a mistake that I tried in the patch.
> 
> > sure, I'l NULL out the table pointer.  I mean we don't have a concrete
> > object that we can qi ourselves to and both have the methods we need and
> > have IsDefunct(), I mean you have nsCOMPtr<nsIAccessibleTableCell> foo; you
> > can't do foo->IsDefunct() because you only new foo implements
> > nsIAccessibleTableCell, not NsAccessible.
> 
> do two query interface, we want to be on safe side, then when you finish
> your table reorg thing you can replace one query interface on AsTable() call.

I'm not convinced I really like this.  On one hand it would work, it would even let me do the decom of the windows table stuff sooner.  On the other hand it'll be a perf hit until the decom is done, I'm not really sure how bad, but I think qi is generally considered slow.  This also feels like a work around we may have to somewhat undo when we do the windows reorg.  So I'm fairly tempted to fix all the places we can without adding a second qi to nsAccessible.  Then we can do the reorg of msaa and ia2 and then finish adding these checks as we decom the last few classes.  Note that all the xpcom methods we call from the interfaces should be safe any  way at least in theory.
Comment 9 alexander :surkov 2011-08-26 04:30:59 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #8)

> > do two query interface, we want to be on safe side, then when you finish
> > your table reorg thing you can replace one query interface on AsTable() call.
> 
> I'm not convinced I really like this.  On one hand it would work, it would
> even let me do the decom of the windows table stuff sooner.  On the other
> hand it'll be a perf hit until the decom is done, I'm not really sure how
> bad, but I think qi is generally considered slow.  This also feels like a
> work around we may have to somewhat undo when we do the windows reorg.  So
> I'm fairly tempted to fix all the places we can without adding a second qi
> to nsAccessible.  Then we can do the reorg of msaa and ia2 and then finish
> adding these checks as we decom the last few classes.  Note that all the
> xpcom methods we call from the interfaces should be safe any  way at least
> in theory.

I don't like it either, shouldn't be noticeable but perf hit. It's not necessary until you dexcpom table interfaces. It will go away after MSAA/IA2 reorg (in particular all IsDefunct checks), at least I don't want to keep internal objects alive if they are ready to die (I can see potential usage, for example, out-of-context events handling but we don't do that nowdays and maybe we won't do that in future). If you like then you can address this problem as part of table dexpcomination work. But please make sure you're consistent for IAccessibleTable and IAccessibleTableCell implementations.
Comment 10 Trevor Saunders (:tbsaunde) 2011-08-26 06:15:28 PDT
I really doubt I'll have time to work on this for a while :(
Comment 11 alexander :surkov 2011-08-28 22:57:25 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> I really doubt I'll have time to work on this for a while :(

in either case we should get an agreement on it, not depending on who's going to finish it.
Comment 12 Trevor Saunders (:tbsaunde) 2011-09-03 18:13:13 PDT
(In reply to alexander surkov from comment #11)

I would tend to favor leaving the table interfaces as they are until we do the table decom stuff.
Comment 13 alexander :surkov 2012-02-07 20:55:29 PST
Jignesh, if one day you like to pick up another big a11y bug then please keep this one in mind.
Comment 14 alexander :surkov 2012-04-09 01:23:26 PDT
Created attachment 613249 [details] [diff] [review]
patch2

it's going to take forever, so let's shut it. just CO_E_OBJNOTCONNECTED fix (no args check) and no special do_QueryObject() calls for things like tables since you didn't like this.
Comment 15 alexander :surkov 2012-04-09 01:24:52 PDT
Comment on attachment 613249 [details] [diff] [review]
patch2

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

::: accessible/src/msaa/CAccessibleHyperlink.cpp
@@ +82,5 @@
>    if (aIndex < 0 || aIndex >= static_cast<long>(thisObj->AnchorCount()))
>      return E_INVALIDARG;
>  
> +  if (thisObj->IsLink())
> +    return S_FALSE;

misspelling, I'll fix these calls
Comment 16 Trevor Saunders (:tbsaunde) 2012-04-09 04:02:35 PDT
Comment on attachment 613249 [details] [diff] [review]
patch2

R=ME WITH iSlINK() CHECKS FIXED, AND iSdEFUNCT() CHECKS RESTORED FOR RETURN VALUE FROMgETxpaCCESSIBLEcHILD()
Comment 17 alexander :surkov 2012-04-09 05:46:37 PDT
Created attachment 613276 [details] [diff] [review]
patch3
Comment 19 :Ehsan Akhgari (out sick) 2012-04-10 08:28:48 PDT
https://hg.mozilla.org/mozilla-central/rev/74670610b919

Note You need to log in before you can comment on or make changes to this bug.