458484 - mochitest-chrome, test_idcheck.xul : addressbook.xul leaks intermittently, due to having cards in outlook address book

Status: NEW

(SeaMonkey :: MailNews: Address Book & Contacts, defect)

Description

[Serge Gautherie (:sgautherie)]

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

(After working around bug 391318.)

"chrome://messenger/content/addressbook/addressbook.xul"
leaks
{
TEST-PASS | runtests-leaks | WARNING leaked 10752 bytes during test execution (under threshold set at 19057 bytes)
TEST-PASS | runtests-leaks | leaked 1 instance of RDFServiceImpl with size 272 bytes
TEST-PASS | runtests-leaks | leaked 4 instances of nsAbOutlookCard with size 88 bytes each (352 bytes total)
TEST-PASS | runtests-leaks | leaked 1 instance of nsGenericFactory with size 16 bytes
TEST-PASS | runtests-leaks | leaked 4 instances of nsMapiEntry with size 8 bytes each (32 bytes total)
TEST-PASS | runtests-leaks | leaked 4 instances of nsRDFResource with size 24 bytes each (96 bytes total)
TEST-PASS | runtests-leaks | leaked 268 instances of nsStringBuffer with size 8 bytes each (2144 bytes total)
TEST-PASS | runtests-leaks | leaked 140 instances of nsVariant with size 56 bytes each (7840 bytes total)
}

Comment 1

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081116 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/8b3caddca8f5
+http://hg.mozilla.org/comm-central/rev/ebcb28433bfe)

This seems to be worse now:

nsTraceRefcntImpl::DumpStatistics: 919 entries
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 13368 bytes during test execution (threshold set at 8405 bytes)
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 1 instance of RDFServiceImpl with size 272 bytes
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 5 instances of nsAbOutlookCard with size 88 bytes each (440 bytes total)
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 1 instance of nsGenericFactory with size 16 bytes
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 5 instances of nsMapiEntry with size 8 bytes each (40 bytes total)
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 5 instances of nsRDFResource with size 24 bytes each (120 bytes total)
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 335 instances of nsStringBuffer with size 8 bytes each (2680 bytes total)
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 175 instances of nsVariant with size 56 bytes each (9800 bytes total)

Comment 2

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081120 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/48ec53300d76
+http://hg.mozilla.org/comm-central/rev/9a933d62a4ec)

Better, somehow, probably after bug 464831:

TEST-PASS | runtests-leaks | WARNING leaked 12960 bytes during test execution (threshold set at 21477 bytes)
TEST-PASS | runtests-leaks | leaked 5 instances of nsAbCardProperty with size 56 bytes each (280 bytes total)
TEST-PASS | runtests-leaks | leaked 325 instances of nsStringBuffer with size 8 bytes each (2600 bytes total)
TEST-PASS | runtests-leaks | leaked 180 instances of nsVariant with size 56 bytes each (10080 bytes total)

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)

Looking for the triggering code line:
1) addressbook.xul
onload="OnLoadAddressBook()"
2) addressbook.js
2a) function OnLoadAddressBook()
setTimeout('OnLoadDirTree()', 0);
2b) function OnLoadDirTree()
SelectFirstAddressBook();
3) abCommon.js
3a) function SelectFirstAddressBook()
dirTree.view.selection.select(0);

Commenting out the latter line removes the leak...

Comment 4

[Mark Banner (:standard8)]

I just checked on Mac and we're not seeing any of these leaks starting up and shutting down TB with the address book window. As the code is pretty much the same, and given the previous leak logs, this really points to a problem in the Outlook directory interfaces.

As to commenting out that last line, the only thing I can think is when we select the dirTree, its causing some datasource initialisation behind the scenes which is kicking the Outlook directory into loading its cards.

I just had a quick look through the outlook code:

In nsAbOutlookDirectory::GetChildCards, this line seems suspect:

236             nsCOMPtr<nsISupports> oldElement = mCardList.Get(&newKey) ;

Compare it with nsDirectoryService::Get:

635     nsCOMPtr<nsISupports> value = dont_AddRef(mHashtable.Get(&key));

It looks to me like we're overdoing our addrefs as a result.

I can't test this at the moment, and it may not be the leak your seeing, but probably worth investigating (oh and if it is, I wouldn't be surprised if you have 5 cards in your outlook address book).

Comment 5

[[e-mail]]

