58337 - after importing outlook or eudora email, all new accounts will have identity problems

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P3)

Description

[(not reading, please use [e-mail] instead)]

Branch build 2000-10-27-08MN6: NT4
I thought this was already reported but I can't find the bug.

Overview: After importing Eudora Mail compose a message using an AOL account.
Notice the identity in the From area states "Import Identity" instead of the
name of the AOL account.


Steps to reproduce:
1. Create a new profile
2. Start Mail
3. Cancel out of the Account Wizard after it opens
4. Import Eudora "Mail"
 a. Select File|Import
 b. Select the "Mail" radio button
 c. Select "Eudora", select Next
 d. Select Finish
5. Add an AOL account
 a. Select Edit|Mail/News Account Settings
 b. Select the "New Account" button
 c. In the Account Wizard progress through the dialogs to create the AOL account

Actual Results:
1. The folder for the AOL account displays "Mail for import@imp"
2. With the AOL account selected, create a new message.  The 'From' area shows
the Identity as "Import Identity" when it should reference a specific AOL email
address. Go back to Account Settings and the entries for the AOL Identitiy panel
all look correct. The Account Name, Your Name and Email Address all reference
the correct information.
3. Exit and Restart and the account in the folder pane now displays correctly
(Mail for [e-mail]) but the Identity in the Compose window still states
"Import Identity"
4. Create and send a message. The recipient receives the email and the From
entry looks correct since it references the AOL email address.

Expected Results: Performing an import should not effect the identity names for
any account including AOL and Webmail.

Additional Information:
1. The problem Also occurs:
-  in migrated profiles
-  with Webmail accounts
2. The problem does Not:
- occur with POP or IMAP accounts created using the "ISP or email provider"
radio button in the Account Wizard
- occur with Newsgroup accounts
- if when Importing you choose "Settings" (instead of "Mail"). In this case I
can create any type of account and it looks correct in the folder pane and the
compose window.


------- Additional Comments From Ninoschka Baca 2000-10-27 16:50 -------

Copying Seth since he saw this problem.

Not sure if I should mark this 'rtm' since it occurs after importing "Mail" and
creating an AOL or Webmail account. It could be very confusing and there is no
easy way for a user to fix this since all the account information looks correct
in Account Settings.


------- Additional Comments From Seth Spitzer 2000-10-27 17:03 -------

