Closed Bug 330624 Opened 14 years ago Closed 14 years ago

accessibility code (when accessibility enabled) holds on to DOM nodes until shutdown

Categories

(Core :: Disability Access APIs, defect, critical)

x86
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: fixed1.8.1, memory-leak, verified1.8.0.4, Whiteboard: [olpc][patch])

Attachments

(1 file)

The accessibility code seems to leak DOM nodes until shutdown.  I learned of this in https://www.redhat.com/archives/olpc-software/2006-March/msg00115.html

When I follow the accessibility-enabling steps in the above message and then:
 * load a file URL for a directory with a lot of large images (e.g., photos)
 * click on each one and hit back
and in another tab watch what "about:cache" says about the image cache (which continues to know about any images as long as something else is holding on to them, even if they're over the size limit), then you see the image cache continue to grow.

The following stack shows the next-to-last release (since the last is the image cache dropping its reference in response) of the last imgRequest object created, which shows the source of the problem:

imgRequest::Release() (/builds/trunk/mozilla/modules/libpr0n/src/imgRequest.cpp:74)
imgRequestProxy::~imgRequestProxy() (/builds/trunk/mozilla/modules/libpr0n/src/imgRequestProxy.cpp:95)
imgRequestProxy::Release() (/builds/trunk/mozilla/modules/libpr0n/src/imgRequestProxy.cpp:58)
nsCOMPtr<imgIRequest>::~nsCOMPtr() (../../dist/include/xpcom/nsCOMPtr.h:583)
nsImageLoadingContent::~nsImageLoadingContent() (/builds/trunk/mozilla/content/base/src/nsImageLoadingContent.cpp:123)
nsHTMLImageElement::~nsHTMLImageElement() (/builds/trunk/mozilla/content/html/content/src/nsHTMLImageElement.cpp:187)
nsGenericElement::Release() (/builds/trunk/mozilla/content/base/src/nsGenericElement.cpp:3129)
nsHTMLImageElement::Release() (/builds/trunk/mozilla/content/html/content/src/nsHTMLImageElement.cpp:191)
nsCOMPtr<nsIDOMNode>::assign_assuming_AddRef(nsIDOMNode*) (../../../dist/include/xpcom/nsCOMPtr.h:568)
nsCOMPtr<nsIDOMNode>::assign_with_AddRef(nsISupports*) (../../../dist/include/xpcom/nsCOMPtr.h:1224)
nsCOMPtr<nsIDOMNode>::operator=(nsIDOMNode*) (../../../dist/include/xpcom/nsCOMPtr.h:714)
nsAccessNode::Shutdown() (/builds/trunk/mozilla/accessible/src/base/nsAccessNode.cpp:155)
nsAccessible::Shutdown() (/builds/trunk/mozilla/accessible/src/base/nsAccessible.cpp:412)
nsLinkableAccessible::Shutdown() (/builds/trunk/mozilla/accessible/src/base/nsBaseWidgetAccessible.cpp:327)
nsAccessNode::ClearCacheEntry(void const*, nsCOMPtr<nsIAccessNode>&, void*) (/builds/trunk/mozilla/accessible/src/base/nsAccessNode.cpp:532)
nsBaseHashtable<nsVoidHashKey, nsCOMPtr<nsIAccessNode>, nsIAccessNode*>::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) (../../../dist/include/xpcom/nsBaseHashtable.h:346)
PL_DHashTableEnumerate (/builds/trunk/obj/firefox-debugopt/xpcom/build/pldhash.c:621)
nsBaseHashtable<nsVoidHashKey, nsCOMPtr<nsIAccessNode>, nsIAccessNode*>::Enumerate(PLDHashOperator (*)(void const*, nsCOMPtr<nsIAccessNode>&, void*), void*) (../../../dist/include/xpcom/nsBaseHashtable.h:221)
nsAccessNode::ClearCache(nsInterfaceHashtable<nsVoidHashKey, nsIAccessNode>&) (/builds/trunk/mozilla/accessible/src/base/nsAccessNode.cpp:537)
nsDocAccessible::Shutdown() (/builds/trunk/mozilla/accessible/src/base/nsDocAccessible.cpp:482)
nsDocAccessibleWrap::Shutdown() (/builds/trunk/mozilla/accessible/src/atk/nsDocAccessibleWrap.cpp:519)
nsAccessNode::ClearCacheEntry(void const*, nsCOMPtr<nsIAccessNode>&, void*) (/builds/trunk/mozilla/accessible/src/base/nsAccessNode.cpp:532)
nsBaseHashtable<nsVoidHashKey, nsCOMPtr<nsIAccessNode>, nsIAccessNode*>::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) (../../../dist/include/xpcom/nsBaseHashtable.h:346)
PL_DHashTableEnumerate (/builds/trunk/obj/firefox-debugopt/xpcom/build/pldhash.c:621)
nsBaseHashtable<nsVoidHashKey, nsCOMPtr<nsIAccessNode>, nsIAccessNode*>::Enumerate(PLDHashOperator (*)(void const*, nsCOMPtr<nsIAccessNode>&, void*), void*) (../../../dist/include/xpcom/nsBaseHashtable.h:221)
nsAccessNode::ClearCache(nsInterfaceHashtable<nsVoidHashKey, nsIAccessNode>&) (/builds/trunk/mozilla/accessible/src/base/nsAccessNode.cpp:537)
nsAccessNode::ShutdownXPAccessibility() (/builds/trunk/mozilla/accessible/src/base/nsAccessNode.cpp:217)
nsAccessNodeWrap::ShutdownAccessibility() (/builds/trunk/mozilla/accessible/src/atk/nsAccessNodeWrap.cpp:74)
nsAccessibilityService::Observe(nsISupports*, char const*, unsigned short const*) (/builds/trunk/mozilla/accessible/src/base/nsAccessibilityService.cpp:155)
nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned short const*) (/builds/trunk/mozilla/xpcom/ds/nsObserverService.cpp:240)
NS_ShutdownXPCOM_P (/builds/trunk/mozilla/xpcom/build/nsXPComInit.cpp:696)
ScopedXPCOMStartup::~ScopedXPCOMStartup() (/builds/trunk/mozilla/toolkit/xre/nsAppRunner.cpp:557)
XRE_main (/builds/trunk/mozilla/toolkit/xre/nsAppRunner.cpp:2403)
main (/builds/trunk/mozilla/browser/app/nsBrowserApp.cpp:62)
__libc_start_main+0x000000D3  (/lib/tls/libc.so.6)

