[ATK only] global nsAppRootAccessible is not released on shutdown

RESOLVED FIXED

Status

()

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

People

(Reporter: dbaron, Assigned: Ginn Chen)

Tracking

({fixed1.8.1, memory-leak})

Trunk
All
Linux
fixed1.8.1, memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

The global nsAppRootAccessible is not released on shutdown; this clutters up the leak stats when accessibility is enabled in the OS (in GNOME, Desktop ->
Preferences -> Accesibility -> Assistive Technology Support -> Enable Assistive
Technologies and log out of GNOME and log back in).  It should probably be released either on an "xpcom-shutdown" observer notification (if its destructor could propagate releases into other XPCOM components) or in the module destructor for the accessibility component (otherwise), although there may be more options as bsmedberg's shutdown changes progress.

I'd also note that code to release it would probably want to do something like call Shutdown on the global (which would need to be moved out of function scope) rather than releasing it, since Shutdown currently releases the global.  Or something like that -- the distinctions between destructors, Shutdown, Destroy, and the ownership models here don't seem to be very well commented.

Updated

12 years ago
Assignee: aaronleventhal → ginn.chen

Comment 1

12 years ago
Any leak or other important bug in mozilla/accessible needs to be at least "major" or it won't make our radar for 2006.
Severity: trivial → major

Updated

12 years ago
OS: All → Linux
This isn't a user-visible leak, it's just a shutdown leak, which interferes only with debugging other leaks.

Updated

12 years ago
Blocks: 333488
(Assignee)

Comment 3

12 years ago
Created attachment 219131 [details] [diff] [review]
patch

I don't know how to verify the leak.

Aaron, do you think this patch reasonable?
Attachment #219131 - Flags: review?(aaronleventhal)

Comment 4

12 years ago
Comment on attachment 219131 [details] [diff] [review]
patch

Makes sense, but don't use #ifdef. Instead, move the code here:
http://lxr.mozilla.org/seamonkey/source/accessible/src/atk/nsAccessNodeWrap.cpp#71

If you do end up needing to put some shutdown code that runs cross platform, you would put it in nsAccessNode::ShutdownXPAccessibility()

We have the similar routines for Init'ing accessibility. Does ATK need to do anything there?

Comment 5

12 years ago
Comment on attachment 219131 [details] [diff] [review]
patch

You have r+=aaronleventhal if you move the code as I suggested.
Attachment #219131 - Flags: review?(aaronleventhal) → review+

Updated

12 years ago
Attachment #219131 - Flags: superreview?(dbaron)
I'd strongly recommend making the sAppRoot in nsAppRootAccessible.cpp be file-scope instead of function-scope and then just calling NS_IF_RELEASE(sAppRoot) in nsAppRootAccessible::Shutdown -- so that the pointer gets nulled out.

You can test the leaks by setting (in a build with --enable-debug or --enable-logrefcnt) the environment variable XPCOM_MEM_LEAK_LOG to a filename to which you want to log leaked XPCOM objects.
Attachment #219131 - Flags: superreview?(dbaron) → superreview-
(Assignee)

Comment 7

12 years ago
Created attachment 219273 [details] [diff] [review]
patch v2

BTW:
I saw nsAccessNodeWrap::ShutdownAccessibility() in nsAccessibilityService destructor.
Do we need it?
Attachment #219131 - Attachment is obsolete: true
Attachment #219273 - Flags: review?(aaronleventhal)

Comment 8