taking from mscott, this would be my problem (or tony's).

I'll investigate.  I think I have a quess at the problem.


------- Additional Comments From Seth Spitzer 2000-10-27 22:13 -------

I saw this when creating news accounts after importing eudora accounts.

accepting.



------- Additional Comments From Seth Spitzer 2000-10-28 11:51 -------

here's how I can reproduce this:

migrate a 4.x profile (with mail & news)
add a eudora account
add a news account.

I'm drilling down on the problem.  I hope to have a fix soon.


------- Additional Comments From Seth Spitzer 2000-10-28 12:20 -------

ok, I see the problem.  the trick to reproducing this is to quit and restart
after importing the eudora account.

we need to move this bug to bugzilla so we can get tony in on it.

in the import backend code we create identities but never hook them up to
accounts.  it is ok for an account to have a server, but no identity.  "Local
Folders" is like that.  I'm assuming tony has a good reason for creating these
identities (as part of import) but he never cleans up after them.

what happens is we write out prefs for identities, and when we exit and restart
we rebuild the hash tables of identities from identities hooked up to accounts.
 that doesn't match the prefs.  so when we create new identities, we pick up the
old prefs.

the fix will be to clean up and remove the temporary identities that tony
creates.  I'm working on the fix now.


------- Additional Comments From Seth Spitzer 2000-10-28 12:41 -------

marking rtm need info.  this bug leave the account manager in a bad state,
messing up all new accounts.

anyone know how to move this bug to bugzilla.mozilla.org?  or do I have do it by
hand?


------- Additional Comments From Seth Spitzer 2000-10-28 13:33 -------

I've got a fix.

I'll attach it after I move this bug to mozilla.org

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: here's the fix.

Comment 2

[(not reading, please use [e-mail] instead)]

updating keywords and status whiteboard.

tony, can you review and test the fix?

also, can you help explain why we create identity on migration?

accepting.

Comment 3

[tonyr]

I guess you figured out the reason for the identity is that nsIMsgSend requires 
it.  Rich Pizarro (or whoever wrote nsMsgSend can probably answer why an identity 
is required.)

Also, if the identity is only temporary then bug #58342 should become irrelevant?

m_pIdentity is used repeatedly in calls to nsIMsgSend::CreateAndSendMessage.  The 
fix removes the identity from the account manager but leaves m_pIdentity as an 
allocated object.  This object is then used again on the next call to 
CreateAndSendMessage.  In addition the fix should attempt to remove it again from 
the account manager - this might slow down the import a little since this routine 
is called for every single message imported.  

2 Questions about the fix:
1 - If the identity is still useful to nsIMsgSend::ComposeAndSendMessage after it 
is removed from the account manager why not just remove it from the account 
manager as soon as it is created?
2 - If ComposeAndSendMessage needs a completely hooked up identity then the fix 
needs to destroy the identity and make sure m_pIdentity is NULL so it will get 
created again for the next message imported.

Comment 4

[(not reading, please use [e-mail] instead)]

> if the identity is only temporary then bug #58342 should become irrelevant?

if understand the code, we call CreateAndSendMessage() a way to create RFC822
messages?

looking at nsMsgSend.cpp, I see why you need to pass an identity.  it looks like
we generate the headers for the migrated messages using the identity.  I wonder
you moved a migrated message to your drafts folder and sent it, would it look
like it came from "Import Identity ([e-mail])" or from the account
associated with the draft folder?


> If the identity is still useful to nsIMsgSend::ComposeAndSendMessage after it
is removed from the account manager why not just remove it from the account
manager as soon as it is created?

I'm not sure I understand your question.

> the fix needs to destroy the identity and make sure m_pIdentity is NULL so
> it will get created again for the next message imported.

thanks for catching that.  I'll work on a new patch.

creating and destroying the identity for every message we import seems like it
could hurt performance.  I haven't done any performance testing importing large
mailboxes from eudora.  but looking at the code that we call for every message,
creating and removing and identity will not be the most expensive part.

ideally, we should be creating one identity for whole migration process, and
then removing it at the end.

also, any interest in switching this code over to nsCOMPtrs?

Comment 5

[tonyr]

>> If the identity is still useful to nsIMsgSend::ComposeAndSendMessage after it
is removed from the account manager why not just remove it from the account
manager as soon as it is created?

>I'm not sure I understand your question.

In looking at CreateAndSendMessage it appears that the identity is only used to 
generate the message ID and for Fcc processing.  It is also passed to the smtp 
service to send the message but we don't send the message from the import.  So 
what I'm thinking is that rather than having to create and clean up the identity  
for every message imported just modify nsEudoraCompose::CreateIdentity to remove 
the identity from the account manager after it is created and only call 
m_pIdentity->ClearAllValues() on the destructor.  That way, the identity is only 
created and cleaned up once.
I'm going to try this out and see it it works, if it does I'll attach a patch 
and see what you think?

Comment 6

[tonyr]

Attachment: Alternate patch? This one works for me.

Comment 7

[(not reading, please use [e-mail] instead)]

tony, it looks like your patch also fixes some assertions.  that's another bug,
let's not mix the two fixes.  we might be able to get this bug fixed for rtm,
but not the assertion bug.

in the 4.x migration code, we call ClearAllValues() and then RemoveIdentity().
RemoveIdentity() does nothing.  In the future, ClearAllValues() may go away and
RemoveIdentity() will probably clear the prefs and fix up internal data structures.

I've got a version of your fix that does this, I'll attach it.

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: another version of the fix.

Comment 9

[tonyr]

Looks good to me.

Comment 10

[(not reading, please use [e-mail] instead)]

thanks for the review tony.  I'll go get a super review, and mark this puppy rtm+.

if it doesn't make rtm, it will at least make the trunk.

Comment 11

[(not reading, please use [e-mail] instead)]

adding alecf (mailnews emeritus) to the cc list, for possible sr.

Comment 12

[(not reading, please use [e-mail] instead)]

updating summary.  sr=alecf

PDT, you know you want this fix.  Everyone is doing it.  First one is free.

Comment 13

[selmer (gone)]

This bug is in candidate limbo.  We will reconsider this fix once we have a 
candidate in hand, but we can't take this fix before then.  Please check into 
the trunk ASAP.

Comment 14

[selmer (gone)]

rtm++, please checkin ASAP so we can build today.

On your trunk landing, please ensure you're not creating new leaks.

Comment 15

[(not reading, please use [e-mail] instead)]

fix landed on trunk and branch.

Comment 16

[esther]

Using rtm branch builds 2000-11-03 on Win98 and Mac this is fixed.  I tried this
with New Profile (original scenario) and migrated profile (Seth's scenario),
both had correct identity for AOL account and news accounts that were added
after eudora import.  Verified