This leak is in the category of "huge memory leak that means we barely ever free any memory".
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Summary: accessibility code (when accessability enabled) holds on to DOM nodes until shutdown → accessibility code (when accessibility enabled) holds on to DOM nodes until shutdown
It wouldn't surprise me if this was caused by bug 300864, which changed the event check in HandleEvent without changing which event the listener was registered for (so we recieve "pagehide" events but have code to handle "unload" events).
Though the obvious fix for that doesn't actually seem to help.
Whiteboard: [olpc]
Well, switching uniformly to unload didn't fix it, but switching uniformly to pagehide does fix it.
Flags: blocking1.8.0.3?
David,

Thank you for investigating this. I'm CC'ing Ginn Chen from Sun. His team in Beijing wrote this code. Ginn, will your team be able to look at this high priority bug? My new guys who will be working on Linux accessibility haven't started yet.

I believe this is only a problem on Linux. First, the AppRootAccessible object only exists on Linux. Secondly, the accessibility architecture on Windows allows us to only create accessible objects if there is an assistive technology asking for them. We don't even load that accessibility code at all unless we need it there. On Linux it's not so smooth. Everything needs to be set up via desktop prefs so it's all or nothing.

Also, on OS X we never initialize accessibility.

David, are the huge leak reports happening on all platforms?
I don't see why you think this is Linux only -- this is all cross-platform code.  I think you're confusing it with bug 330780.
Hmm...  Does doing an unload listener on the _document_ even work?  The unload event is fired on the window last I checked.

That said, I think pagehide is the right handler here; using unload would disable bfcache completely.
Flags: blocking1.9a1?
Flags: blocking1.9a1+
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3+
Aaron, I can reproduce this bug with Firefox 1.5.0.1 on Windows.
(Keep AccEvent run behind, and open a large photo album.)
OS: Linux → All
For lack of progress here, here's a patch to back out the offending part of bug 300864.  I suspect this probably regresses it, although I'm not set up to test that.  The deadline for 1.8.0.3 is fast approaching, so I think we want this sooner rather than later if there's no other fix forthcoming, and I suspect we'll need this change anyway as part of whatever the fix would be that doesn't regress bug 300864.
Attachment #217179 - Flags: superreview?(bryner)
Attachment #217179 - Flags: review?(aaronleventhal)
Attachment #217179 - Flags: approval1.8.0.3?
Attachment #217179 - Flags: approval-branch-1.8.1?(aaronleventhal)
Comment on attachment 217179 [details] [diff] [review]
back out part of bug 300864

Looking at this now -- apologies for the delay. I want to make sure we have the right solution so that page load errors are still accessible.
Comment on attachment 217179 [details] [diff] [review]
back out part of bug 300864

I'm ok with this if aaronl doesn't find a better solution for 1.8.0.3.
Attachment #217179 - Flags: superreview?(bryner) → superreview+
Attachment #217179 - Flags: review?(aaronleventhal)
Attachment #217179 - Flags: review+
Attachment #217179 - Flags: approval-branch-1.8.1?(aaronleventhal)
Attachment #217179 - Flags: approval-branch-1.8.1+
Comment on attachment 217179 [details] [diff] [review]
back out part of bug 300864

For some reason this does not regress bug 300864.

Who gives approval for 1.8.0.3? Module owner?
Assignee: aaronleventhal → dbaron
Whiteboard: [olpc] → [olpc][patch]
Fix checked in to trunk and MOZILLA_1_8_BRANCH.

drivers are doing approvals for 1.8.0.3.
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Comment on attachment 217179 [details] [diff] [review]
back out part of bug 300864

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #217179 - Flags: approval1.8.0.3? → approval1.8.0.3+
Fix checked in to MOZILLA_1_8_0_BRANCH.
Keywords: fixed1.8.0.3
A checkin at 2006-04-18 12:18 PDT incorrectly cited this bug rather than bug 333134.
(In reply to comment #11)
> (From update of attachment 217179 [details] [diff] [review] [edit])
> For some reason this does not regress bug 300864.
> 
Prolly because, unlike PageShow/load events, PageHide is fired for error pages.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.