429990 - check in/out arguments on NULL in MSAA/IA2 methods

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

the spun of bug 429650. It seems we get crashed due to incorrect usage of our methods. We should process them correctly.

Comment 1

[Marco Zehe (:MarcoZ)]

Alexander, are you planning on doing something with this soon?

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Mass un-assigning bugs assigned to Aaron.

Comment 3

[Javi Rueda :javirid]

Will try with this one. Not ready to be an assigned, though.

Comment 4

[Marco Zehe (:MarcoZ)]

Javi, any progress, or did you decide to not work on it?

Comment 5

[Javi Rueda :javirid]

(In reply to Marco Zehe (:MarcoZ) from comment #4)
> Javi, any progress, or did you decide to not work on it?

Sorry. No, I am not working on it. I should have stated that anyone could take it if wanted. Feel free to take it.

Comment 6

[Michael Aro]

Alexander: Could you add more information? What are the files of interest for this issue?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Michael Aro from comment #6)
> Alexander: Could you add more information? What are the files of interest
> for this issue?

They all are located in mssa folder (http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/), nsAccessNodeWrap/nsAccessibleWrap and all classes having 'C' prefix. If you're willing to work on this bug then I'd suggest instead to finish the patch from bug 539683 which contains a fix for this bug.

Comment 8

[Michael Aro]

Bug 539683 is already assigned to Trevor.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to Michael Aro from comment #8)
> Bug 539683 is already assigned to Trevor.

that was a pipe dream of Alex's, I'm not assigned anymore.  I might work on it at some point, but I'd rather spend time on other things, so I'd love to see someone else take it.

Comment 10

[alexander :surkov (:asurkov)]

ok, bug 539683 was landed without in-out arguments checks so feel free to pick up this bug (you can refer to original patch of 539683 bug to see how you can fix this bug).

Basically the fix is described by comment #7.

Comment 11

[Oussama BADR]

(In reply to alexander :surkov from comment #7)


> ... If
> you're willing to work on this bug then I'd suggest instead to finish the
> patch from bug 539683 which contains a fix for this bug.

that means we should forget this bug until the bug 539683 will be fixed correctly!! 
though, bug 539683 is already fixed, what's wrong with? and what kind of crash was expected with it?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Ouss. BADR from comment #11)
> that means we should forget this bug until the bug 539683 will be fixed
> correctly!! 
> though, bug 539683 is already fixed, what's wrong with?

check out the comment #10. originally I planned to get this bug fixed as part of bug 539683 but we didn't do that. So you need to add null check for every MSAA/IA2 method argument of every class located in mssa folder (http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/), i.e. classes postfixed by 'wrap' and all classes having 'C' prefix.

> and what kind of
> crash was expected with it?

when somebody calls MSAA/IA2 method with NULL out argument

Comment 13

[alexander :surkov (:asurkov)]

Oussama, are you working on it? If not then we could find something else for you when you get some time and let Arjun to work on this one.

Comment 14

[Oussama BADR]

:'( !!
Not yet Alexander, I'm trying to do so, but I'm not wholly in the bath!!


> we could find something else for you when you get some time let Arjun to work on this one.
If you look so, it's OK for me.

Comment 15

[alexander :surkov (:asurkov)]

assigning to Arjun then

Comment 16

[Nagarjuna Varma [:Infinity]]

Thanks a lot Oussama and surkov.

Comment 17

[Oussama BADR]

yw!
Go ahead Arjun, it's yours !!! :)

Comment 18

[Nagarjuna Varma [:Infinity]]

Thanks for providing me an opportunity Oussama. Really appreciate it :)

Comment 19

[Mark Capella [:capella]]

Infinity, as per IRC, fyi...

yah ... thats the example im looking for .... i assume

1) you can make a list of the files to change per comment # 12
2) you can examine each method in each file to determine which parameters are OUT parameters
3) you can ensure that each OUT parameter has a NULL assignment / check by that we mean something like this:
http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/CAccessibleTableCell.cpp#230

each parm is set to a null value (usually zero, sometimes -1, sometimes "false", etc.) before the method executes
so that if the method ends no matter what, the caller has valid data in the interface parameters

Comment 20

[Nagarjuna Varma [:Infinity]]

In http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/CAccessibleText.h#120 do I need to initialize the parameter (*aMinimumValue) in the function because the function is anyway set to a default value..

Also in http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/CAccessibleEditableText.cpp#139
here should i initialize *aAttributes to NULL?

There are many such similar functions in different files. Could you advise me on whether I should or should not intialize the parameters here to NULL or 0?

For these functions there are no comments in the header file specifying if they are OUT/IN parameters and all these functions are set to a default value in the declaration in the header file.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Arjun from comment #20)
> In
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/
> CAccessibleText.h#120 do I need to initialize the parameter (*aMinimumValue)
> in the function because the function is anyway set to a default value..

I don't see aMinimumValue there.

> Also in
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/
> CAccessibleEditableText.cpp#139
> here should i initialize *aAttributes to NULL?

no, it's in argument (http://mxr.mozilla.org/mozilla-central/source/other-licenses/ia2/AccessibleEditableText.idl#196)

> There are many such similar functions in different files. Could you advise
> me on whether I should or should not intialize the parameters here to NULL
> or 0?

in/inout arguments shouldn't be initialized, out args should be. You can refer to interface declaration to see which arg is which or figure our that from method implementation or ask :)

