Compose window shows reference count leaks on shutdown

RESOLVED FIXED in Thunderbird 22.0


7 years ago
6 years ago


(Reporter: standard8, Assigned: aceman)


(Depends on 1 bug, Blocks 1 bug, {memory-leak, regression})

13 Branch
Thunderbird 22.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird21+ fixed, thunderbird-esr1721+ fixed)


(Whiteboard: [gs], )


(3 attachments, 2 obsolete attachments)

+++ This bug was initially created as a clone of Bug #764742 +++

Similar to bug 764742 we're seeing memory leaks when shutting after having displayed the compose window - typically nsDocShell and some nsGlobalWindows which is generally bad.

As we recycle the compose window the end-user effects of this are probably not too big, but we still should resolve it to find out what's happening there.
I've got some fixes for this, but it's pretty stubborn. I can fix the leak if you close the window without sending, but if you send, we still leak...
I think the "cause" of the leak is the fact that the backend closes the window, instead of the user. For some reason, that path through the code doesn't clean things up the same way. If I send later, the same leak happens as when I send for real, so it's not the smtp protocol code or anything like that. Very little code knows about the msgWindow, but it's still leaking, so I'm a bit perplexed.
ah, it's all clear now, as it so often is after a night of sleep. MsgComposeCloseWindow is *NOT* called when the user sends a message (as opposed to clicking on the x to close the window, when it is called), which means we don't remove the msg window from the mail session, which makes it leak. Given that we cache the compose window by default, this isn't a big deal, except that it shows up as a leak.

