252731 - Fix memory leaks in accessible/src/atk/nsAccessibleHyperText.cpp

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jonathan Watt [:jwatt]]

Both do_QueryInterface() and ElementAt() call AddRef() - see:
http://lxr.mozilla.org/mozilla/source/xpcom/ds/nsSupportsArray.cpp#323

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[[e-mail]]

fyi, you can use do_QueryElementAt here, and keep the syntactic sugar of do_QI.
just s/do_QueryInterface/do_QueryElementAt/ and you're good to go ;)

http://lxr.mozilla.org/mozilla/source/xpcom/ds/nsICollection.idl#87

Comment 3

[[e-mail]]

er, more like this.

s/do_QueryInterface(mTextChildren->ElementAt(index))/do_QueryElementAt(mTextChildren,
index))/

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch dwitte's way

Interesting, thanks dwitte. I haven't checked that this patch builds okay.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 154074 [details] [diff] [review]
patch dwitte's way

This is fine in general (and I should have recalled do_QueryElementAt), but
please follow the file's style for whitespace around the do_QIAt (that is, no
spaces there) and please don't make random whitespace changes...

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: updated patch

Patch corrected to follow style of the file. Those whitespace changes aren't
random btw, I removed trailing whitespace from the ends of all lines to tidy
the file. Unless you really object to that I'd prefer it gets tidied.

Comment 7

[Jonathan Watt [:jwatt]]

I'd rather not just assume that you're happy with the updated patch bz, can you
add a quick comment to let me know. 

Comment 8

[Boris Zbarsky [:bzbarsky]]

I realize what the whitespace changes were, but they're random in that they are
unrelated to this bug.  And yes, I really object to them -- they make CVS blame
more confusing for little benefit...

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: final patch

(In reply to comment #8)
> I realize what the whitespace changes were, but they're random in that they
are
> unrelated to this bug.  And yes, I really object to them -- they make CVS
blame
> more confusing for little benefit...

I didn't realise that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 154117 [details] [diff] [review]
final patch

r+sr=bzbarsky.

Want me to check this in?

Comment 11

[Bernard Alleysson]

It is a fixed memory leak bug, could it be considered for 1.7.2 ?

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 154117 [details] [diff] [review]
final patch

Marking for approval, sorry I didn't do this before 1.7.2.

Comment 13

[Jonathan Watt [:jwatt]]

Approver: I'm away until September, but please still review - bz will check this
into the branch if he catches the bugmail. 

Comment 14

[Mike Kaply [:mkaply]]

Comment on attachment 154117 [details] [diff] [review]
final patch

a=mkaply for aviary and 1.7.3

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch merged to 1.7 branch

Branch is closed, but this is the patch that should go in once it opens.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Marking fixed, since it's fixed on trunk.