433231 - Places Library leaks the nsGlobalWindow when closed with a history entry selected

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Nickolay_Ponomarev]

Attachment: patch

1. Open the Places Library
2. Click History
3. Click a history entry
4. Quit Firefox

-> the organizer window is leaked

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008051011 Minefield/3.0pre

This is due to a mistake in the code that handles adding/removing a bookmarks service observer, "this._itemId" in the code the attached patch changes can be != -1 when the observer is added and == -1 in the unload event handler.

I'm not really familiar with this code and haven't run the unit tests (since http://developer.mozilla.org/en/docs/Mozilla_automated_testing doesn't have the step-by-step instructions and I don't have the time to figure it out). Would appreciate if someone could test the change and get this in for 1.9+.

Comment 1

[Samuel Sidler (old account; do not CC)]

Mano, can you review this?

Comment 2

[Nickolay_Ponomarev]

This is a trivial change, seems that it still applies. Marco, are you interested in helping to get it in?

Comment 3

[Dietrich Ayala (:dietrich)]

hm, some of the bookmark observer methods seem to assume id > 0, might need updating.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v2

From what i can see, the panel has been created for a one-shot use, so it has to be inited for a certain item, and be uninited after editing has finished. So this to me appear more like an issue in the organizer rather than in editBookmarkOverlay. I think setting and unsetting the bookmarks observer always (for bookmarks and history items) is not correct, moreover in uninit we make other cleanups that could be lost and cause leaks (microsummaries anyone?).

Finally, imo, the correct fix is to uninit the panel in _fillDetailsPane (places.js) before initing it again. This should solve the fact we could start the panel with a certain itemId and uninit it with another one.

What do you think about this?

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 320425 [details] [diff] [review]
patch

mh, for the above reasoning i think this is not the way to go, but feel free to provide further reasons if i'm wrong.

Comment 6

[Dietrich Ayala (:dietrich)]

(In reply to comment #4)
> Created an attachment (id=355760) [details]
> patch v2
> 
> From what i can see, the panel has been created for a one-shot use, so it has
> to be inited for a certain item, and be uninited after editing has finished. So
> this to me appear more like an issue in the organizer rather than in
> editBookmarkOverlay. I think setting and unsetting the bookmarks observer
> always (for bookmarks and history items) is not correct, moreover in uninit we
> make other cleanups that could be lost and cause leaks (microsummaries
> anyone?).

the code that Nickolay pointed out seems fragile as-is, should be more robust wrt to misuse by callers. should be able to resolve that w/o adding the bookmark observer for history items.

agreed on the bit about other cleanup in uninit() that may be causing problems here. but see below.

> Finally, imo, the correct fix is to uninit the panel in _fillDetailsPane
> (places.js) before initing it again. This should solve the fact we could start
> the panel with a certain itemId and uninit it with another one.
> 
> What do you think about this?

i'm a little worried that uniniting the panel while scrolling through many history/bookmark items will be jittery experience. can you test patch v2 for this?

Comment 7

[Nickolay_Ponomarev]

Thanks for looking into this! (Unassigning from myself.)

Comment 8

[Marco Bonardo [:mak]]

(In reply to comment #6)
> the code that Nickolay pointed out seems fragile as-is, should be more robust
> wrt to misuse by callers. should be able to resolve that w/o adding the
> bookmark observer for history items.

We could make editBookmarkOverlay itself being able to detect if someone is trying to call init multiple times without calling uninit, in such a case it could call uninit.  We could set a _initialized private property and check that. Do you think that would be better than relying on the third party implementer to correctly call uninit before init (if needed)?
I suggest doing both, make the panel more robust but also fix the caller.

> i'm a little worried that uniniting the panel while scrolling through many
> history/bookmark items will be jittery experience. can you test patch v2 for
> this?

uninitpanel, if you don't force hiding elements, only removes observers and sets internal properties to null, so on the frontend nothing should happen unless it is inited again.

Comment 9

[Dietrich Ayala (:dietrich)]

(In reply to comment #8)
> We could make editBookmarkOverlay itself being able to detect if someone is
> trying to call init multiple times without calling uninit, in such a case it
> could call uninit.  We could set a _initialized private property and check
> that. Do you think that would be better than relying on the third party
> implementer to correctly call uninit before init (if needed)?
> I suggest doing both, make the panel more robust but also fix the caller.

sounds good.

> uninitpanel, if you don't force hiding elements, only removes observers and
> sets internal properties to null, so on the frontend nothing should happen
> unless it is inited again.

ok.

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v2.1

Nickolay, thanks for first approach!

this fixes both caller and panel

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 355850 [details] [diff] [review]
patch v2.1

r=me thanks!

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/5b18412c4c4d

Comment 13

[Marco Bonardo [:mak]]

i pushed an additional changeset to fix a typo that was causing oranges (the panel could not initialize)
http://hg.mozilla.org/mozilla-central/rev/8c4f6932ab41

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 355850 [details] [diff] [review]
patch v2.1

asking approval, risk is medium but this removes a lying around observer.

Comment 15

[Marco Bonardo [:mak]]

potential dupes of this:
bug 431568
bug 423213
bug 423185

per IRC: Tomcat will take a look at them to see if those are still reproducible.

Comment 16

[Carsten Book [:Tomcat]]

(In reply to comment #15)
> potential dupes of this:
> bug 431568
> bug 423213
> bug 423185
> 
> per IRC: Tomcat will take a look at them to see if those are still
> reproducible.

all the bugs above are now fixed with this patch (and not leaking anymore) with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090114 Shiretoko/3.1b3pre - great job guys ! - also verified fixed

Comment 17

[Marco Bonardo [:mak]]

we need to write a browser-chrome test for this leak

Comment 18

[Marco Bonardo [:mak]]

Attachment: leak browser-chrome test

Comment 19

[Marco Bonardo [:mak]]

Attachment: leak browser-chrome test (unbitrot)

Comment 20

[Marco Bonardo [:mak]]

Attachment: leak browser-chrome test (unregister window watcher)

as done in other tests, we really should stop observing windows at a certain time.

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 359790 [details] [diff] [review]
leak browser-chrome test (unregister window watcher)

r=sdwilsh

Comment 22

[Marco Bonardo [:mak]]

pushed test on m-c:
http://hg.mozilla.org/mozilla-central/rev/e72773cfd493

Comment 23

[Marco Bonardo [:mak]]

asking wanted for the leak and the fact we never uninit panel on branch Library actually.

Comment 24

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

Comment on attachment 355850 [details] [diff] [review]
patch v2.1

a191=beltzner

Comment 25

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/07a3c2f8a1f4 (contains the typo fix)
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/73d3a5b8e2e3

Comment 26

[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