330507 - Crash when clearing a quick search during a LDAP search.

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

Go into the address book window and start a quick search (preferably returning lots of results) and then cancel it whilst the search is still running -> crash.

It crashes at this line:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp&rev=1.33&mark=554#554

If I back out David's patch on bug 206018 this works fine (although may cause other problems looking at that bug) - its because mDirectoryQuery becomes a pointer to nowhere.

Investigating a little further it seems that this section:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/addrbook/src/nsAbLDAPDirectoryQuery.cpp&rev=1.33&mark=204-213#198

should be preventing the address book getting to the crash line but I think for some reason, this section either isn't being set up or something.

Hopefully we can come up with a better fix to sort both of these issues.

Comment 1

[Mark Banner (:standard8)]

Requesting blocking thunderbird 2 - we shouldn't ship with a crash like this and the fix should be reasonably simple.

Comment 2

[David :Bienvenu]

>the fix should be reasonably simple

:-) That has not been my experience. My change in bug 206018 may have caused the problem, or just exposed it - without that change, nothing was ever getting freed, which would hide a multitude of sins. One thing to check is if the assertion I made in that bug is valid, to wit: ldapdirectoryquery strictly owns the listener.

I'll try to recreate this - I wish I had a working Purify again...

Comment 3

[David :Bienvenu]

I was able to recreate the crash. nsAbDirSearchListener has a pointer, not a reference, to mSearchContext - the mSearchContext is the object that's been deleted. That object is an nsAbLDAPDirectory, if I'm reading the code right...that can't be right...

Comment 4

[David :Bienvenu]

This code alway confuses me - iirc, there are multiple nsIAbdirectory objects/rdf resources - one for the directory itself, and one for the search query that's running. It's the latter that's getting deleted, before the nsAbQueryLDAPMessageListener has finished. I'm not sure what cancelling the quick search actually does, because I don't seem to be hitting the cancel code I would expect...

Comment 5

[David :Bienvenu]

the first thing cancelling the search does is clear the mDirectory pointer in nsAbView, by replacing it with an mDirectory for the directory, not the search. That might be the last reference other than js refs that get gc'd away.

Comment 6

[David :Bienvenu]

more info - we're never cancelling the ldap search in this case. 
We bail out of nsAbLDAPDirectory::StartSearch here:
    if (!mIsQueryURI || mQueryString.IsEmpty())
        return NS_OK;

We fail that test because cancelling the quick search just does a "search" over the whole directory. So the ldap query is never cancelled, and eventually, sometimes crashes (depending on whether the js gc causes the directory object to get freed before the query finishes).

So I think the fix is to actually cancel the query. I'll try it out.

Comment 7

[David :Bienvenu]

Attachment: proposed fix

Comment 8

[David :Bienvenu]

the basic premise is that the AbView owns the directoryQuery at this point, and if it's going to create a new directoryQuery, it should cancel the previous directoryQuery so that it can clean up after itself.

Comment 9

[David :Bienvenu]

Attachment: proposed fix

OK, I've basically backed out the change that caused this crash and fixed that bug an other way, by breaking the cycle with the listener, in the OnSearchFinished notification...

Comment 10

[David :Bienvenu]

oops, ignore the nsAbView change - I'm removing that...

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 215712 [details] [diff] [review]
proposed fix

r=me assuming you don't check in the nsAbView.cpp change.

This does appear to fix the crash, and I can't see any further regressions with this fix in place.

Comment 12

[Scott MacGregor]

Comment on attachment 215712 [details] [diff] [review]
proposed fix

without the abview change of course.

Comment 13

[David :Bienvenu]

After removing the abview change, I crash frequently with the remaining changes on the trunk - the ldap connection gets closed, which it shouldn't - we should be re-using the same connection. And when the ldap connection goes away, that causes strange crashes down the road, when the address book is closed. I'm going to see if cancelling the query (the abview change) makes the crashes and extra connections go away, which would be a bit counter-intuitive, but I didn't see those before.

Comment 14

[David :Bienvenu]

