198531 - Valgrind shows nsSOAPEncoding leaks

Status: VERIFIED FIXED

(Core Graveyard :: Web Services, defect)

Description

[timeless]

boffo linux qt tinderbox xpcshell

compare:
mozhack@boffo:~/obj-i686-pc-linux-gnu-qt/dist/bin$ ./run-mozilla.sh
/usr/local/bin/valgrind --logfile-fd=3 ./xpcshell 3>/tmp/vg-soap.nul

Components.classes["@mozilla.org/xmlextras/soap/encoding;1"];quit()

mozhack@boffo:~/obj-i686-pc-linux-gnu-qt/dist/bin$ ./run-mozilla.sh
/usr/local/bin/valgrind --logfile-fd=3 ./xpcshell 3>/tmp/vg-soap.svc

Components.classes["@mozilla.org/xmlextras/soap/encoding;1"].getService();quit()

strip valgrind prefixes (cmd.exe):
for %a in (vg-soap*) do @perl -pi.bak -e "s/^.?==\d+==//;s/^.?--\d+--//;s/
record \d+ of \d+$/ record.../;s/   (?:at|by) 0x[\dA-F]{8}://" %a && rm %a.bak

compare them, notice that there's a lot of talk about Soap stuff. (attachment)

I set a breakpoint on nsSOAPEncoding's destructor and it wasn't hit.

getService v. createInstance was arbitrary, the leak is the same.

Comment 1

[timeless]

Attachment: valgrind diff output

Comment 2

[timeless]

Ok, partial picture:

References to nsSOAPEncoding
1. nsSOAPEncoding gives itself to nsSOAPEncodingRegistry.mEncodings which holds
a reference to nsSOAPEncoding
2. As a service the component manager owns one reference to nsSOAPEncoding
3. XPConnect (testcase) gets a reference
4. -unknown-

References to nsSOAPEncodingRegistry
1. nsSOAPEncoding.mRegistry

That's enough of a loop to cause a problem.

Comment 3

[John Gaunt (redfive)]

taking bug

Comment 4

[John Gaunt (redfive)]

Attachment: break cycle between encoding and encoding registry

This patch changes the addref/release of nsSOAPEncoding to be handled by the
nsSOAPEncodingRegistry. The registry becomse a *-style pointer in the encoding
and the encodings are *-style pointers in the hashtable which has been switched
from nsSupportsHashtable to nsObjectHashtable. So when all the encodings have
been released completely the registry destroys the encodings and goes away.

Comment 5

[harishd]

Comment on attachment 121457 [details] [diff] [review]
break cycle between encoding and encoding registry

I'm not a super reviewer. Requesting sr= from jst. I can r= in addition to
rayw's.

Comment 6

[John Gaunt (redfive)]

Attachment: v1.01 of soap leak patch

had 2 bad spots in the last patch, an NS_ENSURE() of a nsStringKey and I was
missing the NS_IMETHODIMP foo before the AddRef for nsSOAPEncoding.

I've also applied this patch to the new webservices directory and built it just
fine.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 121563 [details] [diff] [review]
v1.01 of soap leak patch

