261450 - Mozilla crashes when changing DOM Nodes - [@ nsRange::TextOwnerChanged]

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Frank Wein [:mcsmurf]]

To reproduce
0. Get trunk build
1. Extract the attachment into
$MOZILLA_PROGRAM_FOLDER/chrome/nickblock/content/nickblock
2. Add the line
content,install,url,resource:/chrome/nickblock/content/nickblock/
to your installed-chrome.txt in your chrome-folder (sorry i dont have a
install.js script for this one yet...).
3. Start Mozilla
4. Go to http://forum.counter-strike.de/bb/thread.php?TID=71788&page=2&override=
5. Look out for the "Show Comment" link(s)
6. Click one or more of those links, now click the "Hide Comment" link(s) (i
think you only need to click one such link).
7. Repeat steps 5/6 from above (click Show Comment/Hide Comment/Show
Comment/scroll around(?)/Hide Comment/etc.). I have no exact steps to reproduce,
except you have to click this link often. Repeat until you crash or you get a
memory corruption (check with valgrind or similar).

The crash happens when executing this JavaScript:
function show_hide (node) {
if (node.style.visibility == "") {
node.style.visibility = "collapse";
node.parentNode.rows[node.rowIndex+1].style.visibility = "collapse";
alert(8);
node.previousSibling.getElementsByTagName("TD")[0].childNodes[0].childNodes[1].nodeValue
= "Show Comment";
node.previousSibling.getElementsByTagName("TD")[0].childNodes[0].childNodes[0].src
= "chrome://nickblock/content/plus.gif"}
else if (node.style.visibility == "collapse") {
node.style.visibility = "";
node.parentNode.rows[node.rowIndex+1].style.visibility = "";
node.previousSibling.getElementsByTagName("TD")[0].childNodes[0].childNodes[1].nodeValue
= "Hide Comment";
node.previousSibling.getElementsByTagName("TD")[0].childNodes[0].childNodes[0].src
= "chrome://nickblock/content/minus.gif"}}');
(node points to a Table Row element, shouldnt matter further here).

Stacktrace:
nsRange::TextOwnerChanged(nsIContent * 0x03ce05d0, int 0x00000000, int
0x0000000c, int 0x00000000) line 2292
nsGenericDOMDataNode::SetData(nsGenericDOMDataNode * const 0x03ce05d0, const
nsAString & {...}) line 346
nsTextNode::SetNodeValue(nsTextNode * const 0x03ce05d0, const nsAString & {...})
line 192
XPTC_InvokeByIndex(nsISupports * 0x03ce05d0, unsigned int 0x00000005, unsigned
int 0x00000001, nsXPTCVariant * 0x0012e9ac) line 102
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
0xaf09f588) line 2028 + 22 bytes
XPC_WN_GetterSetter(JSContext * 0x01fe2580, JSObject * 0x02f15b38, unsigned int
0x00000001, long * 0x02f15b5c, long * 0x0012ec08) line 1311 + 11 bytes
js_Invoke(JSContext * 0x00000001, unsigned int 0x00000001, unsigned int
0x00000002) line 1280 + 17 bytes
js_InternalInvoke(JSContext * 0x065b7ce4, JSObject * 0x0209f588, long
0x02f75380, unsigned int 0x00000000, unsigned int 0x00000001, long * 0x0012ee94,
long * 0x0012ee94) line 1377 + 13 bytes
js_InternalGetOrSet(JSContext * 0x01fe2580, JSObject * 0x0209f588, long
0x01f89748, long 0x02f75380, int 0x00000008, unsigned int 0x00000001, long *
0x0012ee94, long * 0x0012ee94) line 1420 + 21 bytes
js_SetProperty(JSContext * 0x01fe2580, JSObject * 0x0209f588, long 0x01f89748,
long * 0x0012ee94) line 2796 + 33 bytes
js_Interpret(JSContext * 0x01fe2580, long * 0x0012ef38) line 2529
[...]

In frame 0 theRangeList is
+	theRangeList	0x00000000

In frame 1 mParentPtrBits is
	mParentPtrBits	0x00000000

Content of the nsGenericDOMDataNode:
-	nsGenericDOMDataNode	{...}
-	nsITextContent	{...}
-	nsIContent	{...}
-	nsISupports	{...}
-	__vfptr	0x015ff928 const  nsTextNode::`vftable'{for `nsIDOMText'}
	[0x0]	0x014e474f nsTextNode::QueryInterface(const nsID &, void * *)
	[0x1]	0x015d9f4e nsHTMLTableCellElement::AddRef(void)
	[0x2]	0x0153916b nsXMLCDATASection::Release(void)
	sTabFocusModel	0x00000007
	mParentPtrBits	0x00000000
-	mRefCnt	{...}
	mValue	0x00050013
-	mText	{...}
+	m2b	0x00080105
+	m1b	0x00080105 ""
	mAllBits	0x00000041
-	mState	{...}
	mInHeap	0xffffffff
	mIs2b	0x00000000
	mIsBidi	0x00000000
	mLength	0x00000008
-	mDocument	0x00740068
-	nsISupports	{...}
+	__vfptr	0x206e6920
-	mDocumentTitle	{...}
-	nsSubstring	{...}
-	nsAString	{...}
	mVTable	0x20534f44
+	mData	0x65646f6d
	mLength	0x0a0d0d2e
	mFlags	0x00000024
