851467 - Thunderbird 17.0.3 crashes when calling createSmtpServer after createAccount for new profile

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Alexey Ousov]

Attachment: Simple test extension

User Agent: Opera/9.80 (Windows NT 6.1; WOW64) Presto/2.12.388 Version/12.14

Steps to reproduce:

If extension tries to create new account and calls createAccount prior calling createSmtpServer, Thunderbird crashes. Simple test case listed below:

function accountManagerTest() {
    var accman = Components.classes['@mozilla.org/messenger/account-manager;1'].getService(Components.interfaces.nsIMsgAccountManager);
    var smtp = Components.classes['@mozilla.org/messengercompose/smtp;1'].getService(Components.interfaces.nsISmtpService);
    accman.createAccount();
    smtp.createSmtpServer();
}

Also simple xpi reproducing issue attached to this report. To check install extension and press Tools->Account manager test. Thunderbird will crash.


Actual results:

Thunderbird crashes


Expected results:

No crash

Comment 1

[Alexey Ousov]

Forgot to write: test case should be launched on clear newly created profile, where no accounts already exist.

Comment 2

[:aceman]

Do you have a crash ID for this?

Comment 3

[Alexey Ousov]

Crash ID: bp-e77a9181-6af7-4f61-b705-7e11a2130315

Comment 4

[:aceman]

Seems to crash in nsMsgAccountManager::GetDefaultAccount http://hg.mozilla.org/releases/comm-esr17/annotate/5967074c61d4/mailnews/base/src/nsMsgAccountManager.cpp#l851 .

The usual flow we use in tests is to create the server first, then identity, then account and link them. But of course, it should not crash if this is not followed.

Comment 5

[[e-mail]]

Aha, so GetDefaultAccount checks for there being no account, but it doesn't account (pun intended) for the case of no incoming server.

Comment 6

[:aceman]

Should it then fail somewhere starting at http://hg.mozilla.org/releases/comm-esr17/annotate/5967074c61d4/mailnews/base/src/nsMsgAccountManager.cpp#l822?

Do we add a check of return value of account->GetIncomingServer ?

(I have not yet tested the extension so can't confirm the crash.)

Comment 7

[:aceman]

The attachment seems to be corrupt, not a zip file. Alexey, can you check it?

Comment 8

[:aceman]

Neil, what about changing NS_ADDREF to NS_IF_ADDREF ? The m_defaultAccount can still be null in the case here. I see from the file history that this was changed some time in the past, but not sure why.

Comment 9

[:aceman]

Alexey, are you able to build Thunderbird to test a C++ patch?

Comment 10

[:aceman]

Attachment: patch

This prevents the crash for me. I made this scenario behave as when no account is found (in that case we return error, not NS_OK).

Comment 11

[:aceman]

I could reproduce the crash on Linux on trunk with the code from comment 0 put directly into TB.

Comment 12

[Alexey Ousov]

Attachment: Simple test extension2

I don't know why, but test extension attachment was corrupted. First 32 bytes were stripped off. Uploading correct version.

Comment 13

[[e-mail]]

Comment on attachment 725338 [details]
Simple test extension

That was caused by a bug in the MIME type auto-detection routine (bug 848457).

Comment 14

[:aceman]

You mean the attachment corruption, not the bug :)

Comment 15

[[e-mail]]

