Last Comment Bug 429990 - check in/out arguments on NULL in MSAA/IA2 methods
: check in/out arguments on NULL in MSAA/IA2 methods
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: mozilla25
Assigned To: Birunthan Mohanathas [:poiru]
:
:
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2008-04-20 22:32 PDT by alexander :surkov
Modified: 2013-06-28 18:33 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patchv0.1 (32.92 KB, patch)
2012-06-01 05:41 PDT, Nagarjuna Varma [:Infinity]
no flags Details | Diff | Splinter Review
Patchv1 (23.54 KB, patch)
2012-06-07 03:28 PDT, Nagarjuna Varma [:Infinity]
no flags Details | Diff | Splinter Review
Patchv2 (23.44 KB, patch)
2012-06-09 23:22 PDT, Nagarjuna Varma [:Infinity]
no flags Details | Diff | Splinter Review
patchv3 (18.75 KB, patch)
2012-06-13 23:46 PDT, Nagarjuna Varma [:Infinity]
no flags Details | Diff | Splinter Review
Proposed patch for all files in src/windows directory (38.76 KB, patch)
2013-02-25 23:35 PST, lxrv22
no flags Details | Diff | Splinter Review
Patch v1 (56.93 KB, patch)
2013-06-27 11:23 PDT, Birunthan Mohanathas [:poiru]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch v2 (55.64 KB, patch)
2013-06-27 21:02 PDT, Birunthan Mohanathas [:poiru]
surkov.alexander: review+
Details | Diff | Splinter Review

Description alexander :surkov 2008-04-20 22:32:30 PDT
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) 2008-04-24 22:41:57 PDT
Alexander, are you planning on doing something with this soon?
Comment 2 David Bolter [:davidb] 2009-06-16 12:00:59 PDT
Mass un-assigning bugs assigned to Aaron.
Comment 3 Javi Rueda 2011-07-11 19:32:50 PDT
Will try with this one. Not ready to be an assigned, though.
Comment 4 Marco Zehe (:MarcoZ) 2011-11-01 06:35:20 PDT
Javi, any progress, or did you decide to not work on it?
Comment 5 Javi Rueda 2011-11-01 12:36:12 PDT
(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 2011-12-01 15:35:03 PST
Alexander: Could you add more information? What are the files of interest for this issue?
Comment 7 alexander :surkov 2011-12-02 01:07:21 PST
(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 2011-12-02 08:33:32 PST
Bug 539683 is already assigned to Trevor.
Comment 9 Trevor Saunders (:tbsaunde) 2011-12-03 10:48:50 PST
(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 2012-04-09 07:49:49 PDT
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 2012-05-04 04:54:00 PDT
(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 2012-05-07 01:18:38 PDT
(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 2012-05-12 10:42:48 PDT
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 2012-05-13 04:20:57 PDT
:'( !!
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 2012-05-13 06:50:04 PDT
assigning to Arjun then
Comment 16 Nagarjuna Varma [:Infinity] 2012-05-15 07:21:00 PDT
Thanks a lot Oussama and surkov.
Comment 17 Oussama BADR 2012-05-15 07:56:07 PDT
yw!
Go ahead Arjun, it's yours !!! :)
Comment 18 Nagarjuna Varma [:Infinity] 2012-05-15 09:21:05 PDT
Thanks for providing me an opportunity Oussama. Really appreciate it :)
Comment 19 Mark Capella [:capella] 2012-05-22 09:14:13 PDT
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] 2012-05-30 06:21:10 PDT
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 2012-05-30 06:28:37 PDT
(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] 2012-06-01 05:41:58 PDT
Created attachment 629152 [details] [diff] [review]
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 2012-06-01 06:59:43 PDT
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] 2012-06-07 03:28:40 PDT
Created attachment 630910 [details] [diff] [review]
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 2012-06-07 05:37:02 PDT
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] 2012-06-08 12:59:50 PDT
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 2012-06-08 21:55:32 PDT
(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] 2012-06-09 23:22:30 PDT
Created attachment 631721 [details] [diff] [review]
Patchv2
Comment 29 alexander :surkov 2012-06-10 05:05:20 PDT
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] 2012-06-13 23:46:10 PDT
Created attachment 633046 [details] [diff] [review]
patchv3

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

I haven't added a NULL check here have I?
Comment 31 Trevor Saunders (:tbsaunde) 2012-06-16 04:06:54 PDT
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) 2012-06-16 04:07:48 PDT
(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 2012-07-12 07:54:23 PDT
Comment on attachment 633046 [details] [diff] [review]
patchv3

>From: Nagarjuna Varma <junky.argonaut@gmail.com>
>
>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 2013-02-13 09:18:50 PST
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 2013-02-13 16:17:56 PST
(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] 2013-02-14 04:21:20 PST
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 2013-02-14 15:19:17 PST
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 2013-02-14 19:27:25 PST
Yes, I will take on this bug. Thank you.
Comment 39 lxrv22 2013-02-25 23:35:43 PST
Created attachment 718288 [details] [diff] [review]
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 2013-02-27 01:26:45 PST
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] 2013-04-25 03:22:56 PDT
lxrv22, will you be addressing Alexander's feedback?
Comment 42 lxrv22 2013-04-25 12:20:02 PDT
(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 Birunthan Mohanathas [:poiru] 2013-06-27 11:23:13 PDT
Created attachment 768460 [details] [diff] [review]
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 2013-06-27 13:17:19 PDT
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 Birunthan Mohanathas [:poiru] 2013-06-27 21:02:26 PDT
Created attachment 768729 [details] [diff] [review]
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 2013-06-28 02:53:13 PDT
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) 2013-06-28 03:20:08 PDT
Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/9933ac3e6915

Thanks for the patch!
Comment 48 Phil Ringnalda (:philor) 2013-06-28 18:33:48 PDT
https://hg.mozilla.org/mozilla-central/rev/9933ac3e6915

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