433202 - Crash when trying to search in a deleted address book

Status: VERIFIED FIXED

(Thunderbird :: Address Book, defect)

Description

[Serge Gautherie (:sgautherie)]

Testing bug 321271 comment 7 point 2:

0. Create an empty A.B..
1. Ctrl+M = Start Message Compose.
2. F9 = Open A.B. in sidebar.
3. Select the newly created A.B..
4. Alt+F4 to close M.C..
5. Delete the "selected" A.B..
6. Ctrl+M = Reopen Message Compose.
6r. (Notice that the deleted A.B. is not in the dropdown list anymore,
     but its name is still displayed as selected... :-/)
7. Type in something to Search For.
8. Press Enter.
8r. Crash !

Here are two "identical" crash reports (not so much helpful by themselves):

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

<http://crash-stats.mozilla.com/report/index/eca54363-1eeb-11dd-b025-001cc45a2ce4?date=2008-05-10-23>

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

<http://crash-stats.mozilla.com/report/index/e33de051-1eec-11dd-af83-001cc45a2ce4?date=2008-05-10-23>

Comment 1

[WADA:World Anti-bad-Duping Agency]

Crash was re-produced with Version=3.0a2pre, BuildID=2008052904 (MS Win-XP SP2).
However, step 5.(delete of the empty address boob) was not required in my test to re-produce crash. And crash was observed with following case too.
 (0) Non empty A.B (one Card is added) 
     but other entries than "Contact/Internet/Email: a@a.a.a" are not specified.
 (1) Open Compose window(A.B is selected at side bar), close compose window
 (2) Open Compose window(A.B is selected at side bar), type aaa to "Search For"
     => Crash

Comment 2

[Serge Gautherie (:sgautherie)]

(Did you get a (more useful) crash report ?)

Comment 3

[WADA:World Anti-bad-Duping Agency]

Crash report : (Crash when one Card of a@a.a.a with no name, no nick name etc.)
> Crash ID: bp-65d6f200-30e4-11dd-8cf2-001cc45a2c28
> http://crash-stats.mozilla.com/report/index/65d6f200-30e4-11dd-8cf2-001cc45a2c28?date=2008-06-02-20
Sorry but I don't know it's more useful or not.

Comment 4

[WADA:World Anti-bad-Duping Agency]

Modified problem re-creation procedure.
 (1) Open Compose window, open Side bar,
     and select other Address Book than "Personal Address Book",
     including predefined "Collected Address Book",
     and close Compose window 
 (2) Open Compose window again. (side bar is opened)
     type any text in "Search For" => Crash
Workaround of Crash.
 (2-X) Open Compose window again. (side bar is opened)
     Select other Address Book,
     and select Address Book which was selected at step (1) again,
     and type any text in "Search For" => No Crash

Comment 5

[WADA:World Anti-bad-Duping Agency]

Phenomenon of Comment #0 and Phenomenon of my Comment #4 were slightly different. When Comment #0, delete of address book is possible, but when Comment #4, delete of address book is impossible.
 (0) User defined address book exists(say A.B)
 (1) Comment #0 case : Search is not executed/Delete of A.B is possible
 (1-1) Shutdown Tb, and restart Tb
 (1-2) Open Compose window,Contact Sidebar,Select A.B,
       and close Compose window(without execution of search)  
 (1-3) Open Address Book, Try to delete A.B
       => A.B is deleted
 (1-4) Open Compose window, A.B is selected in Contact Sidebar
       => If type something in "Search For", Crash
       => If select other address book, Crash can't occur,
          because A.B is not listed in selection list any more
 (2) Comment #4 case : Search is executed/Delete of A.B is impossible
 (2-1) Shutdown Tb, and restart Tb
 (2-2) Open Compose window,Contact Sidebar,Select A.B,
       execute search(type something in "Search For"), and close Compose window 
 (2-3) Open Address Book, Try to delete A.B
       => Delete of A.B silently fails(no warning/no error message)
 (2-4) Open Compose window. A.B is selected in Contact Sidebar
       => If type something in "Search For", Crash
       => If select other address book and select A.B again, No Crash

Comment 6

[Boying Lu]

Attachment: patch

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 323545 [details] [diff] [review]
patch

Weird, I still can't reproduce these crashes even with all those steps to repeat. Must be something on windows.

+      abList.value =  kPersonalAddressbookURI;

This isn't needed because the lines below it do the same thing. Just fixing the listener is all you need.

Comment 8

[Boying Lu]

