471788 - 4 TUnit tests leak now

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081220 Minefield/3.2a1pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/e45938aee309)

No leak.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090101 Minefield/3.2a1pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/e807ec425ad7)

"Same" leak in:
test_384370.js
test_421483.js
test_457441-import-export-corrupt-bookmarks-html.js
test_bookmarks_html.js

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: test_bookmarks_html.js.log

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

thanks, may i ask you, to please provide in future a pushlog link like:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e45938aee309&tochange=e807ec425ad7
I think that's more useful to see which changesets are in the range.

This is due to bug 414715, i'm taking a look if i can find which part is leaking

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> thanks, may i ask you, to please provide in future a pushlog link like:

Thanks for the tip !
Do you type it manually, or is there a ui yet (bug 452882) ?

> This is due to bug 414715, i'm taking a look if i can find which part is
> leaking

Arf, that bug has already been checked in and backed out multiple times due to leaks: can't it be run through (all) the tests *before* checkin ? :-<

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #3)
> (In reply to comment #2)
> > thanks, may i ask you, to please provide in future a pushlog link like:
> 
> Thanks for the tip !
> Do you type it manually, or is there a ui yet (bug 452882) ?

manually

> > This is due to bug 414715, i'm taking a look if i can find which part is
> > leaking
> 
> Arf, that bug has already been checked in and backed out multiple times due to
> leaks: can't it be run through (all) the tests *before* checkin ? :-<

would be cool if an automation could tell "these tests: 1, 2, 3, are now leaking", the cumulative leak result is not always enough to see exactly what happened.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the leak is activated by:
this._idleService.addIdleObserver(this, BOOKMARKS_ARCHIVE_IDLE_TIME);
not using a getter for idleService solves the leak.

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> would be cool if an automation could tell "these tests: 1, 2, 3, are now
> leaking", the cumulative leak result is not always enough to see exactly what
> happened.

I fully agree, for reftests and mochitests, though I have no idea how this could be done for the time being.
Yet, TUnit tests are run individually, so no issue there. (except wanted bug 469523)

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

the idle observer in nsBrowserGlue is removed on quit-application, but XPCShell does not notify it. When we leak the observer we also leak the full BrowserGlue instance that holds the idleService reference.

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 355111 [details] [diff] [review]
patch v1.0

r=me. should add to bookmarks/tail_bookmarks.js as well?

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

notice this patch needs a refresh after Bug 470348 is pushed

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

added quit-application-granted (see comment above). Ready to land.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 355790 [details] [diff] [review]
patch v1.1

I'd like to fix this leak on branch too.  No actual code changes.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 355790 [details] [diff] [review]
patch v1.1

this is tests-only so does not need specific approval

Comment 14

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090108 Minefield/3.2a1pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/b98a20b1e91e)

V.Fixed

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/93190f5a8045

Comment 16

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv