288848 - Various memory leaks in nsAccessibleHyperText

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The ElementAt method of nsISupportsArray addrefs the element before returning
the pointer.  Therefore the following methods leak (or probably leak, unless the
constructors of the accessible objects expect an already-addrefed pointer coming
in for some reason):

nsAccessibleHyperText::FindTextNodeByOffset
nsAccessibleHyperText::GetCaretOffset
nsAccessibleHyperText::GetCharacterCount
nsAccessibleHyperText::GetSelectionCount
nsAccessibleHyperText::GetText
nsAccessibleHyperText::GetBounds

Note that some code in the same file does correctly use do_QueryElementAt...

Also note that mTextChildren may just want to be an nsCOMArray<nsIDOMNode> --
that would make things simpler in most of this code.

Comment 1

[Aaron Leventhal]

Louie, do you want to check this out, bug 288852 bug 288781 which are related?
It might be easiest to just come up with 1 patch to fix all 3.

Comment 2

[Louie Zhao]

Attachment: patch v1

Since nsISupportsArray is obsoleted, the patch replaces nsISupportsArray with
nsIArray/nsIMutableArray in accessiblity module. Although
nsCOMArray<nsIDOMNode> is more suitable in class nsAccessibleHyperText, it will
be big change to make nsAccessibleText use nsCOMArray<T> because nsCOMArray<T>
is C++ class and doesn't support nsISupport (the original design of
nsAccessibleText and nsAccessibleEditableText needs the "aClosure" argument in
helper functions to support nsISupport).

Comment 3

[Louie Zhao]

*** Bug 288852 has been marked as a duplicate of this bug. ***

Comment 4

[Louie Zhao]

*** Bug 288781 has been marked as a duplicate of this bug. ***

Comment 5

[Aaron Leventhal]

Comment on attachment 179695 [details] [diff] [review]
patch v1

Louie, after you fix this bug I suggest testing with and without
XPCOM_MEM_LEAK_LOG, to see if there are any extra leaks when ATK code is used.
I tried it with MSAA and we're okay as long as the AT is unloaded before the
user exits Firefox.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 179695 [details] [diff] [review]
patch v1

Yeah, if we need to use this for nsISupports closures then using nsIArray is
fine...

The patch looks fine per se, but this part:

>-  nsISupportsArray* mTextNodes;
>+  nsIArray* mTextNodes;

Kinda bothers me.  Shouldn't that hold a ref to the array?

In any case, sr=bzbarsky, since that was already there...

Comment 7

[Louie Zhao]

Aaron, I have tested with XPCOM_MEM_LEAK_LOG. The original code really causes
leak and the patch here has fixed it.

bz, I have changed "nsIArray* mTextNodes" to "nsCOMPtr<nsIArray> mTextNodes"
which is better.

Thanks for sr & r. Patch checked in.