251257 - Memory leak in nsXULTreeAccessible.cpp

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Philip K. Warren]

I ran a trunk build of Mozilla under valgrind, and I'm seeing the following
memory leak in nsXULTreeAccessible.cpp:

64 bytes in 2 blocks are definitely lost in loss record 126 of 347
   at 0x1B904830: operator new(unsigned) (vg_replace_malloc.c:111)
   by 0x1E9F79FC: nsXULTreeAccessible::nsXULTreeAccessible(nsIDOMNode*,
nsIWeakReference*) (nsXULTreeAccessible.cpp:67)
   by 0x1E9E97D7: nsXULTreeAccessibleWrap::nsXULTreeAccessibleWrap(nsIDOMNode*,
nsIWeakReference*) (nsXULTreeAccessibleWrap.cpp:52)
   by 0x1E99BE3B: nsAccessibilityService::CreateXULTreeAccessible(nsIDOMNode*,
nsIAccessible**) (nsAccessibilityService.cpp:1473)
   by 0x1BE8DB80: XPTC_InvokeByIndex (xptcinvoke_gcc_x86_unix.cpp:69)
   by 0x1C073E7A: XPCWrappedNative::CallMethod(XPCCallContext&,
XPCWrappedNative::CallMode) (xpcwrappednative.cpp:2027)
   by 0x1C07DFFA: XPC_WN_CallMethod(JSContext*, JSObject*, unsigned, long*,
long*) (xpcwrappednativejsops.cpp:1287)
   by 0x1B95269E: js_Invoke (jsinterp.c:1281)

I think all that needs to happen is we need to delete the mAccessNodeCache
variable after calling ClearCache in the destructor.

Comment 1

[Philip K. Warren]

Attachment: Patch v1

Fix the memory leak. Verified before/after with Valgrind.

Comment 2

[Aaron Leventhal]

Comment on attachment 153081 [details] [diff] [review]
Patch v1

Great catch!

Even better, all of this code should be moved to
nsXULTreeAccessible::Shutdown() which will get called when the tree node is
deleted from the DOM. It also gets called if nsXULTreeAccessible gets destroyed
-- you don't have to do anything extra for that.

The reason we need to put stuff in Shutdown() methods is that an external MSAA
client can still be holding onto an accessibilty object long after the DOM node
that the accessibility object points to is gone. We call the shutdown method
and all that's left hanging around is the relatively small nsFooAcessible.

You'll also have to upcall to the Shutdown() method of the class we inherited
from. You might want to look at examples of Shutdown() in some of the other
classes.

Comment 3

[Philip K. Warren]

Attachment: Patch v2

Implement Aaron's suggestion.

Comment 4

[Aaron Leventhal]

Comment on attachment 153819 [details] [diff] [review]
Patch v2

So is the following line no longer necessary? Does it also need to be in
Shutdown()?

+ delete mAccessNodeCache;

Comment 5

[Philip K. Warren]

Attachment: Patch v3

Of course. I was testing before and after adding the delete in Valgrind to make
sure this solved the leak. This is the correct patch.

Comment 6

[Darin Fisher]

Comment on attachment 153964 [details] [diff] [review]
Patch v3

sr=darin

Comment 7

[Philip K. Warren]

Fixed.

Checking in nsXULTreeAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULTreeAccessible.cpp,v  <-- 
nsXULTreeAccessible.cpp
new revision: 1.22; previous revision: 1.21
done
Checking in nsXULTreeAccessible.h;
/cvsroot/mozilla/accessible/src/xul/nsXULTreeAccessible.h,v  <-- 
nsXULTreeAccessible.h
new revision: 1.13; previous revision: 1.12
done