Closed
Bug 330780
Opened 19 years ago
Closed 19 years ago
[ATK only] global nsAppRootAccessible is not released on shutdown
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dbaron, Assigned: ginnchen+exoracle)
References
Details
(Keywords: fixed1.8.1, memory-leak)
Attachments
(1 file, 3 obsolete files)
4.90 KB,
patch
|
aaronlev
:
review+
dbaron
:
superreview+
aaronlev
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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•19 years ago
|
Assignee: aaronleventhal → ginn.chen
Comment 1•19 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•19 years ago
|
OS: All → Linux
Reporter | ||
Comment 2•19 years ago
|
||
This isn't a user-visible leak, it's just a shutdown leak, which interferes only with debugging other leaks.
Updated•19 years ago
|
Blocks: fox2access
I don't know how to verify the leak. Aaron, do you think this patch reasonable?
Attachment #219131 -
Flags: review?(aaronleventhal)
Comment 4•19 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•19 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•19 years ago
|
Attachment #219131 -
Flags: superreview?(dbaron)
Reporter | ||
Comment 6•19 years ago
|
||
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.
Reporter | ||
Updated•19 years ago
|
Attachment #219131 -
Flags: superreview?(dbaron) → superreview-
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•19 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?
Attachment #219565 -
Flags: review?(aaronleventhal)
Reporter | ||
Comment 10•19 years ago
|
||
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•19 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•19 years ago
|
||
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•19 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+
Attachment #219568 -
Flags: superreview?(dbaron)
Reporter | ||
Comment 14•19 years ago
|
||
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•19 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
Closed: 19 years ago
Resolution: --- → FIXED
Attachment #219568 -
Flags: approval-branch-1.8.1?(aaronleventhal)
Comment 16•19 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•19 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•19 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.
Description
•