453137 - 'TestPipes' |TestChainedPipes()| leaks its streams

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080901134927 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <TestPipes.cpp>

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080901134927 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)

*[unrelated] Reorder a few lines.
*Add missing |NS_RELEASE(...);| !

***

Note that this function (and others) does not handle correctly its early returns.
If you want this fixed too, I would suggest "goto + NS_IF_RELEASE()" :-|
What do you think ?

Comment 2

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080901134927 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080901143156 SeaMonkey/2.0a1pre] (home, debug default) (W2Ksp4)

I wonder why |make check| reports a leak of 2 only, instead of 4 (when run directly).
I'll look into this after this patch...

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 336345 [details] [diff] [review]
(Av1) <TestPipes.cpp>

I'm not a good choice for general XPCOM first-review these days, due to my workload.

Comment 4

[Benjamin Smedberg]

Comment on attachment 336345 [details] [diff] [review]
(Av1) <TestPipes.cpp>

If you're going to alter this test, please refactor it with nsCOMPtrs.

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <TestPipes.cpp> [Checkin: Comment 7]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080917032624 Minefield/3.1b1pre] (home, optim default) (W2Ksp4)

Av1, with comment 4 suggestion(s).

I made it available in no-debug build too.

NB: First time I use nsCOMPtr, please double check.

PS: I don't know if you want to do something about the |#ifdef 0| code(s), either way...

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> I made it available in no-debug build too.

s/ no-debug / libxul-enabled /

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 339333 [details] [diff] [review]
(Av2) <TestPipes.cpp>
[Checkin: Comment 7]

http://hg.mozilla.org/mozilla-central/rev/cdfaac3ff235

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> PS: I don't know if you want to do something about the |#ifdef 0| code(s),

I filed bug 457046.

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> (From update of attachment 339333 [details] [diff] [review])
> http://hg.mozilla.org/mozilla-central/rev/cdfaac3ff235

PS:
I rewrote 2
|if (receiver == nsnull) return NS_ERROR_OUT_OF_MEMORY;|

Comment 10

[Jeremy Lea]

This change caused my builds with --enable-libxul and --enable-tests, to fail on Linux (Ubuntu), FreeBSD and OS X, with the same linker message about TestSegmentedBuffers() not begin a public symbol.  I think it is probably the makefile changes which have moved this test (so it might now be compiling).  The changes to the makefile also forgot to move TestPipes in CPP_UNIT_TESTS.

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)
> This change caused my builds with --enable-libxul and --enable-tests,

Reproduced with '--enable-debug --enable-libxul' actually ('--enable-tests' being default)
{
TestPipes.obj : error LNK2019: unresolved external symbol "void __cdecl TestSegmentedBuffer(void)" (?TestSegmentedBuffer@@YAXXZ) referenced in function _main
TestPipes.exe : fatal error LNK1120: 1 unresolved externals
}

Fixed by
http://hg.mozilla.org/mozilla-central/rev/60bd0ee88c78

> The changes to the makefile also forgot to move TestPipes in CPP_UNIT_TESTS.

I'll move this part to bug 457046 too.