(In reply to comment #7)
> (From update of attachment 323545 [details] [diff] [review])
> Weird, I still can't reproduce these crashes even with all those steps to
> repeat. Must be something on windows.
> 
> +      abList.value =  kPersonalAddressbookURI;
> 
> This isn't needed because the lines below it do the same thing. Just fixing the
> listener is all you need.
> 
No.  Only fixing listener won't fix the issue because when the "compose" window is closed, GetAbView() will return null so the statements contained in the "if" statement won't be executed

Comment 9

[Boying Lu]

Comment on attachment 323545 [details] [diff] [review]
patch

I'll make a new patch

Comment 10

[Boying Lu]

Attachment: patch v1

Comment 11

[Serge Gautherie (:sgautherie)]

Checking
<http://mxr.mozilla.org/seamonkey/search?string=gCurDirectory&case=on&find=%2Fcontent%2FabCommon%5C.js%24&filter=%5E%5B%5E%5C0%5D*%24&tree=seamonkey>
|gCurDirectory| is (already) read "uninitialized" by the |if| test.

While we're here,
could you initialize it with |= null| ?

Comment 12

[Boying Lu]

Attachment: patch v1.1

initialize gCurDirectory

Comment 13

[[e-mail]]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 323545 [details] [diff] [review])
> > Just fixing the listener is all you need.
> No.  Only fixing listener won't fix the issue because when the "compose" window
> is closed, GetAbView() will return null so the statements contained in the "if"
> statement won't be executed
Sorry, but I have to agree with Mark here; without the patch to bug 321271 GetAbView() will never return null, while with the patch to bug 321271 the listener is removed while the compose window is closed.

Comment 14

[[e-mail]]

Comment on attachment 323839 [details] [diff] [review]
patch v1.1