(In reply to aceman from comment #8)
> Neil, what about changing NS_ADDREF to NS_IF_ADDREF ? The m_defaultAccount
> can still be null in the case here. I see from the file history that this
> was changed some time in the past, but not sure why.
I see there was an outstanding NEEDINFO that I accidentally cancelled, but then I noticed that your patch had switched to throwing an exception anyway, which is probably what I would have suggested.

Comment 16

[Alexey Ousov]

(In reply to :aceman from comment #9)
> Alexey, are you able to build Thunderbird to test a C++ patch?

I manually applied your patch to 17.0.3 and built it. It seems to work, no crashes on test extension. Also my real extension which creates mail accounts doesn't crash now, account created successfully.

Comment 17

[:aceman]

(In reply to [e-mail] from comment #15)
> I see there was an outstanding NEEDINFO that I accidentally cancelled, but
> then I noticed that your patch had switched to throwing an exception anyway,
> which is probably what I would have suggested.

I left it at NS_ADDREF but covered the null case by a new early return.

I added an assertion in case we still missed some case where the default can stay at null and would crash at the NS_IF_ADDREF.

Can you review it now?

(In reply to Alexey Ousov from comment #16)
> I manually applied your patch to 17.0.3 and built it. It seems to work, no
> crashes on test extension. Also my real extension which creates mail
> accounts doesn't crash now, account created successfully.
Thanks for the test. But in the extension make sure you create the server as soon as possible after the account and smtp server and link it to the account. We prevented the crash you found, but if you access the account in any way before it has a server you may trigger some other unexpected behaviour. Simply accounts are supposed to have servers so some code may rely on that without checking.

Comment 18

[Mark Banner (:standard8)]

Not tracking - this appears to be limited to extensions that call in a particular order (which afaict can be worked around), so not a significant issue.

Comment 19

[:aceman]

Neil, ping :)

Comment 20

[Wayne Mery (:wsmwk)]

bp-e77a9181-6af7-4f61-b705-7e11a2130315 
nsMsgAccountManager::GetDefaultAccount(nsIMsgAccount**) 
0	xul.dll	nsMsgAccountManager::GetDefaultAccount	mailnews/base/src/nsMsgAccountManager.cpp:851
1	xul.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
2	xul.dll	XPCWrappedNative::CallMethod	js/xpconnect/src/XPCWrappedNative.cpp:2367
3	xul.dll	XPC_WN_GetterSetter	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1526
4	mozjs.dll	js::InvokeKernel	js/src/jsinterp.cpp:352 

crashes do exist in the wild. looks like all are startup.
some however have a different stack involving nsMessengerWinIntegration. example ...
bp-8d016dea-29ce-40fc-95d6-eb1442130304
0	xul.dll	nsMsgAccountManager::GetDefaultAccount	mailnews/base/src/nsMsgAccountManager.cpp:851
1	xul.dll	nsMessengerWinIntegration::SetupInbox	mailnews/base/src/nsMessengerWinIntegration.cpp:1025
2	xul.dll	nsMessengerWinIntegration::UpdateUnreadCount	mailnews/base/src/nsMessengerWinIntegration.cpp:1097
3	xul.dll	nsMessengerWinIntegration::OnItemIntPropertyChanged	mailnews/base/src/nsMessengerWinIntegration.cpp:900
4	xul.dll	nsMsgMailSession::OnItemIntPropertyChanged	mailnews/base/src/nsMsgMailSession.cpp:133
5	xul.dll	nsMsgDBFolder::NotifyIntPropertyChanged	mailnews/base/util/nsMsgDBFolder.cpp:4926
6	xul.dll	nsMsgDBFolder::UpdateSummaryTotals	mailnews/base/util/nsMsgDBFolder.cpp:4100
7	xul.dll	nsImapMailFolder::UpdateSummaryTotals	mailnews/imap/src/nsImapMailFolder.cpp:1773
8	xul.dll	nsImapMailFolder::GetDatabase	mailnews/imap/src/nsImapMailFolder.cpp:614
9	xul.dll	nsImapMailFolder::GetDBFolderInfoAndDB	mailnews/imap/src/nsImapMailFolder.cpp:2076
10	xul.dll	nsMsgDBFolder::ReadDBFolderInfo	mailnews/base/util/nsMsgDBFolder.cpp:628
11	xul.dll	nsMsgDBFolder::GetFlags	mailnews/base/util/nsMsgDBFolder.cpp:1236
12	xul.dll	nsImapMailFolder::AddSubfolderWithPath	mailnews/imap/src/nsImapMailFolder.cpp:384
13	xul.dll	nsImapMailFolder::CreateClientSubfolderInfo	mailnews/imap/src/nsImapMailFolder.cpp:943
14	xul.dll	nsImapMailFolder::GetSubFolders	mailnews/imap/src/nsImapMailFolder.cpp:577
15	xul.dll	nsMsgDBFolder::GetChildWithURI	mailnews/base/util/nsMsgDBFolder.cpp:3485
16	xul.dll	nsImapIncomingServer::GetExistingMsgFolder	mailnews/imap/src/nsImapIncomingServer.cpp:3249
17	xul.dll	nsImapIncomingServer::GetMsgFolderFromURI	mailnews/imap/src/nsImapIncomingServer.cpp:3203
18	xul.dll	nsSpamSettings::GetSpamFolderURI	mailnews/base/src/nsSpamSettings.cpp:539
19	xul.dll	nsSpamSettings::UpdateJunkFolderState	mailnews/base/src/nsSpamSettings.cpp:424
20	xul.dll	nsSpamSettings::Initialize	mailnews/base/src/nsSpamSettings.cpp:414
21	xul.dll	nsMsgIncomingServer::GetSpamSettings	mailnews/base/util/nsMsgIncomingServer.cpp:2095
22	xul.dll	nsMsgPurgeService::PerformPurge	mailnews/base/src/nsMsgPurgeService.cpp:243

Comment 21

[Mark Banner (:standard8)]

Not tracking for 24 either, due to pretty much the same reasons as 17. I would like to get this fixed though, but it appears to need Neil's input at the moment.

Comment 22

[Mark Banner (:standard8)]

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

Review of attachment 725596 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +799,5 @@
>        }
>      }
>    }
>  
> +  NS_ASSERTION(m_defaultAccount, "Default account is null, when not expected!");

I think rather than assert and crash, we should just return a failure notification. That way, if we do happen to hit it, we'll at least attempt to do something that doesn't make TB disappear from the user.

@@ +812,5 @@
>    {
>      nsCOMPtr<nsIMsgAccount> oldAccount = m_defaultAccount;
>      m_defaultAccount = aDefaultAccount;
> +    (void) setDefaultAccountPref(aDefaultAccount); // it's ok if this fails
> +    (void) notifyDefaultServerChange(oldAccount, aDefaultAccount); // ok if notifications fail

On both the lines here, you can remove the comments as the (void) is basically saying the same thing (i.e. its intentional).

Comment 23

[:aceman]

Attachment: patch v2

OK, would this work for you?

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 766673 [details] [diff] [review]
patch v2

Review of attachment 766673 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

Comment 25

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/9ff0c7ce1ea2