474831 - Add a browser chrome test to check Places Library does not leak when opened and closed

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Drew Willcoxon :adw]

STR:
Run the attached browser chrome test.  It just opens the Places window, waits for it to load, and then finishes.

Comment 1

[Drew Willcoxon :adw]

Attachment: test case

Comment 2

[Drew Willcoxon :adw]

Possible dupe of bug 419721?  How fast the Places window is closed after it's opened does not matter here, though.

Comment 3

[Marco Bonardo [:mak]]

my fear is that the fix for bug 473976 could have broken the fix we put for the library leak in bug 433231.
So i suggest to backout that patch locally to see if the leak is fixed, in case it is, we should backout and reopen bug 473976.

Comment 4

[Marco Bonardo [:mak]]

Adw, i'll take care of the backout, please produce a reviewable test case so we can get regressions on that leak.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 358249 [details] [diff] [review]
test case

>+++ b/browser/components/places/tests/browser/browser_474831.js
r=sdwilsh with a name change.  How about something like browser_library_open_leak.js

Comment 6

[Drew Willcoxon :adw]

Attachment: for checkin

Changes re: comment 5

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Not gonna block on a test, but the good news is that you don't need approval to check in tests, so yay!

Comment 8

[Marco Bonardo [:mak]]

sorry for late, i did not look at this patch recently, the test should do cleanup before exiting, so close the Library window.

moreover opening and closing the Library is not enough to cause the leak (unless you have evidence of the leak doing this), the steps are:

1. open the Library
2. select a bookmark item
3. select an history item
4. close the Library

see bug 433231, bug 423185, bug 431568 for reference

Comment 9

[Marco Bonardo [:mak]]

if you by chance see that open/close fast (the test can be really fast) does not leak, please mark bug 419721 accordingly

Comment 10

[Shawn Wilsher :sdwilsh]

The leak was caused by simply opening and closing the library.

Comment 11

[Marco Bonardo [:mak]]

Not the one fixed in Bug 433231, that leak is due to the fact we add an observer but we never remove it if an history item is selected after a bookmarked item. So i can't see how opening and closing Library could leverage it.

Comment 12

[Shawn Wilsher :sdwilsh]

Then he found a different leak.  The test he wrote originally did in fact trigger a leak.

Comment 13

[Marco Bonardo [:mak]]

so, it should still trigger it since we did not fix it, what am i missing?

Comment 14

[Marco Bonardo [:mak]]

hwv checking if the test works is quite easy, locally add the patch for bug 473976, then it should leak. If the test works for another leak, then we need 2 tests.

Comment 15

[Drew Willcoxon :adw]

(In reply to comment #13)
> so, it should still trigger it since we did not fix it, what am i missing?

I thought you backed out the patch that was causing this leak?

Comment 16

[Marco Bonardo [:mak]]

the leak was present before Bug 433231
Bug 433231 has fixed it
Bug 473976 has practically nulled-out the leak fix (we need test yay!)
Bug 473976 has been backed out

so to reproduce the leak the options are: locally backout Bug 433231 or locally add Bug 473976.

Comment 17

[Shawn Wilsher :sdwilsh]

we already established that bug 473976 caused the leak...

Comment 18

[Marco Bonardo [:mak]]

Trying to to understand why a leak regression causes a different leaking path is crazy, so i've added in-testsuite? to Bug 433231, that path will have a separate test, thanks

Comment 19

[Drew Willcoxon :adw]

Attachment: v3.0

Now closes the window before finishing.

Comment 20

[Marco Bonardo [:mak]]

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

Comment 21

[Marco Bonardo [:mak]]

additional changeset to unregister window watcher observer:
http://hg.mozilla.org/mozilla-central/rev/30325dd9669f

Comment 22

[Tracy Walker [:tracy]]

verified per code checkin

Comment 23

[Drew Willcoxon :adw]

Attachment: mozilla-1.9.1 v1

Marco, I'm trying to get this on 1.9.1 to clear the way for bug 451151, bug 469436, and the others you mention in bug 469436 comment 29.  This is my first time backporting, sorry if I'm doing something wrong.

This patch applies cleanly to 1.9.1 and it also incorporates your small additional changeset in comment 21.

Comment 24

[Drew Willcoxon :adw]

Comment on attachment 368078 [details] [diff] [review]
mozilla-1.9.1 v1

approval1.9.1?
Low risk (with the caveat this is my first time assessing risk :)  This patch is a simple test that's baked on trunk since 1/30/2009.  See comment 23 for motivation.

Comment 25

[Marco Bonardo [:mak]]

Comment on attachment 368078 [details] [diff] [review]
mozilla-1.9.1 v1

does not need approval, tests-only.

Comment 26

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/019a8397a1ca

Comment 27

[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