151745 - nsXMLElement::Init() called with unstable ref-count

Status: RESOLVED FIXED

(Core :: XML, defect, P3)

Description

[Alex Fritze]

In two instances in nsXMLElement.cpp we create a fresh nsXMLElement and call 
its Init()-method with a ref-count of 0. This leads to a crash if we do any 
QI'ing & and subsequent releasing in Init(), as we have to do in the SVG 
implementation.
Solution: stabilize by AddRef'ing before calling Init().
Patch to follow.

Comment 1

[Alex Fritze]

Attachment: patch

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Some additional fixes

Thanks, I added some null pointer checks to these methods as well.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 87703 [details] [diff] [review]
Some additional fixes

>-  return it->QueryInterface(NS_GET_IID(nsIDOMNode), (void**) aReturn);
>+  rv = it->QueryInterface(NS_GET_IID(nsIDOMNode), (void**) aReturn);

You could use CallQueryInterface here I think. r=peterv.

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 87703 [details] [diff] [review]
Some additional fixes

sr=heikki

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fixed.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

It looks like this patch caused a massive increase in leaks on the brad-fast
tinderbox on MozillaTest.  Others haven't cycled yet.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 87703 [details] [diff] [review]
Some additional fixes

>Index: content/xml/content/src/nsXMLElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xml/content/src/nsXMLElement.cpp,v
>retrieving revision 1.82
>diff -u -r1.82 nsXMLElement.cpp
>--- content/xml/content/src/nsXMLElement.cpp	14 Jun 2002 18:52:16 -0000	1.82
>+++ content/xml/content/src/nsXMLElement.cpp	14 Jun 2002 18:57:51 -0000
>@@ -72,17 +72,20 @@
> nsresult
> NS_NewXMLElement(nsIContent** aInstancePtrResult, nsINodeInfo *aNodeInfo)
> {
>+  NS_ENSURE_ARG_POINTER(aInstancePtrResult);

Why bother with a null check?  If anyone passes null for an out param they
deserve to crash.

>+  *aInstancePtrResult = nsnull;
>+
>   nsXMLElement* it = new nsXMLElement();
> 
>   if (!it) {
>     return NS_ERROR_OUT_OF_MEMORY;
>   }
> 
>+  NS_ADDREF(it);

I think you need to remove the NS_ADDREF below to balance this one.

>   nsresult rv = it->Init(aNodeInfo);
> 
>   if (NS_FAILED(rv)) {
>-    delete it;
>-
>+    NS_RELEASE(it);
>     return rv;
>   }
>

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Oops, checked in this now:

-  NS_ADDREF(*aInstancePtrResult);
-