301935 - Crash when trying to send mail and collected address book doesn't exist

Status: VERIFIED FIXED

(Thunderbird :: Address Book, defect)

Description

[Mark Banner (:standard8)]

1) Set the collected address book preference to a valid address book (not pab or
cab)
2) delete that address book
3) write an email and send
4) Thunderbird (or SeaMonkey) crashes in address book.

I'll attach a stack or patch later, but I bet this is due to the collecting code
not correctly checking if a address book really exists before trying to write to it.

Comment 1

[Mark Banner (:standard8)]

Attachment: Part 1 - Fix the crash (checked in)

I realise the branch isn't too far away - so I'm doing this patch in two parts.


The first (this one) fixes the crash by checking the address book database is
valid and then adding the appropriate null checks. This is just c++ fixes - and
I will try and get this in for 1.5, although I don't think its a very frequent
scenario (its still a crasher though).

Comment 2

[David :Bienvenu]

Comment on attachment 190339 [details] [diff] [review]
Part 1 - Fix the crash (checked in)

thx for looking at this.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 190339 [details] [diff] [review]
Part 1 - Fix the crash (checked in)

Low risk patch, fixes crasher in tb and sm.

Comment 4

[Mark Banner (:standard8)]

Attachment: Part 2 - Warn the user before they delete the addr book that is set for collecting

This is the second part of the fix. This patch will warn the user differently
(change of message) for the book that is currently being used to collect
addresses to. If they continue, then it will set the collection prefs to
disabled.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 190339 [details] [diff] [review]
Part 1 - Fix the crash (checked in)

Checked in:
2005-07-25 13:58	bugzilla%standard8.demon.co.uk	mozilla/ mailnews/
compose/ src/ nsMsgSend.cpp  1.374   6/3
2005-07-25 13:58	bugzilla%standard8.demon.co.uk	mozilla/ mailnews/
addrbook/ src/ nsAbAddressCollecter.cpp      1.55    11/9

Comment 6

[Scott MacGregor]

I'd prefer us to not throw up a dialog for this case and just reset the
collected address book pref to the default PAB. We already bring up one dialog
when deleting the address book to confirm if you really want to delete it. It
shouldn't take 2 dialogs to delete an address book.

Alternatively you could hijack that dialog and add extra information to it if
you are deleting the address book that collects addresses.

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> I'd prefer us to not throw up a dialog for this case and just reset the
> collected address book pref to the default PAB. We already bring up one dialog
> when deleting the address book to confirm if you really want to delete it. It
> shouldn't take 2 dialogs to delete an address book.
> Alternatively you could hijack that dialog and add extra information to it if
> you are deleting the address book that collects addresses.

Scott, I think you may have misunderstood something here - I did hijack the 
existing delete address book dialog with the extra information. The patch does 
only give 1 dialog when deleting an address book.

Comment 8

[Scott MacGregor]

Comment on attachment 190426 [details] [diff] [review]
Part 2 - Warn the user before they delete the addr book that is set for collecting

yup, I misunderstood.

How about changing the dialog wording: 

If this address book is deleted, %brand% will no longer collect outgoing e-mail
addresses. Are you sure you want to delete this addressbook?

or

This address book is currently being used for e-mail address collection.
%brand% will no longer collect outgoing e-mail addresses. Are you sure you want
to delete this addressbook?

I like the first one myself. Much more succinct.

Do you mind changing mail\components\addrbook\content\addressbook.js while your
fixing this bug? thanks!

Comment 9

[Mark Banner (:standard8)]

Attachment: Part 2 - Warn the user - fix review nits (checked in)

This patch addresses Scott's review comments.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 190729 [details] [diff] [review]
Part 2 - Warn the user - fix review nits (checked in)

Carrying forward previous r & sr. Requesting approval. Low risk patch changing
the interface when we delete address books to be more informative to the user.
Some string changes.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 190729 [details] [diff] [review]
Part 2 - Warn the user - fix review nits (checked in)

Checking in mailnews/addrbook/resources/content/addressbook.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook.js,v  <-- 
addressbook.js
new revision: 1.117; previous revision: 1.116
done
Checking in mailnews/addrbook/resources/locale/en-US/addressBook.properties;
/cvsroot/mozilla/mailnews/addrbook/resources/locale/en-US/addressBook.propertie
s,v  <--  addressBook.properties
new revision: 1.33; previous revision: 1.32
done
Checking in mail/components/addrbook/content/addressbook.js;
/cvsroot/mozilla/mail/components/addrbook/content/addressbook.js,v  <-- 
addressbook.js
new revision: 1.20; previous revision: 1.19
done
Checking in
mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/addressbook/addressBook.pr
operties,v  <--  addressBook.properties
new revision: 1.5; previous revision: 1.4

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

Note to self: verify on Thunderbird trunk once bug 285076 is fixed; this is
already verified in SeaMonkey trunk.

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using version 1.6a1 (20050915) on Windows XP Thunderbird trunk.

Comment 14

[Rick Stockton]

"Bookkeeping" questions:

1) Is comment #12 in regard to RE-verifying? comment #13 seems to say you're checked in and verified, even though the fix for 285076 was backed out?

2) Comment #13: Is this patched only on the "TB 1.6" (Trunk), or also present on the "1.5" Branch? You show Target Milestone = "1.5". Sorry, I don't know CVS, how to see what the rev levels are.

3) If not yet applied to 1.5: I can't build TB, but if *you* build it, I'd be happy to verify on both Linux and Win-XP.