(In reply to comment #4)
> I just had a quick look through the outlook code:
> 
> In nsAbOutlookDirectory::GetChildCards, this line seems suspect:
> 
> 236             nsCOMPtr<nsISupports> oldElement = mCardList.Get(&newKey) ;
I agree, Get() is already AddRef()ed and needs a dont_AddRef too.

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> [...] its causing some datasource initialisation behind the scenes

This is what I guessed; thanks for pinpointing the (exact) culprit code :-)

> It looks to me like we're overdoing our addrefs as a result.

I'll build a patch...

> [...] if you have 5 cards in your outlook address book).

I do ;->

Comment 7

[Serge Gautherie (:sgautherie)]

Modified
236             nsCOMPtr<nsISupports> oldElement = dont_AddRef(mCardList.Get(&newKey));
doesn't help (or is not enough).

The leak is created when
225         NS_NewArrayEnumerator(aCards, cardList);
is executed...

Comment 8

[[e-mail]]

(In reply to comment #7)
> The leak is created when
> 225         NS_NewArrayEnumerator(aCards, cardList);
> is executed...
I can't see how that leaks; if the caller leaked it would show up with other directories.

You could try adding "return retCode;" in various places to see how that affects the number of leaks. For instance, @227 I would expect no nsAbCardProperty leaks, but @237 I would expect 1 leak of nsAbCardProperty.

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> > 225         NS_NewArrayEnumerator(aCards, cardList);
> > is executed...
> I can't see how that leaks; if the caller leaked it would show up with other
> directories.

Don't ask me ;-/

> You could try adding "return retCode;" in various places [...]

That's exactly what I did: before 225, no leak; after 225, full leak :-|

Comment 10

[[e-mail]]

So, I just noticed that we always clear the card list every time, which means we never find an existing card, so we can't leak them there...

Comment 11

[Serge Gautherie (:sgautherie)]

It looks like bug 391318 comment 51 checkin "hided" this leak.

Mark, do you want to R.Fixed or keep it open to investigate further ?

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> It looks like bug 391318 comment 51 checkin "hided" this leak.

Confirmed, after commenting out that new code.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081221 SeaMonkey/2.0a3pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/b839ff0630c6
 +http://hg.mozilla.org/comm-central/rev/2a4c4c1f0feb + bug 469606 patch)

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #11)
> It looks like bug 391318 comment 51 checkin "hided" this leak.
> 
> Mark, do you want to R.Fixed or keep it open to investigate further ?

I very much doubt it "hided" it, more likely fixed it. If we were leaking at the new enumerator, then its most likely that leaking the rdf service (which is what bug 391318 fixed) would have caused this to leak as a side-effect.

Therefore I don't think we can do anything more here.

Comment 14

[Serge Gautherie (:sgautherie)]

V.WorksForMe then.

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)
> Therefore I don't think we can do anything more here.

Fwiw,
I still have a doubt, due to some kind of lack of explanation feeling;
I'd love something like bug 458486 comment 5.

Comment 16

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081222 SeaMonkey/2.0a3pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/31dbaf4ca0c4
 +http://hg.mozilla.org/comm-central/rev/113540504a46 + bug 469606 patch)

Er, actually, (comment 2) leak is still there:
only, it does not happen every time I run the --chrome test suite.
(That must have confused my yesterday tests :-<)

Comment 17

[Serge Gautherie (:sgautherie)]

It looks like bug 472302 fixed this bug, but it caused bug 473686.
Then, let's wait for that later bug...

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #16)
> it does not happen every time I run the --chrome test suite.
> (That must have confused my yesterday tests :-<)

(In reply to comment #17)
> It looks like bug 472302 fixed this bug, but it caused bug 473686.
> Then, let's wait for that later bug...

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090223 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/f6cdd2d6a9ea
 +http://hg.mozilla.org/comm-central/rev/720d3a1ea63d)

Up to this build, this bug looked like to have been fixed.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090228 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/f7f62131998d
 +http://hg.mozilla.org/comm-central/rev/7ea34ef19dc4)

With this build, I reproduce this intermittent leak "again".

(Don't know if it was just haphazard for a while or if something "regressed" recently: anyway...)

Comment 19

[Karsten Düsterloh]

Nice to have, but not a priority.

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)

Not seeing this very often for some time, but still there:

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090530 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/e44d9c0f4805
 +http://hg.mozilla.org/comm-central/rev/62f2c362a9a4 + bug 493008 patches)

{
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 12960 bytes during test execution
}

Comment 21

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1.1pre) Gecko/20090705 SeaMonkey/2.0b1pre] (comm-1.9.1-win32-unittest/1246823309) (W2Ksp4)
(http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b9cc253bcb8
 +http://hg.mozilla.org/comm-central/rev/035fc7c4ed38)

(Bug still there.)