232989 - Crash - nsAttrAndChildArray::Clear() line 527

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bob Clary [:bc] (inactive)]

I get a crash in recent nightly builds but not Mozilla 1.6 when running DOM 2
Core Test Suite. I have seen a couple of different stacks.

Example Stack

nsAttrAndChildArray::Clear() line 527 + 10 bytes
nsAttrAndChildArray::~nsAttrAndChildArray() line 77
nsGenericHTMLElement::~nsGenericHTMLElement() + 18 bytes
nsGenericHTMLContainerElement::~nsGenericHTMLContainerElement() + 15 bytes
nsHTMLDivElement::~nsHTMLDivElement() line 116 + 8 bytes
nsHTMLDivElement::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsGenericElement::Release(nsGenericElement * const 0x03ea4568) line 3038 + 218 bytes
nsHTMLDivElement::Release(nsHTMLDivElement * const 0x03ea4568) line 120 + 12 bytes
nsAttrAndChildArray::Clear() line 528 + 12 bytes
nsAttrAndChildArray::~nsAttrAndChildArray() line 77
nsGenericHTMLElement::~nsGenericHTMLElement() + 18 bytes
nsGenericHTMLContainerElement::~nsGenericHTMLContainerElement() + 15 bytes
nsHTMLDivElement::~nsHTMLDivElement() line 116 + 8 bytes
nsHTMLDivElement::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsGenericElement::Release(nsGenericElement * const 0x041104b0) line 3038 + 218 bytes
nsHTMLDivElement::Release(nsHTMLDivElement * const 0x041104b0) line 120 + 12 bytes
nsAttrAndChildArray::Clear() line 528 + 12 bytes
nsAttrAndChildArray::~nsAttrAndChildArray() line 77
nsGenericHTMLElement::~nsGenericHTMLElement() + 18 bytes
nsGenericHTMLContainerElement::~nsGenericHTMLContainerElement() + 15 bytes
nsHTMLTableCellElement::~nsHTMLTableCellElement() line 132 + 8 bytes
nsHTMLTableCellElement::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsGenericElement::Release(nsGenericElement * const 0x0417c380) line 3038 + 218 bytes
nsHTMLTableCellElement::Release(nsHTMLTableCellElement * const 0x0417c380) line
136 + 12 bytes
nsAttrAndChildArray::Clear() line 528 + 12 bytes
nsAttrAndChildArray::~nsAttrAndChildArray() line 77
nsGenericHTMLElement::~nsGenericHTMLElement() + 18 bytes
nsGenericHTMLContainerElement::~nsGenericHTMLContainerElement() + 15 bytes
nsHTMLTableRowElement::~nsHTMLTableRowElement() line 251 + 8 bytes
nsHTMLTableRowElement::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsGenericElement::Release(nsGenericElement * const 0x03fcabf0) line 3038 + 218 bytes
nsHTMLTableRowElement::Release(nsHTMLTableRowElement * const 0x03fcabf0) line
255 + 12 bytes
nsAttrAndChildArray::Clear() line 528 + 12 bytes
nsAttrAndChildArray::~nsAttrAndChildArray() line 77
nsGenericHTMLElement::~nsGenericHTMLElement() + 18 bytes
nsGenericHTMLContainerElement::~nsGenericHTMLContainerElement() + 15 bytes
nsHTMLTableElement::~nsHTMLTableElement() line 358 + 8 bytes
nsHTMLTableElement::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsGenericElement::Release(nsGenericElement * const 0x04011358) line 3038 + 218 bytes
nsHTMLTableElement::Release(nsHTMLTableElement * const 0x04011358) line 362 + 12
bytes
nsAttrAndChildArray::Clear() line 528 + 12 bytes
nsAttrAndChildArray::~nsAttrAndChildArray() line 77
nsGenericHTMLElement::~nsGenericHTMLElement() + 18 bytes
nsGenericHTMLContainerElement::~nsGenericHTMLContainerElement() + 15 bytes
nsHTMLDivElement::~nsHTMLDivElement() line 116 + 8 bytes
nsHTMLDivElement::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsGenericElement::Release(nsGenericElement * const 0x03f8e0f0) line 3038 + 218 bytes
nsHTMLDivElement::Release(nsHTMLDivElement * const 0x03f8e0f0) line 120 + 12 bytes
nsAttrAndChildArray::Clear() line 528 + 12 bytes
nsAttrAndChildArray::~nsAttrAndChildArray() line 77
nsGenericHTMLElement::~nsGenericHTMLElement() + 18 bytes
nsGenericHTMLContainerElement::~nsGenericHTMLContainerElement() + 15 bytes
nsHTMLTableCellElement::~nsHTMLTableCellElement() line 132 + 8 bytes
nsHTMLTableCellElement::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsGenericElement::Release(nsGenericElement * const 0x042a67c8) line 3038 + 218 bytes
nsHTMLTableCellElement::Release(nsHTMLTableCellElement * const 0x042a67c8) line
136 + 12 bytes
nsAttrAndChildArray::Clear() line 528 + 12 bytes
nsAttrAndChildArray::~nsAttrAndChildArray() line 77
nsGenericHTMLElement::~nsGenericHTMLElement() + 18 bytes
nsGenericHTMLContainerElement::~nsGenericHTMLContainerElement() + 15 bytes
nsHTMLTableRowElement::~nsHTMLTableRowElement() line 251 + 8 bytes
nsHTMLTableRowElement::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsGenericElement::Release(nsGenericElement * const 0x040252c8) line 3038 + 218 bytes
nsHTMLTableRowElement::Release(nsHTMLTableRowElement * const 0x040252c8) line
255 + 12 bytes
nsAttrAndChildArray::Clear() line 528 + 12 bytes
nsAttrAndChildArray::~nsAttrAndChildArray() line 77
nsGenericHTMLElement::~nsGenericHTMLElement() + 18 bytes
nsGenericHTMLContainerElement::~nsGenericHTMLContainerElement() + 15 bytes
nsHTMLTableElement::~nsHTMLTableElement() line 358 + 8 bytes
nsHTMLTableElement::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsGenericElement::Release(nsGenericElement * const 0x03ffb470) line 3038 + 218 bytes
nsHTMLTableElement::Release(nsHTMLTableElement * const 0x03ffb470) line 362 + 12
bytes
nsAttrAndChildArray::Clear() line 528 + 12 bytes
nsAttrAndChildArray::~nsAttrAndChildArray() line 77
nsGenericHTMLElement::~nsGenericHTMLElement() + 18 bytes
nsGenericHTMLContainerElement::~nsGenericHTMLContainerElement() + 15 bytes
nsHTMLDivElement::~nsHTMLDivElement() line 116 + 8 bytes
nsHTMLDivElement::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsGenericElement::Release(nsGenericElement * const 0x03f8e148) line 3038 + 218 bytes
nsHTMLDivElement::Release(nsHTMLDivElement * const 0x03f8e148) line 120 + 12 bytes
XPCJSRuntime::GCCallback(JSContext * 0x03d973b8, JSGCStatus JSGC_END) line 556 +
18 bytes
DOMGCCallback(JSContext * 0x03d973b8, JSGCStatus JSGC_END) line 1869 + 23 bytes
js_GC(JSContext * 0x03d973b8, unsigned int 0) line 1419 + 12 bytes
js_ForceGC(JSContext * 0x03d973b8, unsigned int 0) line 1000 + 13 bytes
JS_GC(JSContext * 0x03d973b8) line 1675 + 11 bytes
nsJSContext::Notify(nsJSContext * const 0x04069750, nsITimer * 0x04145718) line
1822 + 13 bytes
nsTimerImpl::Fire() line 386
nsTimerManager::FireNextIdleTimer(nsTimerManager * const 0x021f1cd8) line 616
nsAppShell::Run(nsAppShell * const 0x00a5b260) line 142
nsAppShellService::Run(nsAppShellService * const 0x00a59fa8) line 484
main1(int 1, char * * 0x002e2638, nsISupports * 0x00a34678) line 1291 + 32 bytes
main(int 1, char * * 0x002e2638) line 1678 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e814c7()

