226375 - Leaking nsIClassInfo objects

Status: RESOLVED FIXED

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

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Basically, this is because |nsDOMClassInfo::GetClassInfoInstance()| returns an
addref'd raw |nsIClassInfo*|

I'm going to fix |nsDOMClassInfo::GetClassInfoInstance()| to not addref its
return pointer, and just have the callers addref themselves.  This should not be
a problem because most callers already call NS_ADDREF() on |foundInterface| at
the end of their QI implementations anyway, but I'll go through everything and
make sure its kosher.

Comment 1

[Peter Van der Beken [:peterv]]

Pfff, just make it use NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO. :-)

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch

This also fixes a bunch of QI implementations in content to not suck as much.

Comment 3

[Peter Van der Beken [:peterv]]

+// Avoid adding new consumers of this macro if possible, since it breaks
+// the rules of COM as QI'ing to _interface multiple times will yield
+// different pointers.

I don't think that is correct, non-cached tear-offs are perfectly valid. QI'ing
to nsISupports should always return the same pointer.

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Peterv, QIing to an object multiple times must always return the same pointer
value per COM rules.  We intentionally break those rules because it would add
bloat to things like elements and events which we create a lot of.

Here is what Microsoft has to say:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/com/htm/com_1j8l.asp

Objects Must Have Identity

For any given object instance, a call to QueryInterface must always return the
same physical pointer value. This allows you to call
QueryInterface(IID_IUnknown, ...) on any two interfaces and compare the results
to determine whether they point to the same instance of an object.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Note that they use IUnknown for the example, but it is just an example.  Also
note that alecf (and others?) have been complaining to us to fix our QI
implementations to not return different pointers for these tearoffs for a while.

Comment 6

[Peter Van der Beken [:peterv]]

