Closed Bug 330780 Opened 19 years ago Closed 18 years ago

[ATK only] global nsAppRootAccessible is not released on shutdown

Categories

(Core :: Disability Access APIs, defect)

All
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: ginnchen+exoracle)

References

Details

(Keywords: fixed1.8.1, memory-leak)

Attachments

(1 file, 3 obsolete files)

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.
Assignee: aaronleventhal → ginn.chen
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
OS: All → Linux
This isn't a user-visible leak, it's just a shutdown leak, which interferes only with debugging other leaks.
Blocks: fox2access
Attached patch patch (obsolete) — Splinter Review
I don't know how to verify the leak.

Aaron, do you think this patch reasonable?
Attachment #219131 - Flags: review?(aaronleventhal)
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 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+
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-
Attached patch patch v2 (obsolete) — Splinter Review
BTW:
I saw nsAccessNodeWrap::ShutdownAccessibility() in nsAccessibilityService destructor.
Do we need it?
Attachment #219131 - Attachment is obsolete: true
Attachment #219273 - Flags: review?(aaronleventhal)
(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?
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.
(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
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 on attachment 219568 [details] [diff] [review]
patch v4 (addressing dbaron's comments)

One nit:
nsAccessibleNode -> nsAccessNode
Attachment #219568 - Flags: review?(aaronleventhal) → review+
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+
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
Closed: 18 years ago
Resolution: --- → FIXED
Attachment #219568 - Flags: approval-branch-1.8.1?(aaronleventhal)
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+
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
(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.

Attachment

General

Creator:
Created:
Updated:
Size: