Closed
Bug 417018
Opened 17 years ago
Closed 17 years ago
Array allocation mistakes in IA2
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: aaronlev, Assigned: surkov)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(1 file)
11.03 KB,
patch
|
aaronlev
:
review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
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•17 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•17 years ago
|
||
nsAccessibleWrap::get_relations(long aMaxRelations, IAccessibleRelation **aRelation, long *aNRelations) - the same
Assignee | ||
Comment 3•17 years ago
|
||
Attachment #303009 -
Flags: review?(aaronleventhal)
Reporter | ||
Comment 4•17 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•17 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•17 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•17 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•17 years ago
|
||
Regarding comment 6, I got this info from Pete Brunet. He is clarifying the IDL now.
Reporter | ||
Comment 9•17 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•17 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•17 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•17 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•17 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•17 years ago
|
Attachment #303009 -
Flags: review?(Evan.Yan)
Assignee | ||
Updated•17 years ago
|
Attachment #303009 -
Flags: review?(Evan.Yan)
Assignee | ||
Comment 14•17 years ago
|
||
Aaron, should I remove array allocating code from the patch and save only strings stuff?
Reporter | ||
Comment 15•17 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•17 years ago
|
||
Yes, we need for CAccessibleTable::GetSelectedItems which is used by selectedChildren/selectedColumns/selectedRows. The patch makes it.
Reporter | ||
Updated•17 years ago
|
Attachment #303009 -
Flags: review?(aaronleventhal)
Attachment #303009 -
Flags: review+
Attachment #303009 -
Flags: approval1.9?
Comment 17•17 years ago
|
||
Comment on attachment 303009 [details] [diff] [review]
some clean up
a1.9=beltzner
Attachment #303009 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 18•17 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
Closed: 17 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 19•17 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.
Description
•