-	mDocumentURI	{...}
-	nsCOMPtr_base	{...}
+	mRawPtr	0x00000000
-	mDocumentBaseURI	{...}
-	nsCOMPtr_base	{...}
+	mRawPtr	0x58d13427
-	mDocumentLoadGroup	{...}
-	nsCOMPtr_base	{...}
+	mRawPtr	0x0bbf5563
-	mDocumentContainer	{...}
-	nsCOMPtr_base	{...}
+	mRawPtr	0x0bbf5563
-	mCharacterSet	{...}
-	nsCSubstring	{...}
-	nsACString	{...}
	mVTable	0x0bbf5563
+	mData	0x0bac4a01 ""
	mLength	0x0bbf5561
	mFlags	0x0bb54a0c
	mCharacterSetSource	0x0bbf5566
-	mParentDocument	0x0bbb4a0c
+	nsISupports	{...}
+	mDocumentTitle	{...}
+	mDocumentURI	{...}
+	mDocumentBaseURI	{...}
+	mDocumentLoadGroup	{...}
+	mDocumentContainer	{...}
+	mCharacterSet	{...}
	mCharacterSetSource	CXX0030: Error: expression cannot be evaluated
	mParentDocument	CXX0030: Error: expression cannot be evaluated
	mRootContent	CXX0030: Error: expression cannot be evaluated
	mNextContentID	CXX0030: Error: expression cannot be evaluated
+	mBindingManager	{...}
	mNodeInfoManager	CXX0030: Error: expression cannot be evaluated
+	mPropertyTable	{...}
	mBidiEnabled	CXX0030: Error: expression cannot be evaluated
+	mContentLanguage	{...}
+	mContentType	{...}
+	mSecurityInfo	{...}
+	mRootContent	0x0bbf5561
	mNextContentID	0x0bbe5563
+	mBindingManager	{...}
+	mNodeInfoManager	0x0b8f7637
+	mPropertyTable	{...}
	mBidiEnabled	0x0bb953a4
+	mContentLanguage	{...}
+	mContentType	{...}
+	mSecurityInfo	{...}

Everything with CXX0030: Error: expression cannot be evaluated has not been
expanded.

Comment 1

[Frank Wein [:mcsmurf]]

Attachment: Extension needed to test

Note: The alert that appears when clicking the link mentioned in Comment 0
was/is for debugging only.

Comment 2

[Peter Van der Beken [:peterv]]

This could be mine: I combined the flags in nsGenericDOMDataNode (in bug 27382).
I might want to make HasRangeList actually look up in the hashtable OR I need to
check if every caller to HasRangeList does the lookup. Same for
HasEventListenerManager.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note that in this case mParentPtrBits is 0, though.  So we should be testing
false on HasRangeList().  Also note the bogus (I think, because there is no
parent) mDocument value.  It looks to me like the nsGenericDOMDataNode object
may be trashed here.  It would be nice to purify/valgrind this testcase...

Comment 4

[Frank Wein [:mcsmurf]]

Purify doesn't tell many new things as far as i can see this?:
[E] ABW: Array bounds write in WideCharToMultiByte {30 occurrences} <-- this is
from PR_GetHostByName [prnetdb.c:699] (or better said WideCharToMultiByte
[KERNEL32.DLL])
[W] UMR: Uninitialized memory read in WriteFile {59 occurrences} <-- disk cache
[E] NPR: NULL pointer read in nsVoidArray::Count(void)const {1 occurrence} <--
this is probably then the code, which trys to access count in the broken DOM node?
[E] EXU: Unhandled exception in nsVoidArray::Count(void)const {1 occurrence} <--
same as above
[I] Summary of all memory leaks... {706088 bytes, 18068 blocks}
[I] Exiting with code -1073741819 (0xc0000005)

if you need more info, i have saved the purify session (created with a debug
trunk build).

Comment 5

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 258078 has been marked as a duplicate of this bug. ***

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

Confirming bug.

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: Some protection

Probably doesn't fix this bug, but avoids a crash in the same code.

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 162409 [details] [diff] [review]
Some protection

Ignore SetHasProperties, that's from another patch and I removed it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 162409 [details] [diff] [review]
Some protection

No, this is wrong....  For example, the SetHasRangeList(PR_FALSE) call if we
discover we have no more range list will make us also think we can't possibly
have an event listener manager or properties (I realize the properties part is
not really part of this patch).

Comment 10

[Peter Van der Beken [:peterv]]

Yeah, and I'm out of bits :-/.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Can we steal bits off mDocument?  Or are there none left there?

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: Protection is good

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 162502 [details] [diff] [review]
Protection is good

>Index: content/base/src/nsGenericDOMDataNode.cpp

> nsGenericDOMDataNode::RangeRemove(nsIDOMRange* aRange)
>-        SetHasRangeList(PR_FALSE);
>+    SetHasRangeList();

No need to call SetHasRangeList() there -- we don't get into that code unless
the bit is set.  In any cas, we're _removing_ from the range, so that can't
create a range list for us...

r+sr=bzbarsky with that

Comment 14

[Peter Van der Beken [:peterv]]

I checked in attachment 162502 [details] [diff] [review]. A simplified testcase would be nice.

Comment 15

[Frank Wein [:mcsmurf]]

btw: It seems(!) this crash is gone now with the patch (i made some testing).

Comment 16

[Frank Wein [:mcsmurf]]

I'll resolve this one know, also after long testing i can't reproduce this crash
anymore...

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 264422 has been marked as a duplicate of this bug. ***

Comment 18

[Jay Patel [:jay]]

v.fixed per latest Talkback data.