Array allocation mistakes in IA2

RESOLVED FIXED

Status

()

Core
Disability Access APIs
--
major
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Aaron Leventhal, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
(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;
(Assignee)

Comment 2

10 years ago
nsAccessibleWrap::get_relations(long aMaxRelations, IAccessibleRelation **aRelation, long *aNRelations) - the same
(Assignee)

Comment 3

10 years ago
Created attachment 303009 [details] [diff] [review]
some clean up
Attachment #303009 - Flags: review?(aaronleventhal)
(Reporter)

Comment 4

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

Comment 5

10 years ago
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-
(Assignee)

Comment 6

10 years ago
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
(Assignee)

Comment 7

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

Comment 8

10 years ago
Regarding comment 6, I got this info from Pete Brunet. He is clarifying the IDL now.
(Reporter)

Comment 9

10 years ago
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?
(Assignee)

Comment 10

10 years ago
(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.
(Assignee)

Comment 11

10 years ago
(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.
(Assignee)

Comment 12

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

Comment 13

10 years ago
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.

(Assignee)

Updated

10 years ago
Attachment #303009 - Flags: review?(Evan.Yan)
(Assignee)

Updated

10 years ago
Attachment #303009 - Flags: review?(Evan.Yan)
(Assignee)

Comment 14

10 years ago
Aaron, should I remove array allocating code from the patch and save only strings stuff?
(Reporter)

Comment 15

10 years ago
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?
(Assignee)

Comment 16

10 years ago
Yes, we need for CAccessibleTable::GetSelectedItems which is used by selectedChildren/selectedColumns/selectedRows. The patch makes it.
(Reporter)

Updated

10 years ago
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+
(Assignee)

Comment 18

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 19

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