609574 - ###!!! ASSERTION: invalid array index: 'i < Length()', file nsTArray.h, line 339

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[[e-mail]]

nsDocAccessible::Shutdown()
...
  PRUint32 childDocCount = mChildDocuments.Length();
  for (PRUint32 idx = 0; idx < childDocCount; idx++)
    mChildDocuments[idx]->Shutdown();
childDocCount = 2 but mChildDocuments.Length() is now 1, so possibly the first document removed itself from the array when it was shut down? (Examination of the memory shows that mChildDocuments[1] == mChildDocuments[0] at this point.)

Normally I just ignore this assertion and it continues, but sometimes I've crashed, which suggests that there were more than two child documents meaning that it could have attempted to destroy one of them twice.

Stack:
nsDocAccessible::Shutdown
nsDocAccessibleWrap::Shutdown
nsRootAccessible::Shutdown
nsAccDocManager::HandleEvent
nsEventListenerManager::HandleEventSubType
nsEventListenerManager::HandleEventInternal
nsEventListenerManager::HandleEvent
nsEventTargetChainItem::HandleEvent
nsEventTargetChainItem::HandleEventTargetChain
nsEventDispatcher::Dispatch
nsEventDispatcher::DispatchDOMEvent
nsDocument::DispatchPageTransition
nsDocument::OnPageHide
DocumentViewerImpl::PageHide
nsDocShell::FirePageHideNotification
nsDocShell::Destroy
nsXULWindow::Destroy
nsWebShellWindow::Destroy
nsWebShellWindow::HandleEvent
nsWindow::DispatchEvent
nsWindow::DispatchWindowEvent
nsWindow::DispatchStandardEvent
nsWindow::ProcessMessage
nsWindow::WindowProc

Comment 1

[Ginn Chen]

possibly bug 608201 can fix it.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch

I caught a stack for this one today and Alexander and I wrote this patch. This is a bit ugly but protects us from the case where the child document has no parent.

Comment 3

[Marco Zehe (:MarcoZ)]

Comment on attachment 488880 [details] [diff] [review]
patch

>no patches applied

Oops? ;-)

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch

The actual patch :) (thanks Marco)

Comment 5

[[e-mail]]

Comment on attachment 488889 [details] [diff] [review]
patch

>+  PRUint32 childDocCountOld = mChildDocuments.Length();
>+  for (PRUint32 idx = 0; idx < mChildDocuments.Length();) {
>     mChildDocuments[idx]->Shutdown();
>+    if (mChildDocuments.Length() == childDocCountOld) {
>+      NS_ERROR("Child document has no parent!");
So Shutdown() is expected to change the length?

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Yes that's right, it is a few lines above where we (by recursion):
parentDocument->RemoveChildDocument

Comment 7

[[e-mail]]

Ah, so in fact the old code was making a mistaken assumption.

Is there any reason to shutdown the documents in order? Shutting them down in reverse order is an easier way to avoid this sort of issue.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #7)
> Ah, so in fact the old code was making a mistaken assumption.
> 

Right.

> Is there any reason to shutdown the documents in order? Shutting them down in
> reverse order is an easier way to avoid this sort of issue.

I don't know of a reason. So you are suggesting shutdown the children before the parents right?

Comment 9

[[e-mail]]

No, just shut the children down in reverse order, i.e. from (Length - 1) to 0.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 488889 [details] [diff] [review]
patch

(In reply to comment #9)
> No, just shut the children down in reverse order, i.e. from (Length - 1) to 0.

Thank you, that's a great idea of course.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 2

Comment 12

[[e-mail]]

Comment on attachment 489148 [details] [diff] [review]
patch 2

>+  for (PRUint32 idx = childDocCount - 1; idx >= 0; idx--)
PRUint32 is always >= 0

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 2.1

(In reply to comment #12)
> >+  for (PRUint32 idx = childDocCount - 1; idx >= 0; idx--)
> PRUint32 is always >= 0

Silly me.

How's this one look?

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 489165 [details] [diff] [review]
patch 2.1

r=me

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

http://hg.mozilla.org/mozilla-central/rev/92f7bb63af1b

(Note: Commit log should have had Crash [@ nsDocAccessible::Shutdown()])