127528 - Assertion because of index out of range

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Daniel Bratell]

I get an assertion in nsDocShell::GetChildAt because it tries to access item 23 
when there are none at all. This will cause a crash when the extra safe index 
check is removed from nsVoidArray (see bug 96108).

Since this code is called from the outside (js), shouldn't nsDOMWindowList::Item 
make sure the index is legal before using it? 

Stack:
nsDebug::Assertion(const char * 0x1010c5a4 `string', const char * 0x1010c5f4 
`string', const char * 0x1010c660 `string', int 78) line 291 + 13 bytes
nsVoidArray::ElementAt(int 23) line 78 + 35 bytes
nsDocShell::GetChildAt(nsDocShell * const 0x05499c50, int 23, 
nsIDocShellTreeItem * * 0x0012bd48) line 1991 + 16 bytes
nsDOMWindowList::Item(nsDOMWindowList * const 0x05faee10, unsigned int 23, 
nsIDOMWindow * * 0x0012bdb0) line 133 + 46 bytes
nsWindowSH::GetProperty(nsWindowSH * const 0x0380ce30, nsIXPConnectWrappedNative 
* 0x05cf1268, JSContext * 0x05b75cb0, JSObject * 0x052da408, long 47, long * 
0x0012cb00, int * 0x0012bde8) line 2803 + 53 bytes
XPC_WN_Helper_GetProperty(JSContext * 0x05b75cb0, JSObject * 0x052da408, long 
47, long * 0x0012cb00) line 784 + 47 bytes
js_GetProperty(JSContext * 0x05b75cb0, JSObject * 0x052da408, long 47, long * 
0x0012cb00) line 2438 + 313 bytes
js_Interpret(JSContext * 0x05b75cb0, long * 0x0012ccb0) line 2593 + 2246 bytes
js_Invoke(JSContext * 0x05b75cb0, unsigned int 1, unsigned int 2) line 805 + 13 
bytes
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJSClass * const 0x02df3270, 
nsXPCWrappedJS * 0x05d1fd38, unsigned short 3, const nsXPTMethodInfo * 
0x01583b20, nsXPTCMiniVariant * 0x0012d1a4) line 1193 + 21 bytes
nsXPCWrappedJS::CallMethod(nsXPCWrappedJS * const 0x05d1fd38, unsigned short 3, 
const nsXPTMethodInfo * 0x01583b20, nsXPTCMiniVariant * 0x0012d1a4) line 430
PrepareAndDispatch(nsXPTCStubBase * 0x05d1fd38, unsigned int 3, unsigned int * 
0x0012d254, unsigned int * 0x0012d244) line 115 + 31 bytes
SharedStub() line 139
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x05d1fe08, 
nsIDOMEvent * 0x01edb398, nsIDOMEventTarget * 0x051bc5b8, unsigned int 1, 
unsigned int 4) line 1217 + 20 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x051bc618, 
nsIPresContext * 0x0566ecc0, nsEvent * 0x0012f34c, nsIDOMEvent * * 0x0012ee24, 
nsIDOMEventTarget * 0x051bc5b8, unsigned int 4, nsEventStatus * 0x0012f348) line 
1734 + 36 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x051bc5b0, nsIPresContext * 
0x0566ecc0, nsEvent * 0x0012f34c, nsIDOMEvent * * 0x0012ee24, unsigned int 4, 
nsEventStatus * 0x0012f348) line 3453
nsXULElement::HandleDOMEvent(nsXULElement * const 0x04ea13d8, nsIPresContext * 
0x0566ecc0, nsEvent * 0x0012f34c, nsIDOMEvent * * 0x0012ee24, unsigned int 4, 
nsEventStatus * 0x0012f348) line 3434
nsXULElement::HandleDOMEvent(nsXULElement * const 0x04ea1440, nsIPresContext * 
0x0566ecc0, nsEvent * 0x0012f34c, nsIDOMEvent * * 0x0012ee24, unsigned int 4, 
nsEventStatus * 0x0012f348) line 3434
nsXULElement::HandleDOMEvent(nsXULElement * const 0x055fb088, nsIPresContext * 
0x0566ecc0, nsEvent * 0x0012f34c, nsIDOMEvent * * 0x0012ee24, unsigned int 4, 
nsEventStatus * 0x0012f348) line 3434
nsXULElement::HandleDOMEvent(nsXULElement * const 0x05359880, nsIPresContext * 
0x0566ecc0, nsEvent * 0x0012f34c, nsIDOMEvent * * 0x0012ee24, unsigned int 4, 
nsEventStatus * 0x0012f348) line 3434
nsXULElement::HandleDOMEvent(nsXULElement * const 0x05359a60, nsIPresContext * 
0x0566ecc0, nsEvent * 0x0012f34c, nsIDOMEvent * * 0x0012ee24, unsigned int 4, 
nsEventStatus * 0x0012f348) line 3434
nsXULElement::HandleChromeEvent(nsXULElement * const 0x05359a70, nsIPresContext 
* 0x0566ecc0, nsEvent * 0x0012f34c, nsIDOMEvent * * 0x0012ee24, unsigned int 4, 
nsEventStatus * 0x0012f348) line 4675 + 36 bytes
GlobalWindowImpl::HandleDOMEvent(GlobalWindowImpl * const 0x05422b48, 
nsIPresContext * 0x0566ecc0, nsEvent * 0x0012f34c, nsIDOMEvent * * 0x0012ee24, 
unsigned int 4, nsEventStatus * 0x0012f348) line 681
nsDocument::HandleDOMEvent(nsDocument * const 0x06619ab0, nsIPresContext * 
0x0566ecc0, nsEvent * 0x0012f34c, nsIDOMEvent * * 0x0012ee24, unsigned int 1, 
nsEventStatus * 0x0012f348) line 3224
nsEventStateManager::PreHandleEvent(nsEventStateManager * const 0x05058508, 
nsIPresContext * 0x0566ecc0, nsEvent * 0x0012f658, nsIFrame * 0x06706dbc, 
nsEventStatus * 0x0012f4d8, nsIView * 0x055d6908) line 472
PresShell::HandleEventInternal(nsEvent * 0x0012f658, nsIView * 0x055d6908, 
unsigned int 1, nsEventStatus * 0x0012f4d8) line 5997 + 43 bytes
PresShell::HandleEvent(PresShell * const 0x0561e02c, nsIView * 0x055d6908, 
nsGUIEvent * 0x0012f658, nsEventStatus * 0x0012f4d8, int 0, int & 1) line 5926 + 
25 bytes
nsViewManager::HandleEvent(nsView * 0x04f906b0, nsGUIEvent * 0x0012f658, int 0) 
line 2043
nsView::HandleEvent(nsViewManager * 0x0504f1a0, nsGUIEvent * 0x0012f658, int 0) 
line 306
nsViewManager::DispatchEvent(nsViewManager * const 0x0504f1a0, nsGUIEvent * 
0x0012f658, nsEventStatus * 0x0012f5c8) line 1857 + 23 bytes
HandleEvent(nsGUIEvent * 0x0012f658) line 83
nsWindow::DispatchEvent(nsWindow * const 0x04dcb01c, nsGUIEvent * 0x0012f658, 
nsEventStatus & nsEventStatus_eIgnore) line 856 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f658) line 877
nsWindow::DispatchFocus(unsigned int 105, int 1) line 4808 + 15 bytes
nsWindow::ProcessMessage(unsigned int 7, unsigned int 263680, long 0, long * 
0x0012fa2c) line 3647 + 23 bytes
nsWindow::WindowProc(HWND__ * 0x000904de, unsigned int 7, unsigned int 263680, 
long 0) line 1121 + 27 bytes
USER32! 77e12e98()
USER32! 77e139a3()
USER32! 77e1395f()
NTDLL! 77fa032f()
GlobalWindowImpl::Focus(GlobalWindowImpl * const 0x05422b4c) line 2053 + 25 
bytes
CheckForFocus(nsPIDOMWindow * 0x05422b5c, nsIFocusController * 0x0501db08, 
nsIDocument * 0x06619ab0) line 2509
PresShell::UnsuppressAndInvalidate() line 4780 + 35 bytes
PresShell::ProcessReflowCommands(int 1) line 6345
ReflowEvent::HandleEvent() line 6116
HandlePLEvent(ReflowEvent * 0x06809390) line 6130
PL_HandleEvent(PLEvent * 0x06809390) line 590 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x01094070) line 520 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x0003046e, unsigned int 49474, unsigned int 0, 
long 17383536) line 1071 + 9 bytes
USER32! 77e12e98()
USER32! 77e130e0()
USER32! 77e15824()
nsAppShellService::Run(nsAppShellService * const 0x0154f8e0) line 308
main1(int 1, char * * 0x003870d8, nsISupports * 0x00000000) line 1285 + 32 bytes
main(int 1, char * * 0x003870d8) line 1625 + 37 bytes
mainCRTStartup() line 338 + 17 bytes