I'm not seeing the strange crash I was seeing before, but I am seeing connections getting closed. Time to dive into this again.

Comment 15

[David :Bienvenu]

we were still closing connections w/o this patch, so I think I'm going to go with this patch for now on trunk and 1.8 branch

Comment 16

[Ria Klaassen (not reading all bugmail)]

Could this have broken Super DragAndGo (search function)?
I'm using userChrome.js extension with the Drag'n'go code snippet.
http://forums.mozillazine.org/viewtopic.php?t=397735&postdays=0&postorder=asc&postsperpage=15&

When I select a word, drag it away and release it, it only throws and error instead of doing a search:

Error: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]
Source file: XStringBundle
Line: 17

Some patch checked in between 1.9a1_2006062921 and 1.9a1_2006063014 broke it.
I'm just trying to find out what patch caused this.
 

Comment 17

[David :Bienvenu]

this change should have nothing to do with the super drag and drop error.

Comment 18

[David :Bienvenu]

OK, the checked in fix has issues. It's now possible to get multiple threads going against the same nsLDAPConnection object, which causes a crash, if you do quick search in the ab view. By nulling out mListener in OnSearchFinished, we cause nsAbLDAPDirectoryQuery::DoQuery to re-init the nsLDAPConnection object, which makes it spin up a new thread. I'll have to think about how to fix this.

Comment 19

[David :Bienvenu]

Attachment: proposed fix

instead of clearing listener to break cycle at end of search, just don't hold a reference to the ldap connection, so there's no cycle to begin with.

Comment 20

[David :Bienvenu]

with this patch, I verified that we're still cleaning up ldap connections when the address book is closed...

Comment 21

[David :Bienvenu]

Comment on attachment 243490 [details] [diff] [review]
proposed fix

clearing request - this introduces a crash if you close the address book while an ldap search is going on.  The directory the mListener points to gets deleted. mListener might want a weak ref to the directory...or else we should somehow clear the directory pointer from the listener when the directory gets closed...a weak ref is probably the way to go...

Comment 22

[David :Bienvenu]

also, I should say - for 2.0, the problem is that we never close the ldap connections - it doesn't have this patch, per se.

Comment 23

[David :Bienvenu]

Attachment: fix for leaking connections that doesn't crash

Comment 24

[David :Bienvenu]

that patch makes nsAbLDAPDirectoryQuery null out the nsAbQueryLDAPMessageListener's mDirectoryQuery pointer when the nsAbLDAPDirectoryQuery gets destroyed. And it makes nsAbQueryLDAPMessageListener check if the pointer is null in a few places, to avoid a crash. 

Comment 25

[David :Bienvenu]

Standard8, that patch is for the 2.0 branch.

Comment 26

[David :Bienvenu]

Comment on attachment 244127 [details] [diff] [review]
fix for leaking connections that doesn't crash

for the trunk, this fixes the crash closing the ab view while a search is still going on.

Comment 27

[David :Bienvenu]

https://bugzilla.mozilla.org/attachment.cgi?id=244127 is for the 2.0 branch - on the 2.0 branch, we still leak connections. What I meant to say was that a very similar change on the trunk will fix the crash when you close the ab window while an ldap query is going on, i.e., part of that patch.

Comment 28

[David :Bienvenu]

Attachment: fix one more case

Mark, this might fix the crash you saw - we were getting into js gc which was clearing out the directory query out from under the listener. If I null out the search context as well, and check for that, that fixes one last crash. This patch is also for 2.0, though it will need to go on the trunk as well.

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 244265 [details] [diff] [review]
fix one more case

Ok, this seems to work fine now. The code looks reasonable as well.

Comment 30

[David :Bienvenu]

fixed on branch

Comment 31

[David :Bienvenu]

fixed on trunk.

Comment 32

[Mark Banner (:standard8)]

*** Bug 351642 has been marked as a duplicate of this bug. ***

Comment 33

[juan becerra [:juanb]]

Thunderbird 2.0.0.0; version 2.0.0.0 (20070326) does not crash when canceling an address book search, or when closing the address book window. Adding verified1.8.1.3 keyworkd.