nsDocAccessible is not destroyed when closing a tab

RESOLVED FIXED

Status

()

Core
Disability Access APIs
--
critical
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Ginn Chen, Assigned: Ginn Chen)

Tracking

({memory-leak})

Trunk
x86
All
memory-leak
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
With a11y feature enabled, press ctrl+t open a new tab, and then press ctrl+w to close it.
The new created nsDocAccessible is cached in gGlobalDocAccessibleCache but will not be destroyed and removed from the cache until Firefox exits.

The problem is we destroy nsDocAccessible on pagehide event, and we use mWeakShell as hashkey of the cache.
But PresShell is destroyed earlier than pagehide event.
So we don't have a chance to find the nsDocAccessible and destroy it.

The stack of destroying PresShell,
=>[1] nsDocument::DeleteShell(this = 0x93e63d8, aShell = 0x9381de0), line 2044 in "nsDocument.cpp"
  [2] PresShell::Destroy(this = 0x9381de0), line 1656 in "nsPresShell.cpp"
  [3] DocumentViewerImpl::Hide(this = 0x92dcf80), line 1986 in "nsDocumentViewer.cpp"
  [4] nsDocShell::SetVisibility(this = 0x9337270, aVisibility = 0), line 3852 in "nsDocShell.cpp"
  [5] nsSubDocumentFrame::Destroy(this = 0x8dce948), line 728 in "nsFrameFrame.cpp"
  [6] nsFrameList::DestroyFrames(this = 0x8dd2a40), line 67 in "nsFrameList.cpp"


The stack of receiving pagehide event,
=>[1] nsRootAccessible::HandleEventWithTarget(this = 0x8bf8600, aEvent = 0x9370cbc, aTargetNode = 0x93e646c), line 585 in "nsRootAccessible.cpp"
  [2] nsRootAccessible::HandleEvent(this = 0x8bf8600, aEvent = 0x9370cbc), line 579 in "nsRootAccessible.cpp"
  [3] nsEventListenerManager::HandleEventSubType(this = 0x87b8078, aListenerStruct = 0x8a22698, aListener = 0x8bf86b8, aDOMEvent = 0x9370cbc, aCurrentTarget = 0x87b8014, aPhaseFlags = 4U), line 1097 in "nsEventListenerManager.cpp"
  [4] nsEventListenerManager::HandleEvent(this = 0x87b8078, aPresContext = (nil), aEvent = 0x804195c, aDOMEvent = 0x80417c0, aCurrentTarget = 0x87b8014, aFlags = 4U, aEventStatus = 0x80417c4), line 1210 in "nsEventListenerManager.cpp"
  [5] nsEventTargetChainItem::HandleEvent(this = 0x93f5fe8, aVisitor = CLASS, aFlags = 4U), line 203 in "nsEventDispatcher.cpp"
  [6] nsEventTargetChainItem::HandleEventTargetChain(this = 0x93f5fe8, aVisitor = CLASS, aFlags = 6U, aCallback = (nil)), line 237 in "nsEventDispatcher.cpp"
  [7] nsEventDispatcher::Dispatch(aTarget = 0x93dcba0, aPresContext = (nil), aEvent = 0x804195c, aDOMEvent = (nil), aEventStatus = (nil), aCallback = (nil)), line 476 in "nsEventDispatcher.cpp"
  [8] nsDocument::DispatchEventToWindow(this = 0x93e63d8, aEvent = 0x804195c), line 5753 in "nsDocument.cpp"
  [9] nsDocument::OnPageHide(this = 0x93e63d8, aPersisted = 0), line 5806 in "nsDocument.cpp"
  [10] DocumentViewerImpl::PageHide(this = 0x92dcf80, aIsUnload = 1), line 1139 in "nsDocumentViewer.cpp"
  [11] nsDocShell::FirePageHideNotification(this = 0x9337270, aIsUnload = 1), line 988 in "nsDocShell.cpp"
  [12] nsDocShell::Destroy(this = 0x9337270), line 3557 in "nsDocShell.cpp"
  [13] nsFrameLoader::Destroy(this = 0x892ffd0), line 265 in "nsFrameLoader.cpp"
  [14] nsSubDocumentFrame::Destroy(this = 0x8dce948), line 739 in "nsFrameFrame.cpp"
  [15] nsFrameList::DestroyFrames(this = 0x8dd2a40), line 67 in "nsFrameList.cpp"