Variables:

-	child	0x040f1a18
+	nsISupports	{...}
+	mDocument	0xdddddddd
	mParentPtrBits	-572662307
	i	2
	mImpl->mBuffer[i]	0x040f1a18
-	this	0x03ea4584
-	mImpl	0x03ffbb30
	mAttrAndChildCount	7169
	mBufferSize	13
+	mMappedAttrs	0x00000000
-	mBuffer	0x03ffbb3c
	[0]	0x002ee2e8

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The 527 is does |child->SetParent(nsnull);|. Which should only crash if child is
null or a dangling pointer. Don't really see how any of that could happen since
we keep strong references in the array. Possibly if i've messed up the refcount
in the element or some such.

I'll look into this when i get into the office later today

Comment 2

[Boris Zbarsky [:bzbarsky]]

> +	mDocument	0xdddddddd

Whose mDocument is this?  Are we calling SetParent() on a deleted node somehow?

Comment 3

[Bob Clary [:bc] (inactive)]

I get a different stack when running this locally.

Local Stack:

nsAttrName::ReleaseInternalName() line 209 + 9 bytes
nsAttrName::~nsAttrName() line 81
nsAttrAndChildArray::InternalAttr::~InternalAttr() + 26 bytes
nsAttrAndChildArray::InternalAttr::`scalar deleting destructor'(unsigned int 0)
+ 15 bytes
nsAttrAndChildArray::Clear() line 522
nsAttrAndChildArray::~nsAttrAndChildArray() line 77
nsGenericContainerElement::~nsGenericContainerElement() + 18 bytes
nsXMLElement::~nsXMLElement() line 105 + 8 bytes
nsXMLElement::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsGenericElement::Release(nsGenericElement * const 0x03c8ff18) line 3038 + 218 bytes
nsXMLElement::Release(nsXMLElement * const 0x03c8ff18) line 144 + 12 bytes
nsAttrAndChildArray::Clear() line 528 + 12 bytes
nsAttrAndChildArray::~nsAttrAndChildArray() line 77
nsGenericContainerElement::~nsGenericContainerElement() + 18 bytes
nsXMLElement::~nsXMLElement() line 105 + 8 bytes
nsXMLElement::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsGenericElement::Release(nsGenericElement * const 0x03c6d730) line 3038 + 218 bytes
nsXMLElement::Release(nsXMLElement * const 0x03c6d730) line 144 + 12 bytes
ReleaseObjects(void * 0x03c6d730, void * 0x00000000) line 152 + 18 bytes
nsVoidArray::EnumerateForwards(int (void *, void *)* 0x1001ebc0
ReleaseObjects(void *, void *), void * 0x00000000) line 648 + 21 bytes
nsCOMArray_base::Clear() line 160
nsCOMArray<nsIContent>::Clear() line 211
nsDocument::~nsDocument() line 547
nsXMLDocument::~nsXMLDocument() line 198 + 36 bytes
nsXMLDocument::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsDocument::Release(nsDocument * const 0x0455ffb8) line 637 + 233 bytes
nsXMLDocument::Release(nsXMLDocument * const 0x0455ffb8) line 211 + 12 bytes
SubDocClearEntry(PLDHashTable * 0x031dbb88, PLDHashEntryHdr * 0x03bdb8cc) line
1152 + 27 bytes
PL_DHashTableFinish(PLDHashTable * 0x031dbb88) line 343 + 16 bytes
PL_DHashTableDestroy(PLDHashTable * 0x031dbb88) line 185 + 9 bytes
nsDocument::~nsDocument() line 526 + 15 bytes
nsHTMLDocument::~nsHTMLDocument() line 319 + 190 bytes
nsHTMLDocument::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsDocument::Release(nsDocument * const 0x02f16168) line 637 + 233 bytes
nsHTMLDocument::Release(nsHTMLDocument * const 0x02f16168) line 322 + 12 bytes
XPCJSRuntime::GCCallback(JSContext * 0x0419ae40, JSGCStatus JSGC_END) line 556 +
18 bytes
DOMGCCallback(JSContext * 0x0419ae40, JSGCStatus JSGC_END) line 1869 + 23 bytes
js_GC(JSContext * 0x0419ae40, unsigned int 0) line 1419 + 12 bytes
js_ForceGC(JSContext * 0x0419ae40, unsigned int 0) line 1000 + 13 bytes
JS_GC(JSContext * 0x0419ae40) line 1675 + 11 bytes
nsJSContext::Notify(nsJSContext * const 0x0473a7c8, nsITimer * 0x038b31d8) line
1822 + 13 bytes
nsTimerImpl::Fire() line 386
nsTimerManager::FireNextIdleTimer(nsTimerManager * const 0x021e1f80) line 616
nsAppShell::Run(nsAppShell * const 0x00a6d248) line 142
nsAppShellService::Run(nsAppShellService * const 0x00a6cf98) line 484
main1(int 1, char * * 0x002e2638, nsISupports * 0x009af410) line 1291 + 32 bytes
main(int 1, char * * 0x002e2638) line 1678 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL3

  void ReleaseInternalName()
  {
    // Since both nsINodeInfo and nsIAtom inherit nsISupports as its first
    // interface we can safely assume that it's first in the vtable
    nsISupports* name = NS_REINTERPRET_CAST(nsISupports *,
      mBits & ~NS_ATTRNAME_NODEINFO_BIT);

->    NS_RELEASE(name);
  }

Variables:

	mBits	74929337
-	name	0x047754b8
+	__vfptr	0xdddddddd
-	this	0x03db719c
	mBits	74929337

Comment 4

[Bob Clary [:bc] (inactive)]

bz, in the first stack the mDocument belongs to child. In the second stack
__vfptr belongs to name.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, definitly sounds like somewhere along the stack we're starting to delete
deleted objects. My guess is that i've messed up the refcounting somewhere in
Insert/Remove/Replace/AppendChild. Note that the way we did refcounting changed
slightly when I started to use this class.

It used to be that the elements had to NS_ADDREF and NS_RELEASE the elements in
the child-array. With the new class the array handles the refcounting itself.

Comment 6

[Bob Clary [:bc] (inactive)]

To see a variety of stacks pick either IFRAME or Mozilla XML Implementations and
any of the Content Types and run the tests from
<http://dom-ts.bclary.com/dist-dom2-core/ecmascript/level2/core/alltests.html>

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I wonder if nsAttrAndChildArray::ReplaceChildAt needs to do the same dance
nsCOMPtr does when setting the new child. Though I can't think of a senario
where releasing a child would end up deleting the node itself. Doesn't hurt to
protect against I guess, or at least assert on.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, same goes for RemoveChild and Clear. But I still can't see how it could happen.

Comment 9

[Bob Clary [:bc] (inactive)]

correct updated urls. sorry for the spam.

http://dom-ts.bclary.com/build/ecmascript/jsunit/testRunner.html?testpage=http%3A%2F%2Fdom-ts.bclary.com%2Fbuild%2Fecmascript%2Flevel2%2Fcore%2Falltests.html&implementation=iframe&contentType=text%2Fxml

http://dom-ts.bclary.com/build/ecmascript/level2/core/alltests.html

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: reduced testcases nodesetprefix07/08 and data files

Open either nodesetprefix and either reload till you crash or start visiting
other documents to crash with different stacks.

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: reduced testcases nodesetprefix07/08 and *correct* data files

original zip had svg and not xml data.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch to fix

The problem was the nsDOMAttribute::mNodeInfo is a rawpointer and
nsDOMAttribute::SetPrefix doesn't release the new value or addref the old.

Why this is a new crash is beyond me, it should have always crashed.

Made mNodeInfo into an nsCOMPtr and used |swap| to save a cycle or two.

Comment 13

[Brian Ryner (not reading)]

Comment on attachment 141114 [details] [diff] [review]
patch to fix

r+sr=bryner

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Fix checked in. Thanks goes out to Bob for finding this and providing the
reduced testcase!!

Comment 15

[Bob Clary [:bc] (inactive)]

verified fixed.