>   Components.classes["@mozilla.org/abmanager;1"]
>             .getService(Components.interfaces.nsIAbManager)
>-            .addAddressBookListener(gAddressBookAbListener,
>+            .addAddressBookListener(gAddressBookPanelAbListener,
>                                     nsIAbListener.directoryRemoved |
>                                     nsIAbListener.itemChanged);
r=me on this change only (regression from bug 410158).

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #13)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 323545 [details] [diff] [review])
> > > Just fixing the listener is all you need.
> > No.  Only fixing listener won't fix the issue because when the "compose" window
> > is closed, GetAbView() will return null so the statements contained in the "if"
> > statement won't be executed
> Sorry, but I have to agree with Mark here; without the patch to bug 321271
> GetAbView() will never return null, while with the patch to bug 321271 the
> listener is removed while the compose window is closed.
> 
I think before I must have checked this on SeaMonkey. So as SeaMonkey hasn't got 321271 it doesn't need the extra change. Thunderbird does.

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> r=me on this change only (regression from bug 410158).

Don't you accept comment 11 (unrelated) change too ?

Comment 17

[Boying Lu]

> Sorry, but I have to agree with Mark here; without the patch to bug 321271
> GetAbView() will never return null, 

When the compose window is closed, the registered AbPanelOnComposerClose() will be  called, so CloseAbView() is called. In CloseAbView(), gAbView is set to null.
In this case, GetAbView() will return null.

Comment 18

[Mark Banner (:standard8)]

(In reply to comment #17)
> > Sorry, but I have to agree with Mark here; without the patch to bug 321271
> > GetAbView() will never return null, 
> 
> When the compose window is closed, the registered AbPanelOnComposerClose() will
> be  called, so CloseAbView() is called. In CloseAbView(), gAbView is set to
> null.
> In this case, GetAbView() will return null.
> 
Please read my comment 15 - SeaMonkey doesn't need the extra change. Thunderbird does. The code is slightly different between the two apps.

Comment 19

[Boying Lu]

(In reply to comment #18)
> (In reply to comment #17)
> > > Sorry, but I have to agree with Mark here; without the patch to bug 321271
> > > GetAbView() will never return null, 
> > 
> > When the compose window is closed, the registered AbPanelOnComposerClose() will
> > be  called, so CloseAbView() is called. In CloseAbView(), gAbView is set to
> > null.
> > In this case, GetAbView() will return null.
> > 
> Please read my comment 15 - SeaMonkey doesn't need the extra change.
> Thunderbird does. The code is slightly different between the two apps.
> 
I see thanks

Comment 20

[Boying Lu]

Attachment: [checked in] patch for seamonkey

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 324421 [details] [diff] [review]
[checked in] patch for seamonkey

Checking in mailnews/addrbook/resources/content/abCommon.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/abCommon.js,v  <--  abCommon.js
new revision: 1.120; previous revision: 1.119
done
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.18; previous revision: 1.17
done

Comment 22

[Serge Gautherie (:sgautherie)]

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

V.Fixed, SeaMonkey (crash) part.

Comment 23

[Serge Gautherie (:sgautherie)]

(In reply to comment #22)
> V.Fixed, SeaMonkey (crash) part.

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

While SM did need this fix too,
it actually did not have the crash behavior:
that's what confused me about bug 439056 :-<
Sorry.

*****

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

TB has not got its patch yet, and is still crashing.
With recent nightlies, it crashes as soon as I type a character at step 7 (that's new ?), and BreakPad does not catch it (it used too !).
(Maybe a regression from some other bug ... or my computer misbehaves ??)

Comment 24

[Serge Gautherie (:sgautherie)]

(In reply to comment #22)
> [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008061302
> SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
> 
> V.Fixed, SeaMonkey (crash) part.

This build did not have this patch !

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008061305 SeaMonkey/2.0a1pre] (SEA-WIN32-TBOX-trunk) (W2Ksp4)

has it.

Comment 25

[Boying Lu]

Do we really need to call CloseAbView() in AbPanelOnComposerClose() 
(in mail/components/addrbook/content/abContactsPanel.js)?

Comment 26

[Mark Banner (:standard8)]

(In reply to comment #25)
> Do we really need to call CloseAbView() in AbPanelOnComposerClose() 
> (in mail/components/addrbook/content/abContactsPanel.js)?
> 
Yes, if we don't then the search doesn't get tidied up correctly when we close the compose window, and that can lead to us keeping connections open, which will lead to things like bug 382446.

I am working on improving how the nsAbView things work, but I want the patch for TB for this bug to land first, especially as its going to take me a little while yet to sort out nsAbView.

Comment 27

[Boying Lu]

(In reply to comment #26)
> (In reply to comment #25)
> > Do we really need to call CloseAbView() in AbPanelOnComposerClose() 
> > (in mail/components/addrbook/content/abContactsPanel.js)?
> > 
> Yes, if we don't then the search doesn't get tidied up correctly when we close
> the compose window, and that can lead to us keeping connections open, which
> will lead to things like bug 382446.
> 
If so, can you accept my following patch for TB?
--- mail/components/addrbook/content/abCommon.js	2 Jun 2008 07:53:36 -0000	1.34
+++ mail/components/addrbook/content/abCommon.js	5 Jun 2008 09:22:10 -0000
@@ -44,7 +44,7 @@
 var gAbResultsTree = null;
 var gAbView = null;
 var gAddressBookBundle;
-var gCurDirectory;
+var gCurDirectory = null;
 
 var rdf = Components.classes["@mozilla.org/rdf/rdf-service;1"].getService(Components.interfaces.nsIRDFService);
 var gPrefs = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
Index: mail/components/addrbook/content/abContactsPanel.js
===================================================================
RCS file: /cvsroot/mozilla/mail/components/addrbook/content/abContactsPanel.js,v
retrieving revision 1.18
diff -u -r1.18 abContactsPanel.js
--- mail/components/addrbook/content/abContactsPanel.js	16 May 2008 08:55:44 -0000	1.18
+++ mail/components/addrbook/content/abContactsPanel.js	5 Jun 2008 09:22:10 -0000
@@ -85,7 +85,7 @@
       // check if the item being removed is the directory
       // that we are showing in the addressbook sidebar
       // if so, select the person addressbook (it can't be removed)
-      if (directory == GetAbView().directory) {
+      if (directory.URI == gCurDirectory) {
           var abPopup = document.getElementById('addressbookList');
           abPopup.value = kPersonalAddressbookURI;
           LoadPreviouslySelectedAB();
@@ -156,7 +156,7 @@
   // This listener only cares when a directory is removed or modified.
   Components.classes["@mozilla.org/abmanager;1"]
             .getService(Components.interfaces.nsIAbManager)
-            .addAddressBookListener(gAddressBookAbListener,
+            .addAddressBookListener(gAddressBookPanelAbListener,
                                     nsIAbListener.directoryRemoved |
                                     nsIAbListener.itemChanged);

Comment 28

[Mark Banner (:standard8)]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > Do we really need to call CloseAbView() in AbPanelOnComposerClose() 
> > > (in mail/components/addrbook/content/abContactsPanel.js)?
> > > 
> > Yes, if we don't then the search doesn't get tidied up correctly when we close
> > the compose window, and that can lead to us keeping connections open, which
> > will lead to things like bug 382446.
> > 
> If so, can you accept my following patch for TB?

Yes that would work fine. Please attach in the normal way and request review from myself.

Comment 29

[Boying Lu]

Attachment: [checked in] patch for TB

Thanks a lot

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 325244 [details] [diff] [review]
[checked in] patch for TB

Thanks for doing this. r=me.

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 325244 [details] [diff] [review]
[checked in] patch for TB

Checking in mail/components/addrbook/content/abCommon.js;
/cvsroot/mozilla/mail/components/addrbook/content/abCommon.js,v  <--  abCommon.js
new revision: 1.35; previous revision: 1.34
done
Checking in mail/components/addrbook/content/abContactsPanel.js;
/cvsroot/mozilla/mail/components/addrbook/content/abContactsPanel.js,v  <--  abContactsPanel.js
new revision: 1.20; previous revision: 1.19
done

Comment 32

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008061608 Thunderbird/3.0a2pre] (TBNEWREF-WIN32--trunk) (W2Ksp4)

V.Fixed