Closed
Bug 283223
Opened 19 years ago
Closed 19 years ago
nsAccountManager::GetAccount should not create a new account if none is found
Categories
(SeaMonkey :: MailNews: Account Configuration, defect)
SeaMonkey
MailNews: Account Configuration
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: shaver, Assigned: shaver)
Details
Attachments
(1 file, 1 obsolete file)
9.52 KB,
patch
|
Bienvenu
:
review+
mscott
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•19 years ago
|
||
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.)
Attachment #175239 -
Flags: superreview?(mscott)
Attachment #175239 -
Flags: review?(bienvenu)
Comment 2•19 years ago
|
||
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.
Attachment #175239 -
Flags: review?(bienvenu) → review+
Assignee | ||
Comment 3•19 years ago
|
||
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!
Attachment #175239 -
Flags: superreview?(mscott)
Assignee | ||
Comment 4•19 years ago
|
||
Committed, thanks!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 5•19 years ago
|
||
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.
Assignee | ||
Comment 6•19 years ago
|
||
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 7•19 years ago
|
||
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*
Attachment #175239 -
Attachment is obsolete: true
Attachment #175495 -
Flags: superreview?(mscott)
Attachment #175495 -
Flags: review?(bienvenu)
Comment 8•19 years ago
|
||
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 ...
Attachment #175495 -
Flags: review?(bienvenu) → review+
Comment 9•19 years ago
|
||
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.
Attachment #175495 -
Flags: superreview?(mscott) → superreview+
Assignee | ||
Comment 10•19 years ago
|
||
Yeah, it sure does bring up the account wizard. FIXED on trunk. Thanks!
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•19 years ago
|
||
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.)
You need to log in
before you can comment on or make changes to this bug.
Description
•