321271 - Address book sidebar leaks connections

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect)

Description

[Mark Banner (:standard8)]

See bug 321254 for the thunderbird version. Quoted from that bug:

If you open the contacts sidebar in a compose window, and leave it open, and do
an ldap search, we don't close the ldap connection when the compose window
closes. We do close it if you close the sidebar, however. Fix upcoming that
closes the ldap connection, and clears the search input, when the compose
window is closed. This is all because of the compose window caching.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <addressbook-panel.js>

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008042602 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

This patch seems to be the best I can do.
I would file another bug to "port" bug 269304, after this one.

I don't know how to check if it has any (good) effect.
I verified that |AbPanelOnComposerClose()| is called.
I don't know how to check that |AbPanelOnComposerReOpen()| is too.

Comment 2

[[e-mail]]

Comment on attachment 317915 [details] [diff] [review]
(Av1) <addressbook-panel.js>

>+  // |onClearSearch()|: Need to port bug 269304 to SM first.
>+  // onClearSearch();
>+  // Call |onAbClearSearch()| directly in the meantime. (See bug 321271.)
I wouldn't bother with this; should bug 269304 ever be ported then it should be obvious that onAbClearSearch would need to be changed.

>+  parent.document.getElementById("msgcomposeWindow").addEventListener('compose-window-close', AbPanelOnComposerClose, true);
You don't actually need to add/remove the event listeners to that specific element; adding it to the window works fine (see msgCompSMIMEOverlay.js).

sr=me with those fixed.

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 317915 [details] [diff] [review]
(Av1) <addressbook-panel.js>

(In reply to comment #1)
> I verified that |AbPanelOnComposerClose()| is called.
> I don't know how to check that |AbPanelOnComposerReOpen()| is too.

Now, I know: it is called too ;->


(In reply to comment #2)
> (From update of attachment 317915 [details] [diff] [review])
> >+  parent.document.getElementById("msgcomposeWindow").addEventListener('compose-window-close', AbPanelOnComposerClose, true);
> You don't actually need to add/remove the event listeners to that specific
> element; adding it to the window works fine (see msgCompSMIMEOverlay.js).

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008043003 Thunderbird/3.0a1pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008043001 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

<msgCompSMIMEOverlay.js> works fine in SM.

But using |window.*EventListener(...);| for this bug does nothing, in either TB or SM.
|user_pref("mailnews.reuse_message_window", false);| does not seem to make a difference either (in SM).

Comment 4

[[e-mail]]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 317915 [details] [diff] [review] [details])
> > >+  parent.document.getElementById("msgcomposeWindow").addEventListener('compose-window-close', AbPanelOnComposerClose, true);
> > You don't actually need to add/remove the event listeners to that specific
> > element; adding it to the window works fine (see msgCompSMIMEOverlay.js).
> But using |window.*EventListener(...);| for this bug does nothing
Sorry, I meant parent.addEventListener, not window.

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a-SM) <addressbook-panel.js>

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008050302 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Av1, with comment 2 & 4 suggestion(s).

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-TB) <addressbook-panel.js> [Checkin: Comment 22]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008043003 Thunderbird/3.0a1pre] (nightly) (W2Ksp4)

(Back-) Port comment 4 suggestion(s) to TB.

Comment 7

[[e-mail]]

Comment on attachment 319227 [details] [diff] [review]
(Av1a-SM) <addressbook-panel.js>

>+function AbPanelOnComposerClose()
>+{
>+  CloseAbView();
>+  onAbClearSearch();
>+}
>+
>+function AbPanelOnComposerReOpen()
>+{
>+  SetAbView(GetSelectedDirectory(), true);
>+}
This sort of works, but not in the intended way. The problem as I see it is that onAbClearSearch (eventually) calls SetAbView, so that there is always a view loaded when the compose window is hidden. This in itself is not a problem, because it still closes the LDAP search connection, but it seems to me that you have two options, depending on whether you believe the view should be closed while the compose window is hidden:
1. If the view does not need to be closed, then forget about Close/SetAbView since they aren't having any effect, and simply call onAbClearSearch directly.
2. If the view needs to be closed then you can't use onAbClearSearch to clear the search input, and you also lose the ability to track addressbook deletions which means you will then need to call LoadPreviouslySelectedAB instead of SetAbView.

Comment 8

[[e-mail]]

(In reply to comment #7)
> 1. If the view does not need to be closed, then forget about Close/SetAbView
> since they aren't having any effect, and simply call onAbClearSearch directly.
> 2. If the view needs to be closed then you can't use onAbClearSearch to clear
> the search input, and you also lose the ability to track addressbook deletions
> which means you will then need to call LoadPreviouslySelectedAB instead of
> SetAbView.
Actually this doesn't work, so you'll just have to use onAbClearSearch only. Also it might be worth special-casing it so you only add onAbClearSearch as the close listener if you're actually in a compose window (gMsgCompose is true).

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Av2-SM) <addressbook-panel.js>

Av1a-SM, with comment 8 suggestion(s).
And the other space cleanups.

Don't we need to listen to a similar event for the Browser window (sidebar) ?

Comment 10

[[e-mail]]

Comment on attachment 324070 [details] [diff] [review]
(Av2-SM) <addressbook-panel.js>

