The default bug view has changed. See this FAQ.

Compose window shows reference count leaks on shutdown

RESOLVED FIXED in Thunderbird 22.0

Status

Thunderbird
Message Compose Window
--
minor
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: standard8, Assigned: aceman)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {mlk, regression})

13 Branch
Thunderbird 22.0
mlk, regression
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird21+ fixed, thunderbird-esr1721+ fixed)

Details

(Whiteboard: [gs], URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
+++ 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

5 years ago
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

5 years ago
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

5 years ago
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

5 years ago
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.
Assignee: nobody → dbienvenu

Comment 5

5 years ago
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 ?

Comment 7

5 years ago
(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

Comment 8

4 years ago
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.
(Assignee)

Comment 9

4 years ago
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.
(Assignee)

Comment 11

4 years ago
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
(Assignee)

Comment 12

4 years ago
Created attachment 714100 [details]
leak log without the patch [10 opens/closes of the compose window]
(Assignee)

Updated

4 years ago
Attachment #714100 - Attachment description: leak log without the patch → leak log without the patch [10 opens/closes of the compose window]
(Assignee)

Comment 13

4 years ago
Created attachment 714103 [details]
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.
(Assignee)

Comment 14

4 years ago
Created attachment 714131 [details] [diff] [review]
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.
Attachment #714131 - Flags: feedback?(neil)
(Assignee)

Comment 15

4 years ago
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.
(Assignee)

Comment 17

4 years ago
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.
reporter in getsatisfaction http://getsatisfaction.com/mozilla_messaging/topics/17_0_2_memory_problem_with_lightning_add_on
Whiteboard: [gs]
(Assignee)

Comment 20

4 years ago
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)
(Assignee)

Updated

4 years ago
Attachment #714131 - Flags: review?(mkmelin+mozilla)

Comment 22

4 years ago
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+
(Assignee)

Comment 23

4 years ago
(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)

Comment 24

4 years ago
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
(Assignee)

Comment 26

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

Comment 27

4 years ago
Aryx, do you have needed permissions to produce a try build?
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.
I forgot to post a details on Friday of using http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/archaeopteryx@coole-files.de-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
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...
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
(Assignee)

Comment 33

4 years ago
(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?
Depends on: 820427

Comment 34

4 years ago
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.
(Assignee)

Comment 35

4 years ago
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

4 years ago
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).
(Assignee)

Comment 37

4 years ago
Created attachment 720448 [details] [diff] [review]
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.
Attachment #635481 - Attachment is obsolete: true
Attachment #714131 - Attachment is obsolete: true
Attachment #720448 - Flags: review+
(Assignee)

Comment 38

4 years ago
Petr provided testing of the patch in bug 818607.
Flags: needinfo?(petr.v)
Keywords: checkin-needed
No longer blocks: 826494
https://hg.mozilla.org/comm-central/rev/307e36310836
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0

Updated

4 years ago
Blocks: 360488
(Assignee)

Comment 40

4 years ago
Standard8, if this patch proves working and not causing problems in nightly, is this something able to get into 17?
tracking-thunderbird-esr17: --- → ?
(Reporter)

Comment 41

4 years ago
Yes, but I'd rather give it a bit of time for baking first.
tracking-thunderbird21: --- → +
tracking-thunderbird-esr17: ? → 21+
(Reporter)

Comment 42

4 years ago
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+
(Reporter)

Comment 43

4 years ago
https://hg.mozilla.org/releases/comm-beta/rev/663389de0c65
status-thunderbird21: --- → fixed

Comment 44

4 years ago
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.
      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.
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.
(Reporter)

Comment 47

4 years ago
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+
(Reporter)

Comment 48

4 years ago
https://hg.mozilla.org/releases/comm-esr17/rev/e3de65ce0cb2
status-thunderbird-esr17: --- → fixed
You need to log in before you can comment on or make changes to this bug.