339784 - In <mailWindow.js>, |nsMsgStatusFeedback.prototype| leaks when Seach Messages window is closed, as reported by Leak Monitor extension

Status: ASSIGNED

(MailNews Core :: Search, defect)

Description

[Adam Hauner]

2006052401/1.8-branch/SeaMonkey1.1a with Leak Monitor 2.0 (SM port)

Repro:
1. select any folder with messages
2. open Search Messages window (Ctrl+Shift+S)
3. enter for example "s" into search dialog
4. hit search
5. close Search Messages window

Actual:
[+] [leaked object] (8e96130) = [object Object]
 [ ] statusTextFld (8d8c258) = [object XULElement]
 [ ] statusBar (8f438d8) = [object XULElement]
 [ ] statusPanel (8d8c250) = [object XULElement]
 [ ] throbber = null
 [ ] stopCmd = null
 [ ] meteorsSpinning = false
 [ ] startTimeoutID = null
 [ ] myDefaultStatus = 
 [ ] stopTimeoutID = null
[ ] [leaked object] (8f43b08) = [object Object]

Comment 1

[Serge Gautherie (:sgautherie)]

Could you try a current Trunk build (too) ?

Comment 2

[Pavel Cvrcek [:JasnaPaka]]

(In reply to comment #1)
> Could you try a current Trunk build (too) ?

I can confirm this with latest trunk.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060530 SeaMonkey/1.5a

Comment 3

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2) Gecko/20060518 SeaMonkey/1.1a] (nightly) (W98SE)

(Confirmed, on branch.)

This happens at each closing.

Simpler steps:
*At step 1: you can be on a folder, or on an account.
*Step 3 is optional, step 4 is enough (searching for blank, getting no result).

For the record, simply opening and closing the Search window gives me the following report:
[ ] [leaked object] (2603a60) = [object Object]
[ ] [leaked object] (16b5118) = [object Object]

Comment 4

[Serge Gautherie (:sgautherie)]

<http://landfill.bugzilla.org/mxr-test/seamonkey/search?string=nsMsgStatusFeedback&find=.*js>
[[
nsMsgStatusFeedback

/mailnews/base/search/resources/content/SearchDialog.js, line 56 -- var gStatusFeedback = new nsMsgStatusFeedback();
/mailnews/base/resources/content/subscribe.js, line 11 -- var gStatusFeedback = new nsMsgStatusFeedback;
/mailnews/base/resources/content/mailWindow.js, line 146 -- window.MsgStatusFeedback = new nsMsgStatusFeedback();
/mailnews/base/resources/content/mailWindow.js, line 315 -- function nsMsgStatusFeedback()
/mailnews/base/resources/content/mailWindow.js, line 319 -- nsMsgStatusFeedback.prototype =

/mail/base/content/subscribe.js, line 11 -- var gStatusFeedback = new nsMsgStatusFeedback;
/mail/base/content/mailWindow.js, line 151 -- window.MsgStatusFeedback = new nsMsgStatusFeedback();
/mail/base/content/mailWindow.js, line 274 -- function nsMsgStatusFeedback()
/mail/base/content/mailWindow.js, line 278 -- nsMsgStatusFeedback.prototype =
]]

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <SearchDialog.js>

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2) Gecko/20060524 SeaMonkey/1.1a] (nightly) (W98SE)

This fixes the "detailed" object (leak).

Comment 6

[Serge Gautherie (:sgautherie)]

The following test shows where the "undetailed" object leak comes from:
[[
function setupDatasource() {

    RDF = Components.classes[rdfServiceContractID].getService(Components.interfaces.nsIRDFService);
    gSearchView = Components.classes["@mozilla.org/messenger/msgdbview;1?type=search"].createInstance(Components.interfaces.nsIMsgDBView);
    var count = new Object;
    var cmdupdator = new nsMsgSearchCommandUpdater();

//    gSearchView.init(messenger, msgWindow, cmdupdator);
    gSearchView.init(messenger, msgWindow, null);
]]
but I don't know how to release (properly) this |cmdupdator|: helpwanted.
(Call |.init()| again in the |function searchOnUnload()| ??)

Comment 7

[Serge Gautherie (:sgautherie)]

Forgot to say:
the comment 0 and comment 3 leaks are the same;
actually doing a search simply gives details on one of the two objects.

Comment 8

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a3) Gecko/20060603 SeaMonkey/1.1a] (nightly) (W98SE)

(Bug still there, with L.M. v0.3.0.)

Comment 9

[Ian Neal]

Comment on attachment 223999 [details] [diff] [review]
(Av1) <SearchDialog.js>

Sorry, I do not know this part of the code, perhaps bienvenu or neil@parkwaycc ?

Comment 10

[Wayne Mery (:wsmwk)]

Serge, does this still apply?
Neil, are you OK to review?

Comment 11

[[e-mail]]

Does this leak on trunk? Is it only this nsMsgStatusFeedback that leaks?
(Other windows also use nsMsgStatusFeedback.)

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #11)
> Does this leak on trunk? Is it only this nsMsgStatusFeedback that leaks?
> (Other windows also use nsMsgStatusFeedback.)
> 
Having just been looking at some items not too far from this this afternoon, I wouldn't be surprised if this does leak.

I would expect the correct fix would be to call msgWindow.closeWindow (like mailWindow.js does) - which would automatically drop the nsMsgStatusFeedback - but I'm not sure if that is entirely the right fix in this case.

Comment 13

[:aceman]

What needs to be done here? Is this still valid?

Comment 14

[[e-mail]]

Comment on attachment 223999 [details] [diff] [review]
(Av1) <SearchDialog.js>

>     // release this early because msgWindow holds a weak reference
>     msgWindow.rootDocShell = null;
>+
>+    // Unhook instanciated prototype.
>+    msgWindow.statusFeedback = null;
As per comment #12 we should use msgWindow.closeWindow(); instead (which also conveniently clears rootDocShell as well).

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to :aceman from comment #13)
> What needs to be done here? Is this still valid?

Give me some time to get back to this bug...

Comment 16

[Serge Gautherie (:sgautherie)]

Right:

http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgWindow.cpp#134
135 NS_IMETHODIMP nsMsgWindow::CloseWindow()

http://mxr.mozilla.org/comm-central/search?string=rootDocShell+%3D+null&case=1&find=%2Fmail
"Found 2 matching lines in 2 files"

Comment 17

[:aceman]

Ok, I assume you take this to finish it.

Comment 18

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #17)
> Ok, I assume you take this to finish it.

not so far :)

Comment 19

[:aceman]

Attachment: patch v2

Patch incorporating the suggestions.

Comment 20

[Serge Gautherie (:sgautherie)]

Thanks for taking this over, aceman :-)

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 8696244 [details] [diff] [review]
patch v2

Review of attachment 8696244 [details] [diff] [review]:
-----------------------------------------------------------------

f+ : this looks like exactly what was suggested in previous comments.

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 8696244 [details] [diff] [review]
patch v2

Review of attachment 8696244 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable. Did you verify it removes the leak?

Comment 23

[:aceman]

No, I couldn't get the addon to work in TB. While I forced to to install, it can't load its binary component (on Linux). Maybe that one needs recompiling for current releases. The homepage http://dbaron.org/mozilla/leak-monitor/ seems to imply it no longer works.