330780 - [ATK only] global nsAppRootAccessible is not released on shutdown

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[Aaron Leventhal]

Any leak or other important bug in mozilla/accessible needs to be at least "major" or it won't make our radar for 2006.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This isn't a user-visible leak, it's just a shutdown leak, which interferes only with debugging other leaks.

Comment 3

[Ginn Chen]

Attachment: patch

I don't know how to verify the leak.

Aaron, do you think this patch reasonable?

Comment 4

[Aaron Leventhal]

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

[Aaron Leventhal]

Comment on attachment 219131 [details] [diff] [review]
patch

You have r+=aaronleventhal if you move the code as I suggested.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 7

[Ginn Chen]

Attachment: patch v2

BTW:
I saw nsAccessNodeWrap::ShutdownAccessibility() in nsAccessibilityService destructor.
Do we need it?

Comment 8

[Aaron Leventhal]

(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?

Comment 9

[Ginn Chen]

Attachment: patch v3, use a static method to unload

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 11

[Ginn Chen]

(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.

Comment 12

[Ginn Chen]

Attachment: patch v4 (addressing dbaron's comments)

Comment 13

[Aaron Leventhal]

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

One nit:
nsAccessibleNode -> nsAccessNode

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 15

[Ginn Chen]

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

Comment 16

[Aaron Leventhal]

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.

Comment 17

[Ginn Chen]

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

Comment 18

[Ginn Chen]

(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()