Closed Bug 609574 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Disability Access APIs, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- final+

People

(Reporter: neil, Assigned: davidb)

Details

(Keywords: assertion)

Attachments

(1 file, 3 obsolete files)

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
possibly bug 608201 can fix it.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → bolterbugz
Status: NEW → ASSIGNED
Attachment #488880 - Flags: superreview?(neil)
Attachment #488880 - Flags: review?
blocking2.0: --- → final+
Attachment #488880 - Flags: review? → review?(ginn.chen)
Comment on attachment 488880 [details] [diff] [review]
patch

>no patches applied

Oops? ;-)
Attached patch patch (obsolete) — Splinter Review
The actual patch :) (thanks Marco)
Attachment #488880 - Attachment is obsolete: true
Attachment #488889 - Flags: superreview?(neil)
Attachment #488889 - Flags: review?
Attachment #488880 - Flags: superreview?(neil)
Attachment #488880 - Flags: review?(ginn.chen)
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?
Yes that's right, it is a few lines above where we (by recursion):
parentDocument->RemoveChildDocument
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.
(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?
No, just shut the children down in reverse order, i.e. from (Length - 1) to 0.
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.
Attachment #488889 - Flags: superreview?(neil)
Attachment #488889 - Flags: review?
Attached patch patch 2 (obsolete) — Splinter Review
Attachment #488889 - Attachment is obsolete: true
Attachment #489148 - Flags: review?(neil)
Attachment #489148 - Flags: review?(ginn.chen)
Comment on attachment 489148 [details] [diff] [review]
patch 2

>+  for (PRUint32 idx = childDocCount - 1; idx >= 0; idx--)
PRUint32 is always >= 0
Attachment #489148 - Flags: review?(neil) → review-
Attached patch patch 2.1Splinter Review
(In reply to comment #12)
> >+  for (PRUint32 idx = childDocCount - 1; idx >= 0; idx--)
> PRUint32 is always >= 0

Silly me.

How's this one look?
Attachment #489148 - Attachment is obsolete: true
Attachment #489165 - Flags: review?(neil)
Attachment #489148 - Flags: review?(ginn.chen)
Attachment #489165 - Flags: review?(neil) → review+
Attachment #489165 - Flags: review?(surkov.alexander)
Comment on attachment 489165 [details] [diff] [review]
patch 2.1

r=me
Attachment #489165 - Flags: review?(surkov.alexander) → review+
http://hg.mozilla.org/mozilla-central/rev/92f7bb63af1b

(Note: Commit log should have had Crash [@ nsDocAccessible::Shutdown()])
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.