283223 - nsAccountManager::GetAccount should not create a new account if none is found

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect)

Description

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Right now, if you call GetAccount with a non-existent key, it will create one
for you with that key.  This guarantees you an assertion (when there is no
incoming server pref for this newborn account), and is not especially useful
behaviour.  Quite a bit of code in Thunderbird and Mozmail already seems to
think that GetAccount can return a null account if the key isn't matched, and
the patch that will come up shortly will fix the remaining callers.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: Make nsMsgAccountManager::GetAccount return null if it doesn't know about an account

Behold!

Most of the callers were already just fine with getAccount returning null, and
looked like they had expected it to be possible all along.  The more
interesting cases were:

 - GetDefaultAccount, which _might_ have been relying on the automatic creation

   if the defaultKey wasn't in prefs, but probably wasn't written with that
case
   in mind at all.  I made it fall through to the 'nothing in prefs' case if
the
   pref value didn't result in a known account.

 - LoadAccounts, which was depending more on the fallthrough to keyed accounts
   being created on a miss in the hash table.  I changed it to call
   createKeyedAccount directly, and from where LoadAccounts() is called I think

   that's the right thing for now, but I think that what we really want is for
   getAccount() to consult the hash table, then load from prefs if the hash
   table lookup misses, then return NULL.  (I also made it handle bogus entries

   in there more gracefully, because when assertions are made fatal, we were
   going to get hurt pretty badly here.)

 - a pair of JS callers were assuming not only that getAccount would always
   give back an account, but that it would have a legit incomingServer, etc. as

   well.  If the key is unknown, that's not the case, so I added some exception

   tossing in that case, which should at least make it easier to debug if we
   start hitting it.  (I wasn't able to, in my testing.)

Comment 2

[David :Bienvenu]

Comment on attachment 175239 [details] [diff] [review]
Make nsMsgAccountManager::GetAccount return null if it doesn't know about an account

looks good - might want to let QA know they should bang a bit on new acount/new
profile stuff.

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 175239 [details] [diff] [review]
Make nsMsgAccountManager::GetAccount return null if it doesn't know about an account

Agreed on the QA focus.  Thanks!

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Committed, thanks!

Comment 5

[Scott MacGregor]

why was my request for an sr on this removed? I hadn't finished looking at it
yet. Furthemore, this should be fixed for Thunderbird as well since it was fixed
for the suite only.

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Ugh, sorry, I had it in my head that mail and mailnews were one-review code
areas, though I can't really say why.  So I used both review flags just out of
pre-duplicate-flag habit to ask for either of you to review.

Mea maxima culpa.  I'll back it out.

What did I miss for Thunderbird?

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: As before, plus the missing patch-piece for tbird's MsgComposeCommands.js

Edit in mail+mailnews, build in mail+mailnews, diff-and-commit in mailnews,
sigh.

Scott: your review and continued indulgence humbly requested.
David: if you're happy with the additional bit, can you remark?

*off to hide in the corner*

Comment 8

[David :Bienvenu]

Comment on attachment 175495 [details] [diff] [review]
As before, plus the missing patch-piece for tbird's MsgComposeCommands.js

seems OK - sorry about missing the thunderbird part. It shouldn't have caused
problems except in corrupt prefs.js ...

Comment 9

[Scott MacGregor]

Comment on attachment 175495 [details] [diff] [review]
As before, plus the missing patch-piece for tbird's MsgComposeCommands.js

thanks for fixing this for thundebird too.

Please make sure that the account wizard still comes up by default for new
profiles that don't have any accounts (it loos like it still will).

Thanks Mike.

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yeah, it sure does bring up the account wizard.  FIXED on trunk. Thanks!

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I'm seeing some strangeness around early-startup new account creation that I am
tempted to blame on my work here.  If I find out more today I will likely file a
bug for that fallout.

(The symptom I'm seeing is that a duplicate account1 is created, and setting
.key to something like "lightning-fake-account" doesn't help matters, but that's
a very superficial analysis.)