511735 - xpcshell-tests: 4 test_*_removeDataFromDomain_*.js leaks 700 bytes, notably 4 StatementParams

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Serge Gautherie (:sgautherie)]

Firefox trunk, 3 platforms:

The 4 tests:
test_privatebrowsingwrapper_removeDataFromDomain.js
test_privatebrowsingwrapper_removeDataFromDomain_activeDownloads.js
test_removeDataFromDomain.js
test_removeDataFromDomain_activeDownloads.js

NB: This is "in addition" to intermittent bug  471792.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

This was because nsPlacesDBFlush did not get a chance to finalize its storage statements.  Sending out "quit-application" allows it to cleanly shut down and prevent any leaks.

Comment 2

[Marco Bonardo [:mak]]

you could just use do_register_cleanup(yourcleanupmethod); in the head file, the xpcshell test harness will take care to call it after any test in that folder finishes.

see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/tail_bookmarks.js for complete Places finalization we are actually using to prevent any possible leak in xpcshell (actually i think for PB you really only need the quit-application call you already did). Just linking for future in case some leak would still be around.

Notice if we could implement bug 478718 all of thi would be pointless, unfortunatly that has still issues.

Comment 3

[(no longer active)]

(In reply to comment #2)
> you could just use do_register_cleanup(yourcleanupmethod); in the head file,
> the xpcshell test harness will take care to call it after any test in that
> folder finishes.

The thing is that some of the test need to test things specific to shutdown events, so I can't really do something like that which should affect all tests in that folder.

> see
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/tail_bookmarks.js
> for complete Places finalization we are actually using to prevent any possible
> leak in xpcshell (actually i think for PB you really only need the
> quit-application call you already did). Just linking for future in case some
> leak would still be around.

Thanks for the link.  I prefer not to add any extra stuff to shutdownPlaces unless it proves necessary, but I'll make sure to keep this in mind for future.

> Notice if we could implement bug 478718 all of thi would be pointless,
> unfortunatly that has still issues.

Yes, agreed.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 398919 [details] [diff] [review]
Patch (v1)

>diff --git a/browser/components/privatebrowsing/test/unit/head_privatebrowsing.js b/browser/components/privatebrowsing/test/unit/head_privatebrowsing.js
>--- a/browser/components/privatebrowsing/test/unit/head_privatebrowsing.js
>+++ b/browser/components/privatebrowsing/test/unit/head_privatebrowsing.js
>@@ -111,16 +111,25 @@ function cleanUp()
>     let file = dirSvc.get("ProfD", Ci.nsIFile);
>     file.append(files[i]);
>     if (file.exists())
>       file.remove(false);
>   }
> }
> cleanUp();
> 
>+/**
>+ * Shut down the places component in order to prevent leaks
>+ */

just make this comment more self-explaining, since you say "shutdown places" and later you
just call quit-application.
Some brief description of what happens (Places statements are finalized at quit-application) would help.

Comment 5

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9b83b94d7336

Comment 6

[Serge Gautherie (:sgautherie)]

V.Fixed, per tinderboxes.

Comment 7

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/01b63f8158a0
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/13b92f6a1720

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Tests are checked into 1.9.1 but I don't see results on any of the tinderboxes running unit tests.

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> Tests are checked into 1.9.1 but I don't see results on any of the tinderboxes
> running unit tests.

See for example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5-Unittest/1254263388.1254265355.24548.gz&fulltext=1
Linux mozilla-1.9.1 test everythingelse on 2009/09/29 15:29:48

TEST-PASS | /builds/slave/mozilla-1.9.1-linux-unittest-everythingelse/build/xpcshell/tests/test_privatebrowsing/unit/test_privatebrowsingwrapper_removeDataFromDomain.js | test passed
TEST-PASS | /builds/slave/mozilla-1.9.1-linux-unittest-everythingelse/build/xpcshell/tests/test_privatebrowsing/unit/test_privatebrowsingwrapper_removeDataFromDomain_activeDownloads.js | test passed
TEST-PASS | /builds/slave/mozilla-1.9.1-linux-unittest-everythingelse/build/xpcshell/tests/test_privatebrowsing/unit/test_removeDataFromDomain.js | test passed
TEST-PASS | /builds/slave/mozilla-1.9.1-linux-unittest-everythingelse/build/xpcshell/tests/test_privatebrowsing/unit/test_removeDataFromDomain_activeDownloads.js | test passed
}

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Doh. Bitten by the changes to the Tinderboxes again. Thanks.

Marking as verified for 1.9.1.