Closed Bug 417018 Opened 13 years ago Closed 13 years ago

Array allocation mistakes in IA2

Categories

(Core :: Disability Access APIs, defect)

x86
Windows XP
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file)

We did not realize, but arrays in IA2 must be allocated by the IA2 server.
When the array to return is of size zero we must return null for the array and S_FALSE in the HRESULT.
The params such as aMaxRelations were a mistake and must be ignored.

This problem occurs in the following places:
- get_relations
- get_keyBinding same problems
- Also nsAccessibleRelationWrap::get_targets
CAccessibleTable::get_selectedChildren/columns/rows

Also in general we should return S_FALSE when there is no valid result to return and there was no error. For example, in IA2::attributes when there are no attributes.
(In reply to comment #0)

> - Also nsAccessibleRelationWrap::get_targets

Could you give me an idea how I can do it

 get_targets(long aMaxTargets, IUnknown **aTarget, long *aNTargets)

because here should be *** pointers because I see this like:
*aTarget = new Array();
IUnknown *punk;
(*aTarget)[index] = punk;
nsAccessibleWrap::get_relations(long aMaxRelations, IAccessibleRelation **aRelation, long *aNRelations) - the same
Attached patch some clean upSplinter Review
Attachment #303009 - Flags: review?(aaronleventhal)
Comment on attachment 303009 [details] [diff] [review]
some clean up

What about returning S_FALSE when there is nothing useful to return? I would expect this if an array size is 0 (for example for object attributes).
Comment on attachment 303009 [details] [diff] [review]
some clean up

> When the array to return is of size zero we must return null for the array and S_FALSE in the HRESULT.

I don't see that part yet.
Attachment #303009 - Flags: review?(aaronleventhal) → review-
Aaron, where did you catch these rules? Just for example MSDN reference says exactly what the method/function returns depending on the situation. What about IA2?

Also, would be nice to get your point for comment #1, #2 because as I get bug has been targeted on this originally.
Status: NEW → ASSIGNED
Comment on attachment 303009 [details] [diff] [review]
some clean up

bug 418025 is target for correct values of HRESULT. Lets deal here with arrays. To accomplish the bug it would be nice to get an  answer on comment #6.
Attachment #303009 - Flags: review- → review?(aaronleventhal)
Regarding comment 6, I got this info from Pete Brunet. He is clarifying the IDL now.
Why this?

CAccessibleAction::get_localizedName(long aActionIndex, BSTR *aLocalizedName)
 {
+  ::SysFreeString(*aLocalizedName);

And why all the SysAllocString -> SysReallocStringLen? Is that related to this bug?

- get_relations
- get_keyBinding same problems
- Also nsAccessibleRelationWrap::get_targets
CAccessibleTable::get_selectedChildren/columns/rows

I see the expected change to use arrary allocation only in GetSelectedItems(). What about the other places?
(In reply to comment #9)
> Why this?
> 
> CAccessibleAction::get_localizedName(long aActionIndex, BSTR *aLocalizedName)
>  {
> +  ::SysFreeString(*aLocalizedName);
> 

It's our analogue of aName.Truncate().

> And why all the SysAllocString -> SysReallocStringLen? Is that related to this
> bug?

No, for sure. I just run through code and fixed the work with this strings. Idea of string changes is to free strings if they were allocated previously.
(In reply to comment #10)

> > And why all the SysAllocString -> SysReallocStringLen? Is that related to this
> > bug?
> 
> No, for sure. I just run through code and fixed the work with this strings.
> Idea of string changes is to free strings if they were allocated previously.
> 

If you like then I'll move into separate bug.
(In reply to comment #9)

> - get_relations
> - get_keyBinding same problems
> - Also nsAccessibleRelationWrap::get_targets
> CAccessibleTable::get_selectedChildren/columns/rows
> 
> I see the expected change to use arrary allocation only in GetSelectedItems().
> What about the other places?
> 

Possibly I missed something but I found only one array containing simple types. Another array contains interfaces pointers and they are property of your comment #8.
I think you missed something. For example for get_targets we should not use the aMaxTargets, and  we need to create the array that we fill with pointers.

Attachment #303009 - Flags: review?(Evan.Yan)
Attachment #303009 - Flags: review?(Evan.Yan)
Aaron, should I remove array allocating code from the patch and save only strings stuff?
A note from Pete explains what we need to do:

> Aaron and Alexander, Just to bring this to closure.  The IDL doesn't
> need to be changed, at least for now.  
> 
> With the IDL the way it is now the arrays must be allocated on the 
> client side for these two methods which deal with arrays of interface pointers
>     IAccessible2::relations.
>     IARelation::targets. 
> 
> and the arrays must be allocated by the server for these 6 methods 
> that deal with arrays of longs and BSTRs
>     IAccessible2::extendedStates 
>     IAccessible2::localizedExtendedStates  
>     IAAction::keyBinding
>     IATable::selectedChildren
>     IATable::selectedColumns
>     IATable::selectedRows 
> 
> For the last 6 the first max [in] parameter will be ignored and 
> CoMemTaskFree must be used by the client to free the array.

So, Alexander, do we need to change any code?
Yes, we need for CAccessibleTable::GetSelectedItems which is used by selectedChildren/selectedColumns/selectedRows. The patch makes it.
Attachment #303009 - Flags: review?(aaronleventhal)
Attachment #303009 - Flags: review+
Attachment #303009 - Flags: approval1.9?
Comment on attachment 303009 [details] [diff] [review]
some clean up

a1.9=beltzner
Attachment #303009 - Flags: approval1.9? → approval1.9+
checked in

Checking in accessible/src/msaa/CAccessibleAction.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleAction.cpp,v  <--  CAccessibleAction.cpp
new revision: 1.7; previous revision: 1.6
done
Checking in accessible/src/msaa/CAccessibleTable.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleTable.cpp,v  <--  CAccessibleTable.cpp
new revision: 1.9; previous revision: 1.8
done
Checking in accessible/src/msaa/nsAccessibleRelationWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessibleRelationWrap.cpp,v  <--  nsAccessibleRelationWrap.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in accessible/src/msaa/nsAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessibleWrap.cpp,v  <--  nsAccessibleWrap.cpp
new revision: 1.108; previous revision: 1.107
done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(In reply to comment #11)
> (In reply to comment #10)
> 
> > > And why all the SysAllocString -> SysReallocStringLen? Is that related to this
> > > bug?

This changed seems to have caused bug 421650. I'm not sure why. Perhaps we cannot assume they are passing in an initialized pointer.

> > No, for sure. I just run through code and fixed the work with this strings.
> > Idea of string changes is to free strings if they were allocated previously.

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