I'm sorry, but I think that documentation is wrong. See for example:
http://msdn.microsoft.com/library/en-us/dncomg/html/msdn_therules.asp ("Object
identity. It is required that any call to QueryInterface on any interface for a
given object instance for the specific interface IUnknown must always return the
same physical pointer value. This enables calling QueryInterface(IID_IUnknown,
...) on any two interfaces and comparing the results to determine whether they
point to the same instance of an object (the same COM object identity)"). See
also for example http://www.cs.washington.edu/homes/suciu/XMLTK/comlite.html
("Furthermore, there is no general requirement in COM that two calls to
QueryInterface for the same interface return the same interface pointer.  An
object may implement the interface as a tear-off  class which is allocated and
returned for every QueryInterface call, in which case you might have many
different pointers for the same object.  Hence, even if you have two pointers to
the same interface, you are not supposed to compare them.  IUnknown is the only
interface that is not allowed to be a tear-off, according to rule #1 above"). I
don't have the Don Box book with me but it has exactly the same and it details
how to implement non-cached tear-offs.
I'd be interested to know how the MS ATL classes manage to implement non-cached
tear-offs (compare COM_INTERFACE_ENTRY_TEAR_OFF with
COM_INTERFACE_ENTRY_CACHED_TEAR_OFF) with the restriction you're claiming.

Comment 7

[Peter Van der Beken [:peterv]]

You can get the COM specification from
http://www.microsoft.com/com/resources/comdocs.asp Sadly, the link to the html
version leads to nowhere, but the Word version has this:

"IUnknown::QueryInterface 
HRESULT IUnknown::QueryInterface(iid, ppv)
Return a pointer within this object instance that implements the indicated
interface. Answer NULL if the receiver does not contain an implementation of the
interface.
It is required that any query for the specific interface IUnknown always returns
the same actual pointer value, no matter through which interface derived from
IUnknown it is called. This enables the following identity test algorithm to
determine whether two pointers in fact point to the same object: call
QueryInterface(IID_IUnknown, ...) on both and compare the results.
In contrast, queries for interfaces other than IUnknown are not required to
return the same actual pointer value each time a QueryInterface returning one of
them is called. This, among other things, enables sophisticated object
implementors to free individual interfaces on their objects when they are not
being used, recreating them on demand (reference counting is a per-interface
notion, as is explained further below).  This requirement is the basis for what
is called COM identity."

So let's just remove the comment, ok?

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 137683 [details] [diff] [review]
Patch

Peterv, sure.  I can't seem to find any propagandism to suade me otherwise for
the present -- your links are pretty convincing.  I'll remove the comment
locally.  Care to r=?

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 137683 [details] [diff] [review]
Patch

>Index: dom/src/base/nsDOMClassInfo.h
>===================================================================

> #define NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(_class)                          \
>   if (aIID.Equals(NS_GET_IID(nsIClassInfo))) {                                \
>     foundInterface =                                                          \
>       nsDOMClassInfo::GetClassInfoInstance(eDOMClassInfo_##_class##_id);      \
>     NS_ENSURE_TRUE(foundInterface, NS_ERROR_OUT_OF_MEMORY);                   \

We need to set *aInstancePtr to null here on failure.

>Index: content/base/public/nsContentUtils.h
>===================================================================

> #define NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO(_class)                      \
>   if (aIID.Equals(NS_GET_IID(nsIClassInfo))) {                                \
>     foundInterface =                                                          \
>       nsContentUtils::GetClassInfoInstance(eDOMClassInfo_##_class##_id);      \
>     NS_ENSURE_TRUE(foundInterface, NS_ERROR_OUT_OF_MEMORY);                   \

We need to set *aInstancePtr to null here on failure.

>+// Avoid adding new consumers of this macro if possible, since it breaks
>+// the rules of COM as QI'ing to _interface multiple times will yield
>+// different pointers.

Drop this :-).

>+#define NS_INTERFACE_MAP_ENTRY_TEAROFF(_interface, _allocator)                \
>+  if (aIID.Equals(NS_GET_IID(_interface))) {                                  \
>+    foundInterface = NS_STATIC_CAST(_interface *, _allocator);                \
>+    NS_ENSURE_TRUE(foundInterface, NS_ERROR_OUT_OF_MEMORY);                   \

We need to set *aInstancePtr to null here on failure.

>Index: content/base/src/nsGenericElement.cpp
>===================================================================

>+NS_INTERFACE_MAP_BEGIN(nsGenericElement)
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIContent)

Want to move this one to the end of the list?

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>===================================================================

>@@ -2246,22 +2212,24 @@ nsGenericHTMLElement::GetAttr(PRInt32 aN

>+      {
>+        char cbuf[20];
>+        nscolor color = nscolor(value->GetColorValue());
>+        PR_snprintf(cbuf, sizeof(cbuf), "#%02x%02x%02x",
>+                    NS_GET_R(color), NS_GET_G(color), NS_GET_B(color));
>+        aResult.Assign(NS_ConvertASCIItoUCS2(cbuf));

Be sure to pick up the string change that I checked in recently here.

>Index: content/html/content/src/nsGenericHTMLElement.h
>===================================================================

> #define NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO_IF_TAG(_class, _tag)         \
>   if (mNodeInfo->Equals(nsHTMLAtoms::_tag) &&                                 \
>       aIID.Equals(NS_GET_IID(nsIClassInfo))) {                                \
>     foundInterface =                                                          \
>       nsContentUtils::GetClassInfoInstance(eDOMClassInfo_##_class##_id);      \
>     NS_ENSURE_TRUE(foundInterface, NS_ERROR_OUT_OF_MEMORY);                   \

We need to set *aInstancePtr to null here on failure.

>Index: content/xul/content/src/nsXULElement.cpp
>===================================================================

>+NS_INTERFACE_MAP_BEGIN(nsXULElement)
>+    NS_INTERFACE_MAP_ENTRY(nsIStyledContent)
>+    NS_INTERFACE_MAP_ENTRY(nsIContent)
>+    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMXULElement)

Want to move this one to the end of the list (and maybe use nsIStyledContent
instead of nsIDOMXULElement)?

Comment 10

[Peter Van der Beken [:peterv]]

Oh, and could you do nsXMLElement::QueryInterface too?

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Updated patch [Checked in: Comment 16]

> Oh, and could you do nsXMLElement::QueryInterface too?

I left that and a few other QI implementations alone because they have some
slightly different behaviors with PostQueryInterface stuff.  They could be done
as half macros, half code, but I'd prefer to try and macro-ize those more in a
separate patch.

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 138429 [details] [diff] [review]
Updated patch
[Checked in: Comment 16]

Marking r=peterv and trying alecf for sr.

Comment 13

[Alec Flett]

Comment on attachment 138429 [details] [diff] [review]
Updated patch
[Checked in: Comment 16]

Hmm.. I wonder if there is a way to avoid multiple return paths in this
NS_INTERFACE_MAP_ENTRY_DOM_CLASSINFO ?

See the problem is that early returns when using nsCOMPtr<> result in larger
code.. maybe we could do without the nsCOMPtr?

the problem is that this

nsCOMPtr<nsIFoo> foo = ...;
...
if (NS_FAILED(rv))
   return rv;
...
return NS_OK;
generates bigger code than

nsCOMPtr<nsIFoo> foo = ...;
...
if (NS_SUCCEEDED(rv)) {
 ...
}
return rv;

Compilers tend to generate two inline destructors for each nsCOMPtr<> in scope
for each return.

(not withholding review or anything, but maybe we can fix that in another bug?)

>   } else {
>-    aNameSpaceID = kNameSpaceID_None;
>     rv = mAttributes ? >mAttributes->GetAttribute(aAttribute, &value) :
>                        >NS_CONTENT_ATTR_NOT_THERE;
>   }

did you intend to remove this line (don't know it well enough to know)

sr=alecf

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Yes, I did intend to remove that line.  Notice the redundancy looking at the if
clause's contents:

>   if (aNameSpaceID != kNameSpaceID_None) {
...
>   } else {
>-    aNameSpaceID = kNameSpaceID_None;

I agree that the double return kind of sucks. Maybe we need another macro pair
in nsISupportsImpl.h, or similar, which adds an |nsresult rv = NS_OK;| in the
HEAD macro, which callers can override with a failure code, and the TAIL macro
can check the rv, returning there if its a failure code (the nsCOMPtr is already
out of scope by then and we don't have to worry about an early return). 
NS_INTERFACE_MAP_{BEGIN|END}_WITH_RV?  What do you think?  I'll file a bug to do
that if you like.

Comment 15

[Alec Flett]

that would be great..

Comment 16

[Serge Gautherie (:sgautherie)]

<http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey>

Could this '01/05/2004 16:36' check-in cause
"MacOSX Darwin 6.6 monkey Dep"
and
"WINNT 5.0 beast Dep"
to
'testfailed' !!?

Comment 17

[Serge Gautherie (:sgautherie)]

Addition to comment 16:

'Linux btek Dep' too :-(
And we don't know yet for the 4 others...

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

Yes.  You don't need to comment in bugs about tinderbox state changes unless it
goes a really long time unnoticed.  Part of the responsibilities of checking in
are to hang around and watch the tree after you check in, and make sure
everything stays green.  I checked in a patch which should fix the orange.

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

I thought I had marked this fixed already.