Closed Bug 405414 Opened 13 years ago Closed 13 years ago

nsDocAccessible is not destroyed when closing a tab

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

References

Details

(Keywords: memory-leak)

Attachments

(2 files)

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"
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?
Can we change the hash key to nsIDocument?
We can translate it by using doc->GetPrimaryShell() and presShell->GetDocument()

Is there any underlying issue?
Ginn, that should be fine. How will it help?
Attached patch patchSplinter Review
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)
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 on attachment 290514 [details] [diff] [review]
patch

Clearing request until Ginn provides answer.
Attachment #290514 - Flags: review?(aaronleventhal)
Is there a case we call nsDocAccessible::Shutdown() but not nsDocAccessible::Destroy()?

I don't understand how nsDocAccessible::Destroy() works.
 
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.
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() ?
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()
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.
Attached patch patch v2Splinter Review
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)
Attachment #290655 - Flags: review?(aaronleventhal)
Attachment #290655 - Flags: review+
Attachment #290655 - Flags: approval1.9?
Attachment #290655 - Flags: approval1.9? → approval1.9+
Checked in for Ginn.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.