388353 - Displaying a Mail Messages leaks a jsContext & a global window

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Scott MacGregor]

make sure you are running with the fix in Bug 381992.

Start up thunderbird, view a message in the message pane and quit.

We leak a bunch of stuff including a jsContext, nsGlobalWindowImpl and nsMsgWindow.

Comment 1

[Scott MacGregor]

Attachment: leak log

here's a leak log showing all the objects that get leaked.

Comment 2

[Scott MacGregor]

Attachment: [checked in]use weak references for the msg window and the status feedback object

Most of the leaks are happening because the mailnews url for displaying the message gets leaked. And it holds strong refs to both the msg window and a JS status feedback object.

While we still need to fix the url leaks, there's no reason for the mailnews url to hold strong references to these two objects, so let's make them weak references.

Comment 3

[David :Bienvenu]

Comment on attachment 272566 [details] [diff] [review]
[checked in]use weak references for the msg window and the status feedback object

cool

Comment 4

[Scott MacGregor]

It looks like the real problem is that we have a cycle between an imap url and an imap mock channel. This cycle is explicitly broken in nsImapIncomingServer::RemoveChannelFromUrl.

However, it looks like we end up cloning our imap url gets cloned (no less than 32 times!!) by gecko for various security checks. This causes us to create 32 imap urls & mock channels (see http://mxr.mozilla.org/mozilla/source/mailnews/imap/src/nsImapUrl.cpp#108) that have circular references. And since known of these url objects are actually run by the imap back end, we never get a chance to break the cycle.

This should be fun to fix. The url to mock channel relationship has always been very fragile.

Comment 5

[Scott MacGregor]

Attachment: WIP - re-write the ownership model between the channel and the imap url

saving off a work in progress. This patch fixes the leaks for me, I need to do some more testing on it.

Comment 6

[Scott MacGregor]

Attachment: [checked in]re-write the ownership model between the imap mock channel and the url

It's never too late to undo your mistakes...this patch re-writes the ownership model between an imap mock channel and an imap url, removing many band aids and tricks I've used in the past....

* The channel now always owns the url. This is consistent with the ownership model of other channel implementations.

* an imap url has a weak reference to the channel. This is used solely to pass the mock channel into the imap protocol instance when we actually run the url since urls get queued.

* I've removed all the other channel mumbo jumbo that was living in nsImapUrl. Most importantly, the channel is no longer created when the imap url is created. So urls don't have a channel associated with them by default anymore.

* When the io service asks the imap service to create an imap channel (nsImapService::NewChannel) for a given imap url, we  now create a new mock channel instance and initialize it with the imap url.

* Some urls (like folder selection) aren't run through the io service (via nsIChannel::AsyncOpen which creates a channel for the url). When they get to nsImapProtocol::SetupWithUrl, we now create a channel (nsImapProtocol gets unhappy if we run a url without a channel). For the newly created channel, we add it to the load group of the message pane so the url can be interrupted by pressing the stop button. It's bogus that we use the load group of the message pane, but that's what we did before and I don't know of a better way yet. For the case where we already have

* Remove calls to imapUrl->AddChannelToLoadGroup() for various imap operations that don't run through the IO service like folder selection since we don't have a channel associated with the operation yet. The new code in nsImapProtocol::SetupWithUrl now adds the channel to the load group.

I'm going to keep testing this patch a bit more before I ask for a review. I'm worried that the loadgroup changes might regress something.

Comment 7

[Scott MacGregor]

Comment on attachment 273184 [details] [diff] [review]
[checked in]re-write the ownership model between the imap mock channel and the url

See my previous comments for a break down of what this patch is doing.

This patch has a high degree of risk (mostly with urls & load group management) but I need the nightly testers to start using to help figure out any problems I'm introducing.

Comment 8

[David :Bienvenu]

Comment on attachment 273184 [details] [diff] [review]
[checked in]re-write the ownership model between the imap mock channel and the url

I'll run with this as well. My other areas of concern w/ this patch are:

Does queuing and playing back of urls still work?

Does retrying urls work? This often comes up when the server or network drops a connection, and we need to retry the url, with a new connection.

Comment 9

[David :Bienvenu]

The most noticeable use of retrying urls is for an fcc to an imap sent folder, when the connection to the sent folder has gone away, because when the retry doesn't happen, we just sit and spin trying to copy to the sent folder.

nsImapProtocol::RetryUrl is the function that gets hit when we do a retry.

Comment 10

[David :Bienvenu]

I think retrying urls is working, at least in some cases. I haven't seen it happen/work yet for fcc, though.

Comment 11

[lee]

about the time that the last patch was checked in i noticed that my mail filters stop working.  i haven't investigated it fully, but the filters work at thunderbird startup, and some time thereafter they stop filtering and i get a window popup notice that there are "0 new messages" when i check for mail if there are unfiltered messages that qualify for filtering on the imap server.  the only thing that led me to this bug is that std error says "no listeners in set url state" which led me to this patched file and bug.  i find it more than coincidence that the filters stopped working about the time this file was patched and when i noticed this printing in the console.  i'm running tb built from recent cvs on updated fedora 6.  i build just about daily hoping the bug will go away without my intervention, but it doesn't look like it will  <sigh>

Comment 12

[Boris Zbarsky [:bzbarsky]]

This caused crash bug 391259.

Also, the checkin comment for this patch mentioned the wrong bug number.  Please file a bug to cvs admin it to be correct?

Comment 13

[Wayne Mery (:wsmwk)]

Lee, do you still see the filter problem?

Comment 14

[lee]

no i haven't, apparently it has gone into remission, or my suspicions were in error... thanks  :)

Comment 15

[David :Bienvenu]

I'm not sure why this isn't marked fixed...I believe it is fixed, though I haven't verified that it hasn't regressed recently.

Comment 16

[Ludovic Hirlimann [:Usul]]

(In reply to comment #15)
> I'm not sure why this isn't marked fixed...I believe it is fixed, though I
> haven't verified that it hasn't regressed recently.

Resolving Fixed based on your comment and comment #14