12 years ago
(In reply to comment #7)
> Created an attachment (id=219273) [edit]
> patch v2
> 
> BTW:
> I saw nsAccessNodeWrap::ShutdownAccessibility() in nsAccessibilityService
> destructor.
> Do we need it?

Maybe not, but I suppose it's there in case the accessibility service goes away before xpcom-shutdown, although that never happens at the moment. Does it do any harm?

> nsAppRootAccessible *root = new nsAppRootAccessible();
Do we really need to create an object to shut down? That seems a bit odd. Can't we just call a static method on nsAppRootAccessible to shut down the static variable? Or, would you consider making sAppRoot a member of nsAccessNodeWrap? Then I don't think you would need this, or am I missing something?
(Assignee)

Comment 9

12 years ago
Created attachment 219565 [details] [diff] [review]
patch v3, use a static method to unload
Attachment #219565 - Flags: review?(aaronleventhal)
Comment on attachment 219565 [details] [diff] [review]
patch v3, use a static method to unload

>-NS_IMETHODIMP nsAppRootAccessible::Shutdown()
>+nsresult nsAppRootAccessible::Unload()

It might be worth writing "/* static */" at the beginning here for clarity.

>+    if (sAppRoot)
>+        NS_IF_RELEASE(sAppRoot);

You don't need two layers of ifs.  You should drop the "if (sAppRoot)" since that's part of NS_IF_RELEASE.

It seems like Unload should return void rather than nsresult, since it always returns NS_OK and nobody ever checks the return value.
(Assignee)

Comment 11

12 years ago
(In reply to comment #8)
> Maybe not, but I suppose it's there in case the accessibility service goes away
> before xpcom-shutdown, although that never happens at the moment. Does it do
> any harm?
Not yet.
The only problem is nsAccessNodeWrap::ShutdownAccessibility() will be called twice.

> > nsAppRootAccessible *root = new nsAppRootAccessible();
> Do we really need to create an object to shut down? That seems a bit odd. Can't
> we just call a static method on nsAppRootAccessible to shut down the static
> variable? Or, would you consider making sAppRoot a member of nsAccessNodeWrap?
> Then I don't think you would need this, or am I missing something?

Shutdown is a virtual member function, so I have to change its name.
Status: NEW → ASSIGNED
(Assignee)

Comment 12

12 years ago
Created attachment 219568 [details] [diff] [review]
patch v4 (addressing dbaron's comments)
Attachment #219273 - Attachment is obsolete: true
Attachment #219565 - Attachment is obsolete: true
Attachment #219568 - Flags: review?(aaronleventhal)
Attachment #219273 - Flags: review?(aaronleventhal)
Attachment #219565 - Flags: review?(aaronleventhal)

Comment 13

12 years ago
Comment on attachment 219568 [details] [diff] [review]
patch v4 (addressing dbaron's comments)

One nit:
nsAccessibleNode -> nsAccessNode
Attachment #219568 - Flags: review?(aaronleventhal) → review+
(Assignee)

Updated

12 years ago
Attachment #219568 - Flags: superreview?(dbaron)
Comment on attachment 219568 [details] [diff] [review]
patch v4 (addressing dbaron's comments)

sr=dbaron.

It's worth noting that this patch also fixes the fact that nsAppRootAccessible's Shutdown method didn't call its base class's shutdown method, so nsAccessible::Shutdown will now be called for this object.  I'm not sure if that will change anything, but it seems good at first glance.
Attachment #219568 - Flags: superreview?(dbaron) → superreview+
(Assignee)

Comment 15

12 years ago
Checking in nsAccessNodeWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsAccessNodeWrap.cpp,v  <--  nsAccessNodeWrap.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in nsAppRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/atk/nsAppRootAccessible.cpp,v  <--  nsAppRootAccessible.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in nsAppRootAccessible.h;
/cvsroot/mozilla/accessible/src/atk/nsAppRootAccessible.h,v  <--  nsAppRootAccessible.h
new revision: 1.8; previous revision: 1.7
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #219568 - Flags: approval-branch-1.8.1?(aaronleventhal)

Comment 16

12 years ago
Comment on attachment 219568 [details] [diff] [review]
patch v4 (addressing dbaron's comments)

Ginn, nsAppRootAccessible::Init() should call up to nsAccessibleWrap::Init(), correct? It doesn't.
Attachment #219568 - Flags: approval-branch-1.8.1?(aaronleventhal) → approval-branch-1.8.1+
(Assignee)

Comment 17

12 years ago
Checking in nsAccessNodeWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsAccessNodeWrap.cpp,v  <--  nsAccessNodeWrap.cpp
new revision: 1.3.28.1; previous revision: 1.3
done
Checking in nsAppRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/atk/nsAppRootAccessible.cpp,v  <--  nsAppRootAccessible.cpp
new revision: 1.11.26.3; previous revision: 1.11.26.2
done
Checking in nsAppRootAccessible.h;
/cvsroot/mozilla/accessible/src/atk/nsAppRootAccessible.h,v  <--  nsAppRootAccessible.h
new revision: 1.4.68.2; previous revision: 1.4.68.1
done
Keywords: fixed1.8.1
(Assignee)

Comment 18

12 years ago
(In reply to comment #16)
> (From update of attachment 219568 [details] [diff] [review] [edit])
> Ginn, nsAppRootAccessible::Init() should call up to nsAccessibleWrap::Init(),
> correct? It doesn't.
> 

nsAppRootAccessible::Init() is called by nsAppRootAccessible::Create()
nsAppRootAccessible::Create() is called by nsRootAccessibleWrap::Init()

Before nsAppRootAccessible::Create() is called,
nsRootAccessibleWrap::Init() calls nsRootAccessibleWrap::Init(), which calls nsDocAccessible::Init(), which calls nsAccessible::Init(), which calls nsAccessNode::Init()

So we don't need to up call nsAccessibleWrap::Init()
You need to log in before you can comment on or make changes to this bug.