765074 - Compose window shows reference count leaks on shutdown

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Mark Banner (:standard8)]

+++ 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.

Comment 1

[David :Bienvenu]

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...

Comment 2

[David :Bienvenu]

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.

Comment 3

[David :Bienvenu]

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.

Comment 4

[David :Bienvenu]

Attachment: 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.

Comment 5

[David :Bienvenu]

yeah, this patch had no effect on the bloat log.

Comment 6

[Ludovic Hirlimann [:Usul]]

(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 ?

Comment 7

[David :Bienvenu]

(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.

Comment 8

[Petr Vones]

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.

Comment 9

[:aceman]

Is David working on this?
Can anybody explain where to see the 'bloat log' ?

Comment 10

[Florian Quèze [:florian]]

(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.

Comment 11

[:aceman]

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.

Comment 12

[:aceman]

Attachment: leak log without the patch [10 opens/closes of the compose window]

Comment 13

[:aceman]

Attachment: leak log with the patch [10 opens of the compose window]

The numbers (at least the total) in this log are stable regardless on number of compose window opens.

Comment 14

[:aceman]

Attachment: patch v2

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.

Comment 15

[:aceman]

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.

Comment 16

[Wayne Mery (:wsmwk)]

(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.

Comment 17

[:aceman]

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).

Comment 18

[Wayne Mery (:wsmwk)]

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.

Comment 19

[Wayne Mery (:wsmwk)]

reporter in getsatisfaction http://getsatisfaction.com/mozilla_messaging/topics/17_0_2_memory_problem_with_lightning_add_on

Comment 20

[:aceman]

Wayne, can you run with the patch?

Comment 21

[[e-mail]]

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.

Comment 22

[Magnus Melin [:mkmelin]]

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

Comment 23

[:aceman]

(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?

Comment 24

[Magnus Melin [:mkmelin]]

Look at what functions are there and see if everything still seems happy (when you do the things it does through the UI).

Comment 25

[Wayne Mery (:wsmwk)]

(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

Comment 26

[:aceman]

(In reply to Wayne Mery (:wsmwk) from comment #25)
> Or from a try build
Can you produce one?

Comment 27

[:aceman]

Aryx, do you have needed permissions to produce a try build?

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed a build-only, no tests run to Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=a63ca6826d5a
Should be finished in 3+ hours.

Comment 29

[Wayne Mery (:wsmwk)]

I forgot to post a details on Friday of using http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-a63ca6826d5a/

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

Comment 30

[WADA:World Anti-bad-Duping Agency]

bug 818607, bug 826494, bug 826494 are report of "memory leak" around composition window ope/close. Setting dependency for ease of tracking anf analysis.

Comment 31

[WADA:World Anti-bad-Duping Agency]

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"?

Comment 32

[Wayne Mery (:wsmwk)]

on my vista laptop...
reuse_message_window=false
compose.max_recycled_windows=1
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

Comment 33

[:aceman]

(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?

Comment 34

[Magnus Melin [:mkmelin]]

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.

Comment 35

[:aceman]

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?

Comment 36

[Magnus Melin [:mkmelin]]

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).

Comment 37

[:aceman]

Attachment: patch v3

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.

Comment 38

[:aceman]

Petr provided testing of the patch in bug 818607.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/307e36310836

Comment 40

[:aceman]

Standard8, if this patch proves working and not causing problems in nightly, is this something able to get into 17?

Comment 41

[Mark Banner (:standard8)]

Yes, but I'd rather give it a bit of time for baking first.

Comment 42

[Mark Banner (:standard8)]

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.

Comment 43

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-beta/rev/663389de0c65

Comment 44

[611]

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.

Comment 45

[WADA:World Anti-bad-Duping Agency]

> 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.
      mail.compose.max_recycled_windows=0
(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.
 
 
,
  .23.

Comment 46

[WADA:World Anti-bad-Duping Agency]

FYI.
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 47

[Mark Banner (:standard8)]

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.

Comment 48

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr17/rev/e3de65ce0cb2