Closed Bug 433231 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

x86
Windows Vista
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: asqueella, Assigned: mak)

References

Details

(Keywords: memory-leak, verified1.9.1)

Attachments

(2 files, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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+.
Attachment #320425 - Flags: review?(sayrer)
Flags: wanted1.9.0.x?
Blocks: 431568
Attachment #320425 - Flags: review?(sayrer) → review?(mano)
Mano, can you review this?
Assignee: nobody → asqueella
Flags: wanted1.9.0.x? → wanted1.9.0.x+
This is a trivial change, seems that it still applies. Marco, are you interested in helping to get it in?
Attachment #320425 - Flags: review?(mano) → review?(mak77)
hm, some of the bookmark observer methods seem to assume id > 0, might need updating.
Attached patch patch v2 (obsolete) — Splinter Review
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?
Attachment #320425 - Flags: review?(mak77) → review-
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.
Attachment #355760 - Flags: review?(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?
Thanks for looking into this! (Unassigning from myself.)
Assignee: asqueella → mak77
(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.
(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.
Attached patch patch v2.1Splinter Review
Nickolay, thanks for first approach!

this fixes both caller and panel
Attachment #320425 - Attachment is obsolete: true
Attachment #355760 - Attachment is obsolete: true
Attachment #355850 - Flags: review?(dietrich)
Attachment #355760 - Flags: review?(dietrich)
Attachment #355850 - Flags: review?(dietrich) → review+
Comment on attachment 355850 [details] [diff] [review]
patch v2.1

r=me thanks!
Target Milestone: --- → Firefox 3.1
Status: NEW → ASSIGNED
Summary: [fix] Places library/organizer leaks the nsGlobalWindow when closed with a history entry selected → Places Library leaks the nsGlobalWindow when closed with a history entry selected
http://hg.mozilla.org/mozilla-central/rev/5b18412c4c4d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.2a1
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 on attachment 355850 [details] [diff] [review]
patch v2.1

asking approval, risk is medium but this removes a lying around observer.
Attachment #355850 - Flags: approval1.9.1?
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.
(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
Status: RESOLVED → VERIFIED
we need to write a browser-chrome test for this leak
Flags: in-testsuite?
Attached patch leak browser-chrome test (obsolete) — Splinter Review
Attachment #359673 - Flags: review?(sdwilsh)
Attachment #359673 - Attachment is obsolete: true
Attachment #359739 - Flags: review?(sdwilsh)
Attachment #359673 - Flags: review?(sdwilsh)
as done in other tests, we really should stop observing windows at a certain time.
Attachment #359739 - Attachment is obsolete: true
Attachment #359790 - Flags: review?(sdwilsh)
Attachment #359739 - Flags: review?(sdwilsh)
Comment on attachment 359790 [details] [diff] [review]
leak browser-chrome test (unregister window watcher)

r=sdwilsh
Attachment #359790 - Flags: review?(sdwilsh) → review+
Flags: in-testsuite? → in-testsuite+
asking wanted for the leak and the fact we never uninit panel on branch Library actually.
Flags: wanted-firefox3.1?
Comment on attachment 355850 [details] [diff] [review]
patch v2.1

a191=beltzner
Attachment #355850 - Flags: approval1.9.1? → approval1.9.1+
Flags: wanted-firefox3.5?
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.