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

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

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

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

Comment 1

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

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

Comment 2

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

Though the obvious fix for that doesn't actually seem to help.

Comment 3

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

Well, switching uniformly to unload didn't fix it, but switching uniformly to pagehide does fix it.

Comment 4

[Aaron Leventhal]

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?

Comment 5

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

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Ginn Chen]

Aaron, I can reproduce this bug with Firefox 1.5.0.1 on Windows.
(Keep AccEvent run behind, and open a large photo album.)

Comment 8

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

Attachment: back out part of bug 300864

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.

Comment 9

[Aaron Leventhal]

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 10

[Brian Ryner (not reading)]

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.

Comment 11

[Aaron Leventhal]

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?

Comment 12

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

Fix checked in to trunk and MOZILLA_1_8_BRANCH.

drivers are doing approvals for 1.8.0.3.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 217179 [details] [diff] [review]
back out part of bug 300864

approved for 1.8.0 branch, a=dveditz for drivers

Comment 14

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

Fix checked in to MOZILLA_1_8_0_BRANCH.

Comment 15

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

A checkin at 2006-04-18 12:18 PDT incorrectly cited this bug rather than bug 333134.

Comment 16

[Asaf Romano (gone)]

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