>-function AbPanelLoad() 
>+function AbPanelOnComposerClose()
> {
>-  InitCommonJS(); 
>+  onAbClearSearch();
>+}
>+
>+function AbPanelLoad()
>+{
>+  InitCommonJS();
Can we avoid extraneous whitespace fixes please, it only confuses blame.
I've also only just noticed that the function is named OnComposerClose but Composer is of course the SeaMonkey's web page editor, so you need to remove the "r" (or possibly call onAbClearSearch directly?)

>+  gMsgCompose = parent.document.documentElement.getAttribute("windowtype") == "msgcompose";
>+  gSearchInput = document.getElementById("searchInput");
>+
>+  if (gMsgCompose)
>+    parent.addEventListener("compose-window-close", AbPanelOnComposerClose, true);
Any reason you moved gMsgCompose up instead of addEventListener down? [gSearchInput didn't need to be moved at all, did it?]

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Av3-SM) <addressbook-panel.js> [Checkin: Comment 24]

Av2-SM, with comment 10 suggestion(s).

(In reply to comment #10)
> Any reason you moved gMsgCompose up instead of addEventListener down?

I thought that order made sense...

***

(why) Don't we need to listen to a similar event for the Browser window (sidebar) ?

Comment 12

[[e-mail]]

Comment on attachment 324110 [details] [diff] [review]
(Av3-SM) <addressbook-panel.js>
[Checkin: Comment 24]

Don't forget that the change to line 107 from bug 433202 needs to land too.

(In reply to comment #9)
> Don't we need to listen to a similar event for the Browser window (sidebar) ?
No, this is only about clearing the connection when the whole window is "closed" (the compose is often recycled rather and doesn't get really closed).

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)
> (From update of attachment 324110 [details] [diff] [review])
> Don't forget that the change to line 107 from bug 433202 needs to land too.

Reversing the dependency, as, from recent bug 433202 comments, its SeaMonkey fix depends on this bug patch.
(Or did you imply that the two patches should land at the same time ?)

> (In reply to comment #9)
> > Don't we need to listen to a similar event for the Browser window (sidebar) ?
> No, this is only about clearing the connection when the whole window is
> "closed" (the compose is often recycled rather and doesn't get really closed).

Ah: comment 0 reads "This is all because of the compose window caching"... ;->

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 319228 [details] [diff] [review]
(Bv1-TB) <addressbook-panel.js>
[Checkin: Comment 22]


Scott, ping ?

Also, could |onClearSearch()| be called directly, and |CloseAbView()| and |SetAbView()| calls removed ?
(See comment 7.)

Comment 15

[David :Bienvenu]

Comment on attachment 319228 [details] [diff] [review]
(Bv1-TB) <addressbook-panel.js>
[Checkin: Comment 22]

switching review request - I'll look at this.

Comment 16

[David :Bienvenu]

Comment on attachment 319228 [details] [diff] [review]
(Bv1-TB) <addressbook-panel.js>
[Checkin: Comment 22]

Serge, is this still the right patch for TB? It looks like the SM version has changed quite a bit since you posted this.

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #16)

"Yes": my comment 6 patch and comment 14 question stand...

Comment 18

[David :Bienvenu]

Comment on attachment 319228 [details] [diff] [review]
(Bv1-TB) <addressbook-panel.js>
[Checkin: Comment 22]

this doesn't seem to fix the problem for me on TB. I'll look into it.

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #18)
> (From update of attachment 319228 [details] [diff] [review])
> this doesn't seem to fix the problem for me on TB.

I'm not sure which problem you are referring to ?
This patch is (supposed to be) only a "cleanup" of bug 321254 patches and followups.

> I'll look into it.

Thanks...

Comment 20

[David :Bienvenu]

the problem I'm referring to is the actual subject of this bug - we're leaking ldap connections as a result of using the address book sidebar from the compose window. Or maybe we've reverted to our bad habit of leaking ldap connections all the time but I'm hopeful that's not true.

It does scream for a regression test with a fake ldap server, though :-)

Comment 21

[Mark Banner (:standard8)]

(In reply to comment #20)
> the problem I'm referring to is the actual subject of this bug - we're leaking
> ldap connections as a result of using the address book sidebar from the compose
> window. Or maybe we've reverted to our bad habit of leaking ldap connections
> all the time but I'm hopeful that's not true.

David, there is a potential problem with nsAbView not freeing LDAP searches (and therefore connections) until shutdown, that I'm covering in bug 438035. This requires various changes which I'm looking at covering there.

The SM patch on this bug is just porting what TB already has which was intending to fix the problem for SeaMonkey, and the TB patch is just tidying some code up.

There is also bug 382446, which David Ascher has seen, but I know he hadn't been using the sidebar.

Comment 22

[David :Bienvenu]

OK, I'll land the TB patch then...

Comment 23

[David :Bienvenu]

Comment on attachment 319228 [details] [diff] [review]
(Bv1-TB) <addressbook-panel.js>
[Checkin: Comment 22]

this doesn't fix the problem at all - it's just tidying-up. Sorry for the confusion.

Comment 24

[Mark Banner (:standard8)]

Checking in mailnews/addrbook/resources/content/addressbook-panel.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook-panel.js,v  <--  addressbook-panel.js
new revision: 1.17; previous revision: 1.16
done