Comment 1

[Daniel Bratell]

Right now I can reproduce it on http://svd.se/os/ but that might change when the 
contents of the news site change.

Comment 2

[Sivakiran Tummala]

changing priority to P2, nominating nsbeta1

Comment 3

[Johnny Stenback (:jst)]

IMO this should be fixed in the docshell code, crashing just because someone
asks for a child that isn't there seems silly to me, to say the least. It's not
like that code is *that* performance critical, the extra check for out-of-range
index won't hurt us noticeably.

Comment 4

[Adam Lock]

I can change that NS_ASSERT to an NS_ENSURE_TRUE, but I wonder why you're asking
for the 23rd element in an empty list...

Comment 5

[Adam Lock]

Inline patch - r/sr?

Index: mozilla/docshell/base/nsDocShell.cpp
===================================================================
RCS file: /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v
retrieving revision 1.430
diff -u -r1.430 nsDocShell.cpp
--- mozilla/docshell/base/nsDocShell.cpp	6 Mar 2002 07:46:48 -0000	1.430
+++ mozilla/docshell/base/nsDocShell.cpp	7 Mar 2002 19:24:59 -0000
@@ -2029,7 +2034,7 @@
 {
     NS_ENSURE_ARG_POINTER(aChild);
 
-    NS_ASSERTION(aIndex >= 0 && aIndex < mChildren.Count(),"Bad docshell
ChildAt index");
+    NS_ENSURE_TRUE(aIndex >= 0 && aIndex < mChildren.Count(),
NS_ERROR_INVALID_ARG);
     *aChild = (nsIDocShellTreeItem *) mChildren.ElementAt(aIndex);
     NS_IF_ADDREF(*aChild);
 

Comment 6

[Johnny Stenback (:jst)]

Don't you need to replace the ElementAt() call with SafeElementAt() too?

Comment 7

[Johnny Stenback (:jst)]

Oh, reacted too quickly there...

So my issue here is that IMO it's not an error worthy of getting an exception
(nor assertion) thrown in the face of the caller to ask for the child by index
greater than the number of children in the docshell. IMO that's a perfectly fine
thing to do, why not just accept that and return NS_OK and null out the out
parameter in that case? It's IMO not a programming error do ask such a question.

As for why this happens in mozilla, we're coming in through JS, any code that
does something like window[n] where n is a number creater than the number of
children in the docshell will end up asking for a child by index that isn't
there. We don't know the answer to the question why.

Comment 8

[Adam Lock]

IMO it is a programming error to ask the *docshell* for an element that is out
of range. The docshell is a private object, only called internally and therefore
it's clients should know better than to ask it for a dumb value.

With this said, I can understand that it would be a pain for the callers to
check the bounds in this instance, so I am happy to make it more tolerant. I
will use SafeElementAt, and make it generate warnings, not assertions for
invalid array positions.

Comment 9

[Adam Lock]

Attachment: New patch

New patch uses SafeElementAt and generates a warning for index values that are
out of bounds.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 73205 [details] [diff] [review]
New patch

sr=jst

Comment 11

[Conrad Carlen (not reading bugmail)]

Comment on attachment 73205 [details] [diff] [review]
New patch

r=ccarlen

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 73205 [details] [diff] [review]
New patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 13

[Adam Lock]

Fix checked in