> For these functions there are no comments in the header file specifying if
> they are OUT/IN parameters and all these functions are set to a default
> value in the declaration in the header file.

yep, check spec in that case.

Comment 22

[Nagarjuna Varma [:Infinity]]

Attachment: Patchv0.1

I have a few doubts in some places as to whether I have to assign the null value to the parameters as I'm not sure if they are OUT parameters. I have marked them as "doubt" in the patch. Could you tell me what they are so I can fix them. Also could you point out if I have made any mistakes or left any parameters out so that I can fix them?!

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 629152 [details] [diff] [review]
Patchv0.1

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

::: accessible/src/msaa/ApplicationAccessibleWrap.cpp
@@ +149,3 @@
>  {
>  __try {
> +  *aName = NULL;

you should check also wither aName is not NULL before assinging, here and everywhere,
perhaps that's why some methods are missed like get_accName

::: accessible/src/msaa/CAccessibleEditableText.cpp
@@ +97,5 @@
>    return E_FAIL;
>  }
>  
>  STDMETHODIMP
> +CAccessibleEditableText::insertText(long aOffset, BSTR *aText)            

whitespaces in the end of line aren't allowed

@@ +146,5 @@
>  }
>  
>  STDMETHODIMP
>  CAccessibleEditableText::replaceText(long aStartOffset, long aEndOffset,
> +                                     BSTR *aText)                                

please do type* name

::: accessible/src/msaa/CAccessibleTable.cpp
@@ +61,5 @@
>  
>  // IUnknown
>  
>  STDMETHODIMP
> +CAccessibleTable::QueryInterface(REFIID iid, void** ppv)  //should i initialise iid

no

::: accessible/src/msaa/CAccessibleText.cpp
@@ +532,1 @@
>    return GetModifiedText(true, aNewText);

GetModifiedText can take care about arguments

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +420,5 @@
>  {
>    if (!aNode)
>      return NULL;
>  
> +  *aNode = NULL;

nope, when in doubt then take a look at code, if you see *aNode = something then it's out arg, otherwise it's in arg.

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +446,5 @@
>  }
>  
>  STDMETHODIMP nsAccessibleWrap::get_accState(
>        /* [optional][in] */ VARIANT varChild,
> +      /* [retval][out] */ VARIANT __RPC_FAR *pvarState)               //should I change?

you should null check it, here and below

::: accessible/src/msaa/nsHTMLWin32ObjectAccessible.cpp
@@ +125,2 @@
>  {
> +  *aNativeAccessible = NULL;

you could not worry about this particular method since it goes away but if you like then you need to null check it and assing a null as you did

::: accessible/src/msaa/nsHyperTextAccessibleWrap.cpp
@@ +50,5 @@
>                           ia2AccessibleHypertext,
>                           CAccessibleEditableText);
>  
>  nsresult
> +nsHyperTextAccessibleWrap::HandleAccEvent(AccEvent* aEvent)           //doubt

in arg

::: accessible/src/msaa/nsXULListboxAccessibleWrap.cpp
@@ +43,5 @@
>  ////////////////////////////////////////////////////////////////////////////////
>  
>  nsXULListboxAccessibleWrap::
>    nsXULListboxAccessibleWrap(nsIContent* aContent, nsDocAccessible* aDoc) :
> +  nsXULListboxAccessible(aContent, aDoc)                                           

please remove all whitespaces changes you introduced (it makes sense to setup your editor to make such changes impossible)

::: accessible/tests/mochitest/tree/test_img.html
@@ +30,5 @@
>            }
>          ]
>        };
>  
> +      ensureImageMapTree("imgmap");

it's from another bug? (btw, I thought it was landed, you need to update your local copy to trunk)

Comment 24

[Nagarjuna Varma [:Infinity]]

Attachment: Patchv1

Took a lot of time as I had to get used to MQ ( lost some data in the process)
But finally I did end up using it thanks to capella 
All the changes requested in the previous review have been incorporated here.

Comment 25

[alexander :surkov (:asurkov)]

Comment on attachment 630910 [details] [diff] [review]
Patchv1

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