- In nsSOAPEncodingRegistry::~nsSOAPEncodingRegistry()
 {
-  /* destructor code */
-  delete mEncodings;
+  if (mEncodings)
+    delete mEncodings;

delete is null safe, i.e. that if check is redundant, remove it.

- In nsSOAPEncodingRegistry::AddEncoding():

+  if (!mEncodings) {
+    mEncodings = new nsObjectHashtable(nsnull, nsnull, DeleteEncodingEntry,
nsnull, 4);
+  }
+
+  if (!mEncodings)
+    return NS_ERROR_OUT_OF_MEMORY;

mEncodings will never be null except if the new inside the previous if fails,
therefore you should move this check inside the previous one. One less check to
make in the common case.

+nsSOAPEncoding::nsSOAPEncoding() : mEncoders(new nsSupportsHashtable()), 
+				    mDecoders(new nsSupportsHashtable()), 
+				    mMappedInternal(new nsSupportsHashtable()), 
+				    mMappedExternal(new nsSupportsHashtable())

I know this is the way we do this in the current code, so you're not adding
this, but I see no reason for any of those hashtables to be pointers, they
should be direct members of the nsSOAPEncoding class. That way we'd have less
traffic on the heap (i.e. fewer allocations) and we wouldn't need to worry
about error checking those initializers for OOM. Want to change that in this
bug, or you want to file a new bug for that?

- In nsSOAPEncoding.h:

+class nsSOAPEncodingRegistry : public nsISOAPEncodingRegistry
...
+  nsObjectHashtable *mEncodings;

Make that a dirrect reference, no need to allocate that object separately on
the heap (we even leak that hash in this code in some cases).

-  nsCOMPtr < nsISOAPEncoding > mRegistry;
+  nsISOAPEncodingRegistry *mRegistry;

Add a comment here explaining why this is a weak reference.

Fix that, and I'll have one more look.

Comment 8

[Ray Whitmer]

Comment on attachment 121563 [details] [diff] [review]
v1.01 of soap leak patch

It looks good.	You did a good job of cleanly redefine the registry instead of
deriving it from the encoding as I did.

You could clear up the registry constructor, as your comment points out -- what
to do if there were no associated style string.  The if that is there on
failure is just a placeholder since mEncodings was already null.  But that case
never happens.	The "new" gets called only after assigning that string.

Johnny's suggestions to avoid heap allocation are obviously good -- my errors. 
The rest looks good to me.

Comment 9

[John Gaunt (redfive)]

Attachment: v1.5

Switched to direct refs from pointers for all the hashtables

Added some comments about the ownership model in the .h

fixed the if() checks - they're gone, those pointers don't exist anymore

Comment 10

[John Gaunt (redfive)]

Comment on attachment 121750 [details] [diff] [review]
v1.5

requesting approval to land this leak patch in 1.4b

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 121750 [details] [diff] [review]
v1.5

NS_IMETHODIMP_(nsrefcnt) 
nsSOAPEncoding::AddRef(void)
{
  if(mRegistry)
    mRegistry->AddRef();
  return 0;
}

NS_IMETHODIMP_(nsrefcnt) 
nsSOAPEncoding::Release()
{
  if(mRegistry)
    mRegistry->Release();
  return 0;
}

The one thing that's a bit shady here is the return value from these methods.
In XPCOM, you're supposed to return the resulting reference count from your
AddRef() and Release() methods, these always returns 0. It's easy enough to
make them return the return value from the call on mRegistry, and that would
fix this, for the most part, but in the case where there is no registry, then
what do we do?

It seems like there's a possible leak in this code if we fail to create a new
registry when creating new SOAP encodings, is there anything we can do about
that?

sr=jst for the patch if you make the AddRef() and Release() calls return the
registry's refcount when there's a registry, but ideally we'd have a sollution
for the remaining problem as well. Ideas?

Comment 12

[John Gaunt (redfive)]

Yeah, I didn't particularly like returning 0. The problem with returning the
result from the mRegistry call is that it could be much larger than just the one
object's refcount, since you could have half a dozen encodings in the registry,
each of which reports its refcount to the registry. But, that may be better than 0. 

In the case of no registry, we're really in a lot of trouble anyway right? Since
that means the new() call failed most likely due to low memory. I could make the
registry an object. Maybe its better to replace the if() with an NS_ASSERTION
just to make it clear we really need the registry? The registry also gets
created in the constructor, which can't report failure to create it if something
should happen. Perhaps an Init() method on the encoding coupled with the
NS_ASSERTION?

Comment 13

[Benjamin Smedberg]

nsObjectHashtable is now deprecated in favor of nsClassHashtable... would it be
hard to use the new hashtable type?

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 121750 [details] [diff] [review]
v1.5

a=asa (on behalf of drivers) for checkin to 1.4b.

Comment 15

[Johnny Stenback (:jst)]

John, the most important thing with the return from Release() is to return 0
only when the object went away (not that anyone cares in this case, but that's
what matters in some cases), so returning mRegistry's refcount should be fine.
AFAIK AddRef() should never return 0 since you should never really get into such
a case :-)

I think the right thing to do wrt the null registry is to do what you suggested,
add an Init() method and prevent the usage of SOAP encodings that we fail to
initialize.

I'm cool with making those changes at a later stage tho, if you prefere that.

Comment 16

[Ray Whitmer]

Comment on attachment 121750 [details] [diff] [review]
v1.5

r=rayw

Comment 17

[John Gaunt (redfive)]

I've changed the AddRef/Release to return the value of the call to mRegisry's
AddRef/Release and the default return value to 1 (from 0). I'll file another bug
on adding the Init() method to get the call to |new nsSOAPEncodingRegistry()|
out of constructor.

Comment 18

[John Gaunt (redfive)]

Checked in!

Comment 19

[timeless]

vrfy fixed