Comment 1

11 years ago
Also, I noticed that we are destroying an nsDocAccessible when a dialog is opened. It's strange, we receive a pagehide event even though nothing is being hidden, afaict.

Ginn. can you look at both at the same time?
(Assignee)

Comment 2

11 years ago
Can we change the hash key to nsIDocument?
We can translate it by using doc->GetPrimaryShell() and presShell->GetDocument()

Is there any underlying issue?

Comment 3

11 years ago
Ginn, that should be fine. How will it help?
(Assignee)

Comment 4

11 years ago
Created attachment 290514 [details] [diff] [review]
patch

I didn't address the problem in comment #1 in this patch.

Use mDocument as key, so that on pagehide, we can get docAcc and shutdown it without presShell.
Assignee: aaronleventhal → ginn.chen
Status: NEW → ASSIGNED
Attachment #290514 - Flags: review?(aaronleventhal)

Comment 5

11 years ago
Ginn, why is Destroy() no longer necessary? I think it was in there to avoid a problem where the doc accessible was getting destroyed and recreated in a Shutdown() procedure.

The rest looks good.

Comment 6

11 years ago
Comment on attachment 290514 [details] [diff] [review]
patch

Clearing request until Ginn provides answer.
Attachment #290514 - Flags: review?(aaronleventhal)
(Assignee)

Comment 7

11 years ago
Is there a case we call nsDocAccessible::Shutdown() but not nsDocAccessible::Destroy()?

I don't understand how nsDocAccessible::Destroy() works.
 

Comment 8

11 years ago
Ginn, I'm not totally sure, but I'd rather file a follow-up bug to investigate that during a refactoring phase after Firefox 3 ships.

Right now we shouldn't make an optional change like that unless it fixes something, especially since we don't know what the effects would be.
(Assignee)

Comment 9

11 years ago
Agree, but what should we do in Destroy()?

{
  nsresult rv = Shutdown();
  if (mDocument) {
    gGlobalDocAccessibleCache.Remove(static_cast<void*>(mDocument));
    mDocument = nsnull;
  }
  return rv;
}

looks good?

Is it an issue if we don't set mDocument to nsnull in Shutdown() ?

Comment 10

11 years ago
Right, we should set it to null in shutdown, so I guess Destroy() will have to use a temporary nsCOMPtr<nsIDocument> doc = mDocument before it calls Shutdown()
(Assignee)

Comment 11

11 years ago
I think it couldn't work.

1) If we always call nsDocAccessible::Shutdown() through nsDocAccessible::Destroy()
We don't really need nsDocAccessible::Destroy()

2) If we call nsDocAccessible::Shutdown() somewhere else, and mDocument is set to null, there's no chance for nsDocAccessible::Destroy() to save mDocument in a temporary variable.
(Assignee)

Comment 12

11 years ago
Created attachment 290655 [details] [diff] [review]
patch v2

What about to move the code of Destroy() to nsRootAccessible?
It's the only place that calls Desktroy() and it has a pointer of nsIDocument.
Attachment #290655 - Flags: review?(aaronleventhal)

Updated

11 years ago
Attachment #290655 - Flags: review?(aaronleventhal)
Attachment #290655 - Flags: review+
Attachment #290655 - Flags: approval1.9?

Updated

11 years ago
Attachment #290655 - Flags: approval1.9? → approval1.9+

Comment 13

11 years ago
Checked in for Ginn.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.