::: accessible/src/msaa/ApplicationAccessibleWrap.cpp
@@ +99,5 @@
>  STDMETHODIMP
>  ApplicationAccessibleWrap::get_appName(BSTR* aName)
>  {
>  __try {
> +  if(*aName != NULL)

check aName, btw, it's out argument then *aName is likely NULL, please fix other comments/places before I continue review

::: accessible/src/msaa/CAccessibleEditableText.cpp
@@ +151,2 @@
>  {
>  __try {

no NULL check, here and below

::: accessible/src/msaa/CAccessibleTable.cpp
@@ +474,4 @@
>  {
>  __try {
> +  *aChildren = 0;
> +  *aNChildren = 0;

there's no NULL checks

Comment 26

[Nagarjuna Varma [:Infinity]]

I don not quite understand what you mean by "check aName, btw, it's out argument then *aName is likely NULL, please fix other comments/places before I continue revie" in your previous comment. In the earlier review you said that -- "you should check also wither aName is not NULL before assinging, here and everywhere"
So what am I supposed to do here regarding aName?

Comment 27

[alexander :surkov (:asurkov)]

(In reply to Arjun from comment #26)
> So what am I supposed to do here regarding aName?

if (!aName)
  return E_INVALIDARG;
*aName = NULL;

Comment 28

[Nagarjuna Varma [:Infinity]]

Attachment: Patchv2

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 631721 [details] [diff] [review]
Patchv2

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

::: accessible/src/msaa/CAccessibleEditableText.cpp
@@ +151,2 @@
>  {
>  __try {

there's no check for this one

@@ +169,5 @@
>  }
>  
>  STDMETHODIMP
>  CAccessibleEditableText::setAttributes(long aStartOffset, long aEndOffset,
> +                                       BSTR *aAttributes)                        

type* name (and in other files), 

also add a check for it (also in other files)

Comment 30

[Nagarjuna Varma [:Infinity]]

Attachment: patchv3

@@ +151,2 @@
>  {
>  __try {

I haven't added a NULL check here have I?

Comment 31

[Trevor Saunders (:tbsaunde)]

Comment on attachment 633046 [details] [diff] [review]
patchv3

> ApplicationAccessibleWrap::get_toolkitName(BSTR* aName)
> {
> __try {
>+  if (!aName)
>+    return E_INVALIDARG;
>+  *aName = NULL;

nit, blank line after if

> CAccessibleEditableText::replaceText(long aStartOffset, long aEndOffset,
>-                                     BSTR *aText)
>+                                     BSTR* aText)
> {
> __try {

you should check aText is not null

> CAccessibleEditableText::setAttributes(long aStartOffset, long aEndOffset,
>-                                       BSTR *aAttributes)
>+                                       BSTR* aAttributes)
> {
> __try {
>+  *aAttributes = NULL;

its in argument not out you should  null check, but don't change

> CAccessibleTable::get_selectedColumns(long aMaxColumns, long **aColumns,
>                                       long *aNColumns)
> {
> __try {
>+  *aColumns = 0;
>+  *aNColumns = 0;

you should null check them first

> CAccessibleTable::get_selectedRows(long aMaxRows, long **aRows, long *aNRows)
> {
> __try {
>+  *aRows = 0;
>+  *aNRows = 0;

same here and a bunch of places below.

> CAccessibleTable::get_modelChange(IA2TableModelChange *aModelChange)
> {
> __try {
>+  *aModelChange = NULL;

same

Comment 32

[Trevor Saunders (:tbsaunde)]

(In reply to Arjun from comment #30)
> Created attachment 633046 [details] [diff] [review]
> patchv3
> 
> @@ +151,2 @@
> >  {
> >  __try {
> 
> I haven't added a NULL check here have I?

its hard to tell what you talk about with that little context, but probably not.  However the point is you should add null checks.

Comment 33

[Takanori MATSUURA]

Comment on attachment 633046 [details] [diff] [review]
patchv3

>From: Nagarjuna Varma <[e-mail]>
>
>diff --git a/accessible/src/msaa/ApplicationAccessibleWrap.cpp b/accessible/src/msaa/ApplicationAccessibleWrap.cpp
>--- a/accessible/src/msaa/ApplicationAccessibleWrap.cpp
>+++ b/accessible/src/msaa/ApplicationAccessibleWrap.cpp
>@@ -95,16 +95,18 @@ ApplicationAccessibleWrap::QueryInterfac
> 
> ////////////////////////////////////////////////////////////////////////////////
> // IAccessibleApplication
> 
> STDMETHODIMP
> ApplicationAccessibleWrap::get_appName(BSTR* aName)
> {
> __try {
>+  if (!aName)
>+    return E_INVALIDARG;
>   *aName = NULL;
> 
>   if (IsDefunct())
>     return CO_E_OBJNOTCONNECTED;
> 
>   nsAutoString name;
>   nsresult rv = GetAppName(name);
>   if (NS_FAILED(rv))
>@@ -143,16 +145,20 @@ ApplicationAccessibleWrap::get_appVersio
> } __except(FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
>   return E_FAIL;
> }
> 
> STDMETHODIMP
> ApplicationAccessibleWrap::get_toolkitName(BSTR* aName)
> {
> __try {
>+  if (!aName)
>+    return E_INVALIDARG;
>+  *aName = NULL;
>+
>   if (IsDefunct())
>     return CO_E_OBJNOTCONNECTED;
> 
>   nsAutoString name;
>   nsresult rv = GetPlatformName(name);
>   if (NS_FAILED(rv))
>     return GetHRESULT(rv);
> 
>diff --git a/accessible/src/msaa/CAccessibleEditableText.cpp b/accessible/src/msaa/CAccessibleEditableText.cpp
>--- a/accessible/src/msaa/CAccessibleEditableText.cpp
>+++ b/accessible/src/msaa/CAccessibleEditableText.cpp
>@@ -142,17 +142,17 @@ CAccessibleEditableText::pasteText(long 
>   return GetHRESULT(rv);
> 
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
>   return E_FAIL;
> }
> 
> STDMETHODIMP
> CAccessibleEditableText::replaceText(long aStartOffset, long aEndOffset,
>-                                     BSTR *aText)
>+                                     BSTR* aText)
> {
> __try {
>   nsRefPtr<nsHyperTextAccessible> textAcc(do_QueryObject(this));
>   if (textAcc->IsDefunct())
>     return CO_E_OBJNOTCONNECTED;
> 
>   nsresult rv = textAcc->DeleteText(aStartOffset, aEndOffset);
>   if (NS_FAILED(rv))
>@@ -165,15 +165,16 @@ CAccessibleEditableText::replaceText(lon
>   return GetHRESULT(rv);
> 
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
>   return E_FAIL;
> }
> 
> STDMETHODIMP
> CAccessibleEditableText::setAttributes(long aStartOffset, long aEndOffset,
>-                                       BSTR *aAttributes)
>+                                       BSTR* aAttributes)
> {
> __try {
>+  *aAttributes = NULL;
> 
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
>   return E_NOTIMPL;
> }
>diff --git a/accessible/src/msaa/CAccessibleTable.cpp b/accessible/src/msaa/CAccessibleTable.cpp
>--- a/accessible/src/msaa/CAccessibleTable.cpp
>+++ b/accessible/src/msaa/CAccessibleTable.cpp
>@@ -480,27 +480,33 @@ CAccessibleTable::get_selectedChildren(l
>   return E_FAIL;
> }
> 
> STDMETHODIMP
> CAccessibleTable::get_selectedColumns(long aMaxColumns, long **aColumns,
>                                       long *aNColumns)
> {
> __try {
>+  *aColumns = 0;
>+  *aNColumns = 0;
>+
>   return GetSelectedItems(aColumns, aNColumns, ITEMSTYPE_COLUMNS);
> 
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(),
>                                                   GetExceptionInformation())) {}
>   return E_FAIL;
> }
> 
> STDMETHODIMP
> CAccessibleTable::get_selectedRows(long aMaxRows, long **aRows, long *aNRows)
> {
> __try {
>+  *aRows = 0;
>+  *aNRows = 0;
>+
>   return GetSelectedItems(aRows, aNRows, ITEMSTYPE_ROWS);
> 
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(),
>                                                   GetExceptionInformation())) {}
>   return E_FAIL;
> }
> 
> STDMETHODIMP
>@@ -700,40 +706,46 @@ CAccessibleTable::get_rowColumnExtentsAt
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
>   return E_FAIL;
> }
> 
> STDMETHODIMP
> CAccessibleTable::get_modelChange(IA2TableModelChange *aModelChange)
> {
> __try {
>+  *aModelChange = NULL;
> 
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
>   return E_NOTIMPL;
> }
> 
> ////////////////////////////////////////////////////////////////////////////////
> // IAccessibleTable2
> 
> STDMETHODIMP
> CAccessibleTable::get_cellAt(long row, long column, IUnknown **cell)
> {
>+  *cell = NULL;
>   return get_accessibleAt(row, column, cell);
> }
> 
> STDMETHODIMP
> CAccessibleTable::get_nSelectedCells(long *cellCount)
> {
>+  *cellCount = 0;
>   return get_nSelectedChildren(cellCount);
> }
> 
> STDMETHODIMP
> CAccessibleTable::get_selectedCells(IUnknown ***cells, long *nSelectedCells)
> {
> __try {
>+  *cells = NULL;
>+  *nSelectedCells = 0;
>+
>   nsCOMPtr<nsIAccessibleTable> tableAcc(do_QueryObject(this));
>   NS_ASSERTION(tableAcc, CANT_QUERY_ASSERTION_MSG);
>   if (!tableAcc)
>     return E_FAIL;
> 
>   nsCOMPtr<nsIArray> geckoCells;
>   nsresult rv = tableAcc->GetSelectedCells(getter_AddRefs(geckoCells));
>   if (NS_FAILED(rv))
>@@ -746,27 +758,31 @@ CAccessibleTable::get_selectedCells(IUnk
> 
>   return E_FAIL;
> }
> 
> STDMETHODIMP
> CAccessibleTable::get_selectedColumns(long **aColumns, long *aNColumns)
> {
> __try {
>+  *aColumns = 0;
>+  *aNColumns = 0;
>   return GetSelectedItems(aColumns, aNColumns, ITEMSTYPE_COLUMNS);
> 
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(),
>                                                   GetExceptionInformation())) {}
>   return E_FAIL;
> }
> 
> STDMETHODIMP
> CAccessibleTable::get_selectedRows(long **aRows, long *aNRows)
> {
> __try {
>+  *aRows = 0;
>+  *aNRows = 0;
>   return GetSelectedItems(aRows, aNRows, ITEMSTYPE_ROWS);
> 
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(),
>                                                   GetExceptionInformation())) {}
>   return E_FAIL;
> }
> 
> ////////////////////////////////////////////////////////////////////////////////
>diff --git a/accessible/src/msaa/CAccessibleTableCell.cpp b/accessible/src/msaa/CAccessibleTableCell.cpp
>--- a/accessible/src/msaa/CAccessibleTableCell.cpp
>+++ b/accessible/src/msaa/CAccessibleTableCell.cpp
>@@ -74,16 +74,18 @@ CAccessibleTableCell::QueryInterface(REF
> 
> ////////////////////////////////////////////////////////////////////////////////
> // IAccessibleTableCell
> 
> STDMETHODIMP
> CAccessibleTableCell::get_table(IUnknown **table)
> {
> __try {
>+  *table = NULL;
>+
>   nsCOMPtr<nsIAccessibleTableCell> tableCell(do_QueryObject(this));
>   NS_ASSERTION(tableCell, TABLECELL_INTERFACE_UNSUPPORTED_MSG);
>   if (!tableCell)
>     return E_FAIL;
> 
>   nsCOMPtr<nsIAccessibleTable> geckoTable;
>   nsresult rv = tableCell->GetTable(getter_AddRefs(geckoTable));
>   if (NS_FAILED(rv))
>@@ -133,16 +135,19 @@ CAccessibleTableCell::get_columnExtent(l
>   return E_FAIL;
> }
> 
> STDMETHODIMP
> CAccessibleTableCell::get_columnHeaderCells(IUnknown ***cellAccessibles,
>                                             long *nColumnHeaderCells)
> {
> __try {
>+  *cellAccessibles = NULL;
>+  *nColumnHeaderCells = 0;
>+
>   nsCOMPtr<nsIAccessibleTableCell> tableCell(do_QueryObject(this));
>   NS_ASSERTION(tableCell, TABLECELL_INTERFACE_UNSUPPORTED_MSG);
>   if (!tableCell)
>     return E_FAIL;
> 
>   nsCOMPtr<nsIArray> headerCells;
>   nsresult rv = tableCell->GetColumnHeaderCells(getter_AddRefs(headerCells));  
>   if (NS_FAILED(rv))
>@@ -209,16 +214,19 @@ CAccessibleTableCell::get_rowExtent(long
>   return E_FAIL;
> }
> 
> STDMETHODIMP
> CAccessibleTableCell::get_rowHeaderCells(IUnknown ***cellAccessibles,
>                                          long *nRowHeaderCells)
> {
> __try {
>+  *cellAccessibles = NULL;
>+  *nRowHeaderCells = 0;
>+
>   nsCOMPtr<nsIAccessibleTableCell> tableCell(do_QueryObject(this));
>   NS_ASSERTION(tableCell, TABLECELL_INTERFACE_UNSUPPORTED_MSG);
>   if (!tableCell)
>     return E_FAIL;
> 
>   nsCOMPtr<nsIArray> headerCells;
>   nsresult rv = tableCell->GetRowHeaderCells(getter_AddRefs(headerCells));  
>   if (NS_FAILED(rv))
>diff --git a/accessible/src/msaa/CAccessibleText.cpp b/accessible/src/msaa/CAccessibleText.cpp
>--- a/accessible/src/msaa/CAccessibleText.cpp
>+++ b/accessible/src/msaa/CAccessibleText.cpp
>@@ -523,38 +523,41 @@ CAccessibleText::scrollSubstringToPoint(
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
>   return E_FAIL;
> }
> 
> STDMETHODIMP
> CAccessibleText::get_newText(IA2TextSegment *aNewText)
> {
> __try {
>+  *aNewText = NULL;
>   return GetModifiedText(true, aNewText);
> 
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
>   return E_FAIL;
> }
> 
> STDMETHODIMP
> CAccessibleText::get_oldText(IA2TextSegment *aOldText)
> {
> __try {
>+  *aOldText = NULL;
>   return GetModifiedText(false, aOldText);
> 
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
>   return E_FAIL;
> }
> 
> // CAccessibleText
> 
> HRESULT
> CAccessibleText::GetModifiedText(bool aGetInsertedText,
>                                  IA2TextSegment *aText)
> {
>+  *aText = NULL;
>   PRUint32 startOffset = 0, endOffset = 0;
>   nsAutoString text;
> 
>   nsresult rv = GetModifiedText(aGetInsertedText, text,
>                                 &startOffset, &endOffset);
>   if (NS_FAILED(rv))
>     return GetHRESULT(rv);
> 
>diff --git a/accessible/src/msaa/ia2AccessibleAction.cpp b/accessible/src/msaa/ia2AccessibleAction.cpp
>--- a/accessible/src/msaa/ia2AccessibleAction.cpp
>+++ b/accessible/src/msaa/ia2AccessibleAction.cpp
>@@ -174,16 +174,18 @@ ia2AccessibleAction::get_keyBinding(long
> } __except(nsAccessNodeWrap::FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
>   return E_FAIL;
> }
> 
> STDMETHODIMP
> ia2AccessibleAction::get_name(long aActionIndex, BSTR *aName)
> {
> __try {
>+  if (!aName)
>+    return E_INVALIDARG;
>   *aName = NULL;
> 
>   nsAccessibleWrap* acc = static_cast<nsAccessibleWrap*>(this);
>   if (acc->IsDefunct())
>     return CO_E_OBJNOTCONNECTED;
> 
>   nsAutoString name;
>   PRUint8 index = static_cast<PRUint8>(aActionIndex);
>diff --git a/accessible/src/msaa/nsAccessNodeWrap.cpp b/accessible/src/msaa/nsAccessNodeWrap.cpp
>--- a/accessible/src/msaa/nsAccessNodeWrap.cpp
>+++ b/accessible/src/msaa/nsAccessNodeWrap.cpp
>@@ -87,16 +87,17 @@ NS_IMPL_ISUPPORTS_INHERITED1(nsAccessNod
> 
> //-----------------------------------------------------
> // nsIWinAccessNode methods
> //-----------------------------------------------------
> 
> NS_IMETHODIMP
> nsAccessNodeWrap::QueryNativeInterface(REFIID aIID, void** aInstancePtr)
> {
>+  *aInstancePtr = nsnull;
>   return QueryInterface(aIID, aInstancePtr);
> }
> 
> //-----------------------------------------------------
> // IUnknown interface methods - see iunknown.h for documentation
> //-----------------------------------------------------
> 
> STDMETHODIMP nsAccessNodeWrap::QueryInterface(REFIID iid, void** ppv)
>@@ -208,17 +209,21 @@ STDMETHODIMP nsAccessNodeWrap::get_nodeI
>     /* [out] */ short __RPC_FAR *aNameSpaceID,
>     /* [out] */ BSTR __RPC_FAR *aNodeValue,
>     /* [out] */ unsigned int __RPC_FAR *aNumChildren,
>     /* [out] */ unsigned int __RPC_FAR *aUniqueID,
>     /* [out] */ unsigned short __RPC_FAR *aNodeType)
> {
> __try{
>   *aNodeName = nsnull;
>+  *aNameSpaceID = 0;
>   *aNodeValue = nsnull;
>+  *aNumChildren = 0;
>+  *aUniqueID = 0;
>+  *aNodeType = 0;
> 
>   nsINode* node = GetNode();
>   if (!node)
>     return E_FAIL;
> 
>   nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(node));
> 
>   PRUint16 nodeType = 0;
>@@ -256,16 +261,19 @@ STDMETHODIMP nsAccessNodeWrap::get_nodeI
> STDMETHODIMP nsAccessNodeWrap::get_attributes( 
>     /* [in] */ unsigned short aMaxAttribs,
>     /* [length_is][size_is][out] */ BSTR __RPC_FAR *aAttribNames,
>     /* [length_is][size_is][out] */ short __RPC_FAR *aNameSpaceIDs,
>     /* [length_is][size_is][out] */ BSTR __RPC_FAR *aAttribValues,
>     /* [out] */ unsigned short __RPC_FAR *aNumAttribs)
> {
> __try{
>+  *aAttribNames = nsnull;
>+  *aAttribValues = nsnull;
>+  *aNameSpaceIDs = 0;
>   *aNumAttribs = 0;
> 
>   if (!mContent || IsDocumentNode())
>     return E_FAIL;
> 
>   PRUint32 numAttribs = mContent->GetAttrCount();
>   if (numAttribs > aMaxAttribs)
>     numAttribs = aMaxAttribs;
>@@ -329,16 +337,18 @@ STDMETHODIMP nsAccessNodeWrap::get_attri
> STDMETHODIMP nsAccessNodeWrap::get_computedStyle( 
>     /* [in] */ unsigned short aMaxStyleProperties,
>     /* [in] */ boolean aUseAlternateView,
>     /* [length_is][size_is][out] */ BSTR __RPC_FAR *aStyleProperties,
>     /* [length_is][size_is][out] */ BSTR __RPC_FAR *aStyleValues,
>     /* [out] */ unsigned short __RPC_FAR *aNumStyleProperties)
> {
> __try{
>+  *aStyleValues = nsnull;
>+  *aStyleProperties = nsnull;
>   *aNumStyleProperties = 0;
> 
>   if (!mContent || IsDocumentNode())
>     return E_FAIL;
> 
>   nsCOMPtr<nsIDOMCSSStyleDeclaration> cssDecl =
>     nsWinUtils::GetComputedStyleDeclaration(mContent);
>   NS_ENSURE_TRUE(cssDecl, E_FAIL);
>@@ -366,16 +376,17 @@ STDMETHODIMP nsAccessNodeWrap::get_compu
> 
> STDMETHODIMP nsAccessNodeWrap::get_computedStyleForProperties( 
>     /* [in] */ unsigned short aNumStyleProperties,
>     /* [in] */ boolean aUseAlternateView,
>     /* [length_is][size_is][in] */ BSTR __RPC_FAR *aStyleProperties,
>     /* [length_is][size_is][out] */ BSTR __RPC_FAR *aStyleValues)
> {
> __try {
>+  *aStyleValues = nsnull;
>   if (!mContent || IsDocumentNode())
>     return E_FAIL;
>  
>   nsCOMPtr<nsIDOMCSSStyleDeclaration> cssDecl =
>     nsWinUtils::GetComputedStyleDeclaration(mContent);
>   NS_ENSURE_TRUE(cssDecl, E_FAIL);
> 
>   PRUint32 index;
>@@ -438,72 +449,77 @@ nsAccessNodeWrap::MakeAccessNode(nsINode
> 
>   return iNode;
> }
> 
> 
> STDMETHODIMP nsAccessNodeWrap::get_parentNode(ISimpleDOMNode __RPC_FAR *__RPC_FAR *aNode)
> {
> __try {
>+  *aNode = NULL;
>   nsINode* node = GetNode();
>   if (!node)
>     return E_FAIL;
> 
>   *aNode = MakeAccessNode(node->GetNodeParent());
> 
> } __except(FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
> 
>   return S_OK;
> }
> 
> STDMETHODIMP nsAccessNodeWrap::get_firstChild(ISimpleDOMNode __RPC_FAR *__RPC_FAR *aNode)
> {
> __try {
>+  *aNode = NULL;
>   nsINode* node = GetNode();
>   if (!node)
>     return E_FAIL;
> 
>   *aNode = MakeAccessNode(node->GetFirstChild());
> 
> } __except(FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
> 
>   return S_OK;
> }
> 
> STDMETHODIMP nsAccessNodeWrap::get_lastChild(ISimpleDOMNode __RPC_FAR *__RPC_FAR *aNode)
> {
> __try {
>+  *aNode = NULL;
>   nsINode* node = GetNode();
>   if (!node)
>     return E_FAIL;
> 
>   *aNode = MakeAccessNode(node->GetLastChild());
> 
> } __except(FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
> 
>   return S_OK;
> }
> 
> STDMETHODIMP nsAccessNodeWrap::get_previousSibling(ISimpleDOMNode __RPC_FAR *__RPC_FAR *aNode)
> {
> __try {
>+  *aNode = NULL;
>   nsINode* node = GetNode();
>   if (!node)
>     return E_FAIL;
> 
>   *aNode = MakeAccessNode(node->GetPreviousSibling());
> 
> } __except(FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
> 
>   return S_OK;
> }
> 
> STDMETHODIMP nsAccessNodeWrap::get_nextSibling(ISimpleDOMNode __RPC_FAR *__RPC_FAR *aNode)
> {
> __try {
>+  *aNode = NULL;
>   nsINode* node = GetNode();
>   if (!node)
>     return E_FAIL;
> 
>   *aNode = MakeAccessNode(node->GetNextSibling());
> 
> } __except(FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
> 
>@@ -597,16 +613,17 @@ void nsAccessNodeWrap::ShutdownAccessibi
> 
>   nsWinUtils::ShutdownWindowEmulation();
> 
>   nsAccessNode::ShutdownXPAccessibility();
> }
> 
> int nsAccessNodeWrap::FilterA11yExceptions(unsigned int aCode, EXCEPTION_POINTERS *aExceptionInfo)
> {
>+  *aExceptionInfo = NULL;
>   if (aCode == EXCEPTION_ACCESS_VIOLATION) {
> #ifdef MOZ_CRASHREPORTER
>     // MSAA swallows crashes (because it is COM-based)
>     // but we still need to learn about those crashes so we can fix them
>     // Make sure to pass them to the crash reporter
>     nsCOMPtr<nsICrashReporter> crashReporter =
>       do_GetService("@mozilla.org/toolkit/crash-reporter;1");
>     if (crashReporter) {
>diff --git a/accessible/src/msaa/nsAccessibleWrap.cpp b/accessible/src/msaa/nsAccessibleWrap.cpp
>--- a/accessible/src/msaa/nsAccessibleWrap.cpp
>+++ b/accessible/src/msaa/nsAccessibleWrap.cpp
>@@ -445,16 +445,19 @@ STDMETHODIMP nsAccessibleWrap::get_accRo
>   return E_FAIL;
> }
> 
> STDMETHODIMP nsAccessibleWrap::get_accState(
>       /* [optional][in] */ VARIANT varChild,
>       /* [retval][out] */ VARIANT __RPC_FAR *pvarState)
> {
> __try {
>+  if (*pvarState != NULL)
>+    *pvarState = nsnull;
>+
>   VariantInit(pvarState);
>   pvarState->vt = VT_I4;
>   pvarState->lVal = 0;
> 
>   if (IsDefunct())
>     return CO_E_OBJNOTCONNECTED;
> 
>   nsAccessible* xpAccessible = GetXPAccessibleFor(varChild);
>@@ -546,16 +549,19 @@ STDMETHODIMP nsAccessibleWrap::get_accFo
> {
>   // VT_EMPTY:    None. This object does not have the keyboard focus itself
>   //              and does not contain a child that has the keyboard focus.
>   // VT_I4:       lVal is CHILDID_SELF. The object itself has the keyboard focus.
>   // VT_I4:       lVal contains the child ID of the child element with the keyboard focus.
>   // VT_DISPATCH: pdispVal member is the address of the IDispatch interface
>   //              for the child object with the keyboard focus.
> __try {
>+  if (*pvarChild != NULL)
>+    pvarChild = NULL;
>+
>   if (IsDefunct())
>     return CO_E_OBJNOTCONNECTED;
> 
>   VariantInit(pvarChild);
> 
>   // Return the current IAccessible child that has focus
>   nsAccessible* focusedAccessible = FocusedChild();
>   if (focusedAccessible == this) {
>@@ -639,16 +645,18 @@ AccessibleEnumerator::Release(void)
>     delete this;
>   return r;
> }
> 
> STDMETHODIMP
> AccessibleEnumerator::Next(unsigned long celt, VARIANT FAR* rgvar, unsigned long FAR* pceltFetched)
> {
> __try {
>+  rgvar = NULL;
>+  pceltFetched = 0;
>   PRUint32 length = 0;
>   mArray->GetLength(&length);
> 
>   HRESULT hr = S_OK;
> 
>   // Can't get more elements than there are...
>   if (celt > length - mCurIndex) {
>     hr = S_FALSE;
>@@ -1181,16 +1189,17 @@ nsAccessibleWrap::get_relations(long aMa
>                                 IAccessibleRelation **aRelation,
>                                 long *aNRelations)
> {
> __try {
>   if (!aRelation || !aNRelations)
>     return E_INVALIDARG;
> 
>   *aNRelations = 0;
>+  *aRelation = NULL;
> 
>   if (IsDefunct())
>     return CO_E_OBJNOTCONNECTED;
> 
>   for (PRUint32 relType = nsIAccessibleRelation::RELATION_FIRST;
>        relType <= nsIAccessibleRelation::RELATION_LAST &&
>        *aNRelations < aMaxRelations; relType++) {
>     Relation rel = RelationByType(relType);
>@@ -1480,16 +1489,17 @@ nsAccessibleWrap::get_locale(IA2Locale *
>   // Two-letter primary codes are reserved for [ISO639] language abbreviations.
>   // Any two-letter subcode is understood to be a [ISO3166] country code.
> 
>   if (IsDefunct())
>     return CO_E_OBJNOTCONNECTED;
> 
>   nsAutoString lang;
>   Language(lang);
>+  *aLocale = NULL;
> 
>   // If primary code consists from two letters then expose it as language.
>   PRInt32 offset = lang.FindChar('-', 0);
>   if (offset == -1) {
>     if (lang.Length() == 2) {
>       aLocale->language = ::SysAllocString(lang.get());
>       return S_OK;
>     }

Comment 34

[lxrv22]

Hi. I was wondering if this bug is still being worked on, I'd like to take on it for a school project. Also was wondering if it is available, if it could be completed in about 2 weeks.

Comment 35

[alexander :surkov (:asurkov)]

(In reply to lxrv22 from comment #34)
> Hi. I was wondering if this bug is still being worked on, I'd like to take
> on it for a school project. Also was wondering if it is available, if it
> could be completed in about 2 weeks.

Sounds good with me (the bug is inactive for half of year). Let's check with Nagarjuna if that's fine. 2 weeks should be even more than needed.

Comment 36

[Nagarjuna Varma [:Infinity]]

Yeah, its fine with me guys. I've been caught up with Performance analysis (sorry about the inactive period of the bug surkov - shoulda managed time and work better) and so will not be able to work on this bug any time this month :(
So, feel free and go wave your magic wand and fix this bug ;)
Thanks !!! (1 week of dedicated time is more than enough if you are new..don't sweat it)

Comment 37

[alexander :surkov (:asurkov)]

Thanks, Nagarjuna. You're welcome back. Let me know if you need a new bug.

lxrv22, should I assign this bug to you?

Comment 38

[lxrv22]

Yes, I will take on this bug. Thank you.

Comment 39

[lxrv22]

Attachment: Proposed patch for all files in src/windows directory

Here is a proposed patch for the bug only for files in the src/windows directory.  I wasn't sure which arguments were considered out arguments so I checked to see if every pointer argument passed in was null. Also, I gave the pointer a NULL value after the check if it wasn't passed in to any other method before being assigned a new value, but I did not assign it a null value if it was being passed into a method.

Comment 40

[alexander :surkov (:asurkov)]

Comment on attachment 718288 [details] [diff] [review]
Proposed patch for all files in src/windows directory

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

::: accessible/src/windows/ia2/ia2AccessibleAction.cpp
@@ +17,5 @@
>  
>  STDMETHODIMP
>  ia2AccessibleAction::QueryInterface(REFIID iid, void** ppv)
>  {
> +

nit: no empty line pls

::: accessible/src/windows/ia2/ia2AccessibleComponent.cpp
@@ +20,5 @@
>  
>  STDMETHODIMP
>  ia2AccessibleComponent::QueryInterface(REFIID iid, void** ppv)
>  {
> +

nit: no empty line

@@ +91,5 @@
>  STDMETHODIMP
>  ia2AccessibleComponent::get_foreground(IA2Color* aForeground)
>  {
> +  if (!aForeground)
> +    return E_INVALIDARG;

keep it under try block (below and in other files)

@@ +93,5 @@
>  {
> +  if (!aForeground)
> +    return E_INVALIDARG;
> +
> +  *aForeground = NULL;

0 since it's number

@@ +116,5 @@
>  {
> +  if (!aBackground)
> +    return E_INVALIDARG;
> +
> +  *aBackground = NULL;

same

Comment 41

[Josh Matthews [:jdm]]

lxrv22, will you be addressing Alexander's feedback?

Comment 42

[lxrv22]

(In reply to Josh Matthews [:jdm] from comment #41)
> lxrv22, will you be addressing Alexander's feedback?

Hello Josh Matthews. No I will no longer be addressing it. A few things have come up and I don't have the time to at the moment.

Comment 43

[Biru [:poiru]]

Attachment: Patch v1

I went through accessible/src/windows again (i.e. the attached patch is not based on any previous patches).

Comment 44

[alexander :surkov (:asurkov)]

Comment on attachment 768460 [details] [diff] [review]
Patch v1

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

::: accessible/src/windows/ia2/ia2AccessibleEditableText.cpp
@@ +52,5 @@
>  {
>    A11Y_TRYBLOCK_BEGIN
>  
> +  if (!aText)
> +    return E_INVALIDARG;

not needed, since aText is 'in' argument

@@ +104,5 @@
>  {
>    A11Y_TRYBLOCK_BEGIN
>  
> +  if (!aText)
> +    return E_INVALIDARG;

same

::: accessible/src/windows/msaa/DocAccessibleWrap.cpp
@@ +221,5 @@
>        /* [retval][out] */ BSTR __RPC_FAR *pszValue)
>  {
> +  if (!pszValue)
> +    return E_INVALIDARG;
> + 

nit: space at empty line

Comment 45

[Biru [:poiru]]

Attachment: Patch v2

(In reply to alexander :surkov from comment #44)
> Comment on attachment 768460 [details] [diff] [review]

Thanks, updated the patch.

Comment 46

[alexander :surkov (:asurkov)]

Comment on attachment 768729 [details] [diff] [review]
Patch v2

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

nice work, r=me, thank you

Comment 47

[Marco Zehe (:MarcoZ)]

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/9933ac3e6915

Thanks for the patch!

Comment 48

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9933ac3e6915