490068 - leak window after showing bookmarks panel

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We leak a JS object, and the window object that contains it, after showing the bookmarks panel.

Steps to reproduce:
 * install leak-monitor from https://addons.mozilla.org/en-US/firefox/addon/2490
 * go to a page
 * press Ctrl+D to bookmark (key probably varies by platform); this still works if the page is already bookmarked
 * click the "Done" button

Expected results: no leak alerts

Actual results:  leak alerting me to the fact that the anonymous transaction object created on lines 220-229 of:
http://hg.mozilla.org/mozilla-central/annotate/e44b672c7e4d/browser/base/content/browser-places.js
has been leaked (which also means the window containing it has been leaked).

Note that the same code is duplicated in browser/components/places/content/bookmarkProperties.js, lines 451-460.


(Note that my build has patches to cycle collect the transaction manager, so I'd have thought that might have helped... though maybe this particular transaction manager isn't itself reachable through edges known to the cycle collector.)

Comment 1

[Asaf Romano (gone)]

Taking.

Comment 2

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

Mano: any progress update?

Comment 3

[Asaf Romano (gone)]

Attachment: patch

Comment 4

[Dietrich Ayala (:dietrich)]

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

This seems fine, r=me. Since this affects all batched Places transactions, please verify that this code path is already tested, or add a new test, before landing.

Comment 5

[Robert Sayre]

Why does it leak at window scope, though?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If the transaction manager lives longer than the window (does it?), then it would keep objects in the window alive past when the window is closed, and thus keep the window alive.  (This would also risk triggering the XPConnect assertions about calling through XPConnect objects on a window without a Components object.)

Comment 7

[Asaf Romano (gone)]

The transaction manager is a service, and thus lives longer than the window. I cannot see which object is created in the window scope here.

Comment 8

[Asaf Romano (gone)]

(In reply to comment #5)
> Why does it leak at window scope, though?

See bug 376004 (or better, bug 168411).

Comment 9

[Asaf Romano (gone)]

_Ah_ I read dbaron's comment wrong and thus didn't realize it's answering robert's comments.

Comment 10

[Asaf Romano (gone)]

Attachment: with test

Comment 11

[Asaf Romano (gone)]

http://hg.mozilla.org/mozilla-central/rev/18accb6d014c
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/deb60868e6b1

Comment 12

[Markus Stange [:mstange]]

(In reply to comment #11)
> http://hg.mozilla.org/mozilla-central/rev/18accb6d014c

A change to build/wince/tools/Makefile slipped in there again.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #12)
> A change to build/wince/tools/Makefile slipped in there again.

The change of build/wince/tools/Makefile apparently breaks WinCE build. Please back out it.

Comment 14

[Asaf Romano (gone)]

Fixed.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marking verified fixed on trunk and 1.9.1 based on none failed tests or backouts.

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