197827 - nsSHEntry::AddChild() leaks

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Harshal Pradhan]

Shift+Reload on frameset pages appears to be leaking nsSHEntry objects.
For example just visit http://developer.kde.org/~sewardj/docs/ and hit shift+reload.

This seems to be because AddChild does not consider the possibility that there
may be pre-exisiting entries in the same offset possition.

Comment 1

[Harshal Pradhan]

Attachment: fix leak by releasing old entry if necessary

The surrounding code seems to have a mix of spaces and tabs. Let me know if I
should be using tabs insteads.

Comment 2

[Harshal Pradhan]

To me, since I have a patch 

Comment 3

[Alec Flett]

Comment on attachment 117473 [details] [diff] [review]
fix leak by releasing old entry if necessary

wow, damn. good catch.

We really should be using nsCOMArray for mChildren if it is really storing
strong references.

sr=alecf as long as radha agrees this is the right thing. (though again, I'd
prefer switching over to nsCOMArray entirely)

Comment 4

[Harshal Pradhan]

>We really should be using nsCOMArray for mChildren if it is really storing
>strong references.

Yeah, I looked at doing that. However, thats not totally trivial, and will take 
some thought. (e.g. nsCOMArray doesn't seem to have SafeElementAt() ... and so 
on.)

Comment 5

[Harshal Pradhan]

Comment on attachment 117473 [details] [diff] [review]
fix leak by releasing old entry if necessary

Setting the flag that alecf meant, and restoring review request that is still
pending.

Comment 6

[Harshal Pradhan]

Attachment: Convert mChildren to nsCOMArray

Well it turns out its quite simple to just do the nsCOMArray thing as alecf
suggested. So lets do this instead.

Comment 7

[Harshal Pradhan]

Attachment: fixed silly bug

Err ... that last patch had a really embarrasing bug. This one should hopefully
be better.

Comment 8

[Harshal Pradhan]

Comment on attachment 117598 [details] [diff] [review]
fixed silly bug

Sorry for all the spam. 

This one hopefully is good.

Comment 9

[Alec Flett]

Comment on attachment 117598 [details] [diff] [review]
fixed silly bug

nice cleanup!
again, sr=alecf, as long as radha is the reviewer.

Comment 10

[Radha on family leave (not reading bugmail)]

Comment on attachment 117598 [details] [diff] [review]
fixed silly bug

Presuming you have tested this well.

Comment 11

[Harshal Pradhan]

Browsing around some framset pages in a debug build with this patch throws up 
these assestions.

###!!! ASSERTION: nsVoidArray::ElementAt(index past end array) - note on bug 961
08: 'aIndex < Count()', file d:\trunk\mozilla\xpcom\ds\nsVoidArray.h, line 72
Break: at file d:\trunk\mozilla\xpcom\ds\nsVoidArray.h, line 72

---------------------------------------------------------------------------
xpcom.dll!nsDebug::Assertion(const char * aStr=0x100f3c00, const char * 
aExpr=0x100f3c50, const char * aFile=0x100f3cbc, int aLine=72)  Line 280 + 
0xdxpcom.dll!nsVoidArray::ElementAt(int aIndex=0)  Line 72 + 0x23	C++
xpcom.dll!nsCOMArray_base::ObjectAt(int aIndex=0)  Line 96	C++
xpcom.dll!nsCOMArray_base::ReplaceObjectAt(nsISupports * aObject=0x04b75a5c, 
int aIndex=0)  Line 89 + 0xc	C++
docshell.dll!nsCOMArray<nsISHEntry>::ReplaceObjectAt(nsISHEntry * 
aObject=0x04b75a5c, int aIndex=0)  Line 173	C++
docshell.dll!nsSHEntry::AddChild(nsISHEntry * aChild=0x04b75a5c, int 
aOffset=0)  Line 371	C++
---------------------------------------------------------------------------

The problem is that inspite of what the comment in the header file seems to 
suggest, nsCOMArray::ReplaceElementAt() is not safe for growing the array to an 
aribitrary length (like nsVoidArray::ReplaceElementAt()) is. 

I dont want to write ugly code to explicity insert nulls in the array when 
needed. So either we go back to the original patch, or maybe nsCOMArray needs 
to changed somewhat like so........


diff -u -r1.10 nsCOMArray.cpp
--- nsCOMArray.cpp	18 Jan 2003 14:04:23 -0000	1.10
+++ nsCOMArray.cpp	19 Mar 2003 11:28:12 -0000
@@ -86,7 +86,8 @@
 nsCOMArray_base::ReplaceObjectAt(nsISupports* aObject, PRInt32 aIndex)
 {
     // its ok if oldObject is null here
-    nsISupports *oldObject = ObjectAt(aIndex);
+    nsISupports *oldObject = 
+        NS_REINTERPRET_CAST(nsISupports*, mArray.SafeElementAt(aIndex));
 
     PRBool result = mArray.ReplaceElementAt(aObject, aIndex);
 

Thoughts?

Comment 12

[Alec Flett]

I like that patch to nsCOMArray. sr=alecf on that...

Comment 13

[Alec Flett]

though I suppose what would be nicer would be to have a SafeObjectAt() where the
casting happens.

Comment 14

[Radha on family leave (not reading bugmail)]

I can not comment on the patch to nsCOMArray. So, please go ahead with alec's
suggestion and any reviews he may want you to get. However, I feel that the
change to nsCOMArray should be tested well before checked in. 

Comment 15

[Harshal Pradhan]

Looks like alecf already checked in that nsCOMArray change which I needed, so 
the conversion patch above can go ahead.

> However, I feel that the change to nsCOMArray should be tested well before
> checked in. 

Radha, I've tested things like moving back and forth in history while browsing 
(and frameset pages specifically) quite a bit. Everything appears fine to me. 
Are there any other specific tests available that you would like me to do 
before I check this in?

Comment 16

[Radha on family leave (not reading bugmail)]

Since the change is deep in the management of the entries, I think  some basic
testing of frames pages (back, forward, reload, shift-reload, using the Go menu)
should do.  I guess you can check in what you have. 

Comment 17

[Harshal Pradhan]

I checked this in.