The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla25

Status

()

Core
Disability Access APIs
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: surkov, Assigned: poiru)

Tracking

(Blocks: 1 bug)

unspecified
mozilla25
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

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

Comment 1

9 years ago
Alexander, are you planning on doing something with this soon?
Mass un-assigning bugs assigned to Aaron.
Assignee: aaronleventhal → nobody
(Reporter)

Updated

6 years ago
Blocks: 389800
Whiteboard: [good first bug]

Comment 3

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

Comment 4

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

Comment 5

6 years ago
(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.

Updated

5 years ago
Whiteboard: [good first bug] → [good first bug][mentor=surkov.alexander@gmail.com]

Comment 6

5 years ago
Alexander: Could you add more information? What are the files of interest for this issue?
(Reporter)

Comment 7

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

5 years ago
Bug 539683 is already assigned to Trevor.
(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.
(Reporter)

Comment 10

5 years ago
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.
Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com] → [good first bug][mentor=surkov.alexander@gmail.com][lang=c++]

Comment 11

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

Comment 12

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

Comment 13

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

5 years ago
:'( !!
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.
(Reporter)

Comment 15

5 years ago
assigning to Arjun then
Assignee: nobody → junky.argonaut
Thanks a lot Oussama and surkov.

Comment 17

5 years ago
yw!
Go ahead Arjun, it's yours !!! :)
Thanks for providing me an opportunity Oussama. Really appreciate it :)
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
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.
(Reporter)

Comment 21

5 years ago
(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.
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?!
Attachment #629152 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 23

5 years ago
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)
Attachment #629152 - Flags: feedback?(surkov.alexander)
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.
Attachment #629152 - Attachment is obsolete: true
Attachment #630910 - Flags: review?(surkov.alexander)
(Reporter)

Comment 25

5 years ago
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
Attachment #630910 - Flags: review?(surkov.alexander)
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?
(Reporter)

Comment 27

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

if (!aName)
  return E_INVALIDARG;
*aName = NULL;
Created attachment 631721 [details] [diff] [review]
Patchv2
Attachment #630910 - Attachment is obsolete: true
Attachment #631721 - Flags: review?(surkov.alexander)
(Reporter)

Comment 29

5 years ago
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)
Attachment #631721 - Flags: review?(surkov.alexander)
Created attachment 633046 [details] [diff] [review]
patchv3

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

I haven't added a NULL check here have I?
Attachment #631721 - Attachment is obsolete: true
Attachment #633046 - Flags: review?(surkov.alexander)
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
Attachment #633046 - Flags: review?(surkov.alexander)
(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

5 years ago
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;
>     }
Attachment #633046 - Attachment is patch: true

Comment 34

4 years ago
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.
(Reporter)

Comment 35

4 years ago
(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.
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)
(Reporter)

Comment 37

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

4 years ago
Yes, I will take on this bug. Thank you.
(Reporter)

Updated

4 years ago
Assignee: junky.argonaut → lxrv22

Comment 39

4 years ago
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.
(Reporter)

Comment 40

4 years ago
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
lxrv22, will you be addressing Alexander's feedback?
Flags: needinfo?(lxrv22)

Comment 42

4 years ago
(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.
Flags: needinfo?(lxrv22)

Updated

4 years ago
Assignee: lxrv22 → nobody
(Assignee)

Comment 43

4 years ago
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).
Attachment #768460 - Flags: review?(surkov.alexander)
(Reporter)

Comment 44

4 years ago
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
Attachment #768460 - Flags: review?(surkov.alexander) → review+
(Reporter)

Updated

4 years ago
Assignee: nobody → birunthan
(Assignee)

Comment 45

4 years ago
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.
Attachment #768460 - Attachment is obsolete: true
Attachment #768729 - Flags: review?(surkov.alexander)
(Reporter)

Comment 46

4 years ago
Comment on attachment 768729 [details] [diff] [review]
Patch v2

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

nice work, r=me, thank you
Attachment #768729 - Flags: review?(surkov.alexander) → review+
(Reporter)

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Attachment #718288 - Attachment is obsolete: true

Updated

4 years ago
Attachment #633046 - Attachment is obsolete: true
Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/9933ac3e6915

Thanks for the patch!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9933ac3e6915
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.