My plan is to move the msgWindow and a couple other things into the GlobalVariables sections of the code, so that they're initialized in InitializeGlobalVariables and released in ReleaseGlobalVariables.
Posted patch fix some leaks (obsolete) — Splinter Review
this fixes some leaks in some situations, but I don't think it'll fix what the bloat log is showing. I'll try it, though.
Assignee: nobody → dbienvenu
yeah, this patch had no effect on the bloat log.
(In reply to David :Bienvenu from comment #4)
> Created attachment 635481 [details] [diff] [review]
> fix some leaks
> this fixes some leaks in some situations, but I don't think it'll fix what
> the bloat log is showing. I'll try it, though.

so not worth pushing or being reviewed ?
(In reply to Ludovic Hirlimann [:Usul] from comment #6)
> so not worth pushing or being reviewed ?

I think it's worth pursuing, but it's much more important to fix the bloat log.
Severity: normal → minor
It seems as the issue has become more noticeable and irritating in TB 17.0. I'm not sure whether it is the same bug so there is another bug 818607 created.
Is David working on this?
Can anybody explain where to see the 'bloat log' ?
(In reply to :aceman from comment #9)

> Can anybody explain where to see the 'bloat log' ?

Use a debug build, and run it with the XPCOM_MEM_LEAK_LOG environment variable set to 1 (to print the log to stdout), 2 (stderr), or a filename.
I don't think I can fix what David couldn't but at least we could drive the partial patch into the tree. He says it fixes some (other) leaks.
I'll refresh the patch and then try what Florian says.
Assignee: mozilla → acelists
Attachment #714100 - Attachment description: leak log without the patch → leak log without the patch [10 opens/closes of the compose window]
The numbers (at least the total) in this log are stable regardless on number of compose window opens.
Posted patch patch v2 (obsolete) — Splinter Review
When Opening/Sending a mail from the Compose window I only sometimes got a leak (about 1,3MB), sometimes not. But after any number of opens I could never get the leak above 2,3MB. So the patch definitely helps a lot.
I also tried setting the pref about recycling compose windows to 0, but I think it made no difference.

Seamonkey also has similar constructs in the compose window, that this patch is changing. Maybe they have the same leak?

So here is the unbitrotted patch to try out.
Attachment #714131 - Flags: feedback?(neil)
Petr Vones, are you able to try out the patch? If you can't build from source, and want to stay on stable TB17.0.x, you can still open the omni.ja archive (copy/backup it before that), find the file MsgComposeCommands.js and do the changes the patch is describing (lines preceded by '-' sign should be removed, lines with '+' should be added). It is just several lines. Then delete the file startupCache4.little in your TB profile. Then start TB and see if you get the leaks. If anything goes wrong, restore omni.ja from the good copy.
Flags: needinfo?(petr.v)
(In reply to :aceman from comment #14)
> When Opening/Sending a mail from the Compose window I only sometimes got a
> leak (about 1,3MB), sometimes not. But after any number of opens I could
> never get the leak above 2,3MB. So the patch definitely helps a lot.

Nice work aceman. I also reproduce ~2MB per window over multiple tests just opening and closing - no typing of text.  2MB will be a fantastic win for users - I can imagine reductions of 200MB-300MB per day. I look forward to testing with the patch.
Just to make sure, my 0-1,3-2,3MB figure with the patch is the total leak after any number of opens I have tried (not per compose window open).
I should have stated my 2MB increase per compose windows is measured by leak log but as Working Set in taskmgr on vista. Same for plain text and for html, new profile.
Wayne, can you run with the patch?
Comment on attachment 714131 [details] [diff] [review]
patch v2

No, we don't have the same constructs.

Except on the Mac, Thunderbird tries to "shutdown" the account manager when the main three pane window is closed. To avoid this, bug 530779 pretends that the compose window is a three pane window, but a circular reference was created resulting in a leak, which this patch appears to fix.

The gMessenger changes appear to be unnecessary.
Attachment #714131 - Flags: feedback?(neil)
Attachment #714131 - Flags: review?(mkmelin+mozilla)
Comment on attachment 714131 [details] [diff] [review]
patch v2

Review of attachment 714131 [details] [diff] [review]:

Have you made sure this works ok with mail-offline.js? (That the msgWindow is shared sure is an ugly bug!)
Looks reasonable to me. r=mkmelin

::: mail/components/compose/content/MsgComposeCommands.js
@@ +41,5 @@
> + * should not be renamed. We need to avoid doing this kind of cross file global
> + * stuff in the future and instead pass this object as parameter when needed by
> + * functions in the other js file.
> + */
> +let msgWindow;

i'd prefer to keep these kind of "top-level" variables 'var'

@@ +148,5 @@
>  InitializeGlobalVariables();
>  function ReleaseGlobalVariables()
>  {
> +  dump("releasing global vars\n"); // ACE: remove after debugging

to remove

@@ +2523,5 @@
>  }
>  function ComposeUnload()
>  {
> +  dump("in composeunload\n"); // ACE: remove after debugging

to remove
Attachment #714131 - Flags: review?(mkmelin+mozilla) → review+
(In reply to Magnus Melin from comment #22)
> Have you made sure this works ok with mail-offline.js? (That the msgWindow
> is shared sure is an ugly bug!)
No, how do I do this?
Flags: needinfo?(mkmelin+mozilla)
Look at what functions are there and see if everything still seems happy (when you do the things it does through the UI).
Flags: needinfo?(mkmelin+mozilla)
(In reply to :aceman from comment #20)
> Wayne, can you run with the patch?

Happy to test this after it lands.  Or from a try build
(In reply to Wayne Mery (:wsmwk) from comment #25)
> Or from a try build
Can you produce one?
Aryx, do you have needed permissions to produce a try build?
I forgot to post a details on Friday of using

My preliminary results at work were very good. Unfortunately I do not have my notes of that handy.

On my 32bit vista laptop however, no significant change.  a loss of ~2MB per compose window open/close cycle by doing the following:
1. set thunderbird offline
2. check memory amount
3. open 10 compose windows
4. check memory amount
5 close 10 compose windows
6. check memory amount
bug 818607, bug 826494, bug 826494 are report of "memory leak" around composition window ope/close. Setting dependency for ease of tracking anf analysis.
Blocks: 818607, 826494
According to relevant bugs, "reference count" may depend on "how composition window is closed"(close by Send, close by X button), and as I wrote in bug 826494 comment #1, Bug 814651 may be relevant to proble(even tough mailnews.reuse_message_window==true && mail.compose.max_recycled_windows>0, document under DOM Window for composition window looks unloaded, and the DOM window may be closed instead of cached, or may be cached and hidden but not re-used).

Wayne, do you see memory increase or reference cout leak with mail.compose.max_recycled_windows==0?
(re-use of cached composition window is disabled, so composition window is not cached)
Do you see difference between "compose/send or send later" and "compose/close without send"?
on my vista laptop...
mailnews.sendInBackground=true  (and also at work)
ctrl+w is being used to close compose windows in all my testing thus far.

I have not been checking reference count leaks.

closing compose with mail.compose.max_recycled_windows==0 no significant change in memory usage.

sending individual messages (not closing window) with mail.compose.max_recycled_windows==0 still loses 2-3 MB per send
(In reply to Magnus Melin from comment #22)
> Have you made sure this works ok with mail-offline.js? (That the msgWindow
> is shared sure is an ugly bug!)

Sorry, I have no idea how the two files relate. Can you finish it?
We don't need to fix that bug here. If going online/offline syncs as expected, and outbox msgs are sent, i'm happy with it.
OK, the test is if unsent messages are sent when offline is toggled to offline? That is with compose window closed, but some code of msgComposeCommands.js are used to do it?
I think, after closing the compose window, going offline/online should ask to sync, and if there are msgs in th outbox after coming online it should send (and ask depending on prefs).
Posted patch patch v3Splinter Review
Yes, after composing a message and clicking Send later, then toggling offline to online, TB asks for sending of messages. After confirmation, the message is sent, moved to Sent.
Attachment #635481 - Attachment is obsolete: true
Attachment #714131 - Attachment is obsolete: true
Attachment #720448 - Flags: review+
Petr provided testing of the patch in bug 818607.
Flags: needinfo?(petr.v)
Keywords: checkin-needed
No longer blocks: 826494
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0
Blocks: TB2SM
Standard8, if this patch proves working and not causing problems in nightly, is this something able to get into 17?
Yes, but I'd rather give it a bit of time for baking first.
Comment on attachment 720448 [details] [diff] [review]
patch v3

[Triage Comment]
Ok, given this is now on Aurora with no reported issues, taking onto beta as well.
Attachment #720448 - Flags: approval-comm-beta+
This is reported as fixed in TB 21's changelog, but I still get the memory leak (and slowdown over time).

How to reproduce: Simply open and close the compose window repeatedly and check TB's memory usage.
> Bug summary : Compose window shows reference count leaks on shutdown

As for "actually cached compose window", I think it's normal phenomenon, as far as composed window is actually cached corectly, because cached compose window is hidden from Window Enumerator for both DOM window Enumerator and XUL widow Enumaroror. i.e. No one can know about existence of "cached(==hidden from XUL window Enumerator) compose window" except manager of "cached compose window".

Problem of this bug is perhaps exposed ater Tb 12 when;
(a) "cached compose window" is not used.
(b) cached compose window is requested by
      mail.compose.max_recycled_windows>0 (default=5)
    but compose window is not cached, instead is closed always,
    after Tb trunk 2012-07-31 build(Tb 17.0a0),
    due to patch for bug 777063 which is landed on 2012-07-31.
And, if this bug occurs, severe memory leak reported to bug 818607 comment #0 is externally observed while Tb is runnung.
See bug 818607, please.

To all problem reportes in this bug:
Did you check with which Tb buiuld with what mail.compose.max_recycled_windows setting?

As seen in test results with patch of this bug in bug 818607, patch of this bug perhaps correctly resolved problem of "non-destoryed object after compose window close" and the patch surely resolved memory leak due to non-destoryed object.
However, virtual memory increase after repeated compose window open/close is still observed, although increment is far smaller than before patch of this bug is landed. 
If you'll do test for this bug(and bug 818607), please check with mail.compose.max_recycled_windows=0 always in order to avoid our confusion by ourself and misleading others, and after patch for "regression by patch of bug 777063" will be landed, check with mail.compose.max_recycled_windows>0, please.
For "regression by patch of bug 777063", symptom of "compose window is never cached after Tb 17", see bug 814651 comment #28 and bug 814651 comment #29, please.
Comment on attachment 720448 [details] [diff] [review]
patch v3

[Triage Comment]
Taking as this has been in beta for a cycle, and is believed to at least reduce the leak if not more.
Attachment #720448 - Flags: approval-comm-esr17+
You need to log in before you can comment on or make changes to this bug.