Last Comment Bug 851467 - Thunderbird 17.0.3 crashes when calling createSmtpServer after createAccount for new profile
: Thunderbird 17.0.3 crashes when calling createSmtpServer after createAccount ...
Status: RESOLVED FIXED
: crash, reproducible
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: 17
: All All
: -- critical (vote)
: Thunderbird 24.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks: 880602
  Show dependency treegraph
 
Reported: 2013-03-15 04:03 PDT by Alexey Ousov
Modified: 2013-06-24 11:40 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
-
-


Attachments
Simple test extension (1.49 KB, application/octet-stream)
2013-03-15 04:03 PDT, Alexey Ousov
no flags Details
patch (3.43 KB, patch)
2013-03-15 14:14 PDT, :aceman
standard8: review-
Details | Diff | Splinter Review
Simple test extension2 (1.52 KB, application/octet-stream)
2013-03-17 20:07 PDT, Alexey Ousov
no flags Details
patch v2 (3.36 KB, patch)
2013-06-24 07:14 PDT, :aceman
standard8: review+
Details | Diff | Splinter Review

Description Alexey Ousov 2013-03-15 04:03:58 PDT
Created attachment 725338 [details]
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 2013-03-15 04:07:23 PDT
Forgot to write: test case should be launched on clear newly created profile, where no accounts already exist.
Comment 2 :aceman 2013-03-15 04:13:42 PDT
Do you have a crash ID for this?
Comment 3 Alexey Ousov 2013-03-15 05:22:28 PDT
Crash ID: bp-e77a9181-6af7-4f61-b705-7e11a2130315
Comment 4 :aceman 2013-03-15 05:38:33 PDT
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 neil@parkwaycc.co.uk 2013-03-15 05:46:28 PDT
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 2013-03-15 05:55:52 PDT
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 2013-03-15 13:20:03 PDT
The attachment seems to be corrupt, not a zip file. Alexey, can you check it?
Comment 8 :aceman 2013-03-15 13:45:21 PDT
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 2013-03-15 13:48:44 PDT
Alexey, are you able to build Thunderbird to test a C++ patch?
Comment 10 :aceman 2013-03-15 14:14:30 PDT
Created attachment 725596 [details] [diff] [review]
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 2013-03-15 14:18:58 PDT
I could reproduce the crash on Linux on trunk with the code from comment 0 put directly into TB.
Comment 12 Alexey Ousov 2013-03-17 20:07:46 PDT
Created attachment 725979 [details]
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 neil@parkwaycc.co.uk 2013-03-18 02:41:32 PDT
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 2013-03-18 04:01:16 PDT
You mean the attachment corruption, not the bug :)
Comment 15 neil@parkwaycc.co.uk 2013-03-18 04:11:27 PDT
(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 2013-03-18 04:29:44 PDT
(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 2013-03-18 04:38:26 PDT
(In reply to neil@parkwaycc.co.uk 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, afk until Dec) 2013-04-09 05:48:43 PDT
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 2013-05-29 01:51:10 PDT
Neil, ping :)
Comment 20 Wayne Mery (:wsmwk, NI for questions) 2013-06-07 01:11:02 PDT
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, afk until Dec) 2013-06-19 02:07:08 PDT
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, afk until Dec) 2013-06-24 03:59:30 PDT
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 2013-06-24 07:14:35 PDT
Created attachment 766673 [details] [diff] [review]
patch v2

OK, would this work for you?
Comment 24 Mark Banner (:standard8, afk until Dec) 2013-06-24 10:56:10 PDT
Comment on attachment 766673 [details] [diff] [review]
patch v2

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

Looks good, thanks.
Comment 25 Mark Banner (:standard8, afk until Dec) 2013-06-24 11:40:39 PDT
https://hg.mozilla.org/comm-central/rev/9ff0c7ce1ea2

Note You need to log in before you can comment on or make changes to this bug.