Thunderbird 17.0.3 crashes when calling createSmtpServer after createAccount for new profile

RESOLVED FIXED in Thunderbird 24.0

Status

MailNews Core
Account Manager
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Alexey Ousov, Assigned: aceman)

Tracking

(Blocks: 1 bug, {crash, reproducible})

Thunderbird 24.0
crash, reproducible

Thunderbird Tracking Flags

(thunderbird24-, thunderbird-esr17-)

Details

(crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

1.52 KB, application/octet-stream
Details
3.36 KB, patch
standard8
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
Forgot to write: test case should be launched on clear newly created profile, where no accounts already exist.
(Assignee)

Comment 2

4 years ago
Do you have a crash ID for this?
(Reporter)

Comment 3

4 years ago
Crash ID: bp-e77a9181-6af7-4f61-b705-7e11a2130315
(Assignee)

Comment 4

4 years ago
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.
Component: Account Manager → Account Manager
Product: Thunderbird → MailNews Core

Comment 5

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

Comment 6

4 years ago
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.)
(Assignee)

Comment 7

4 years ago
The attachment seems to be corrupt, not a zip file. Alexey, can you check it?
(Assignee)

Comment 8

4 years ago
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.
Flags: needinfo?(neil)
(Assignee)

Comment 9

4 years ago
Alexey, are you able to build Thunderbird to test a C++ patch?
(Assignee)

Comment 10

4 years ago
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).
Assignee: nobody → acelists
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #725596 - Flags: review?(neil)
(Assignee)

Comment 11

4 years ago
I could reproduce the crash on Linux on trunk with the code from comment 0 put directly into TB.
Severity: normal → major
Keywords: crash, crashreportid, reproducible
OS: Windows 7 → All
Hardware: x86_64 → All
Severity: major → critical
(Reporter)

Comment 12

4 years ago
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 on attachment 725338 [details]
Simple test extension

That was caused by a bug in the MIME type auto-detection routine (bug 848457).
Attachment #725338 - Attachment is obsolete: true
Flags: needinfo?(neil)
(Assignee)

Comment 14

4 years ago
You mean the attachment corruption, not the bug :)
(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.
(Reporter)

Comment 16

4 years ago
(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.
(Assignee)

Comment 17

4 years ago
(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.
(Assignee)

Updated

4 years ago
tracking-thunderbird-esr17: --- → ?
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.
tracking-thunderbird-esr17: ? → -
(Assignee)

Updated

4 years ago
Attachment #725596 - Flags: review?(mbanner)
(Assignee)

Comment 19

4 years ago
Neil, ping :)
Flags: needinfo?(neil)
(Assignee)

Updated

4 years ago
tracking-thunderbird24: --- → ?
(Assignee)

Updated

4 years ago
Blocks: 880602
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
Crash Signature: [@ nsMsgAccountManager::GetDefaultAccount(nsIMsgAccount**) ]
Keywords: crashreportid
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.
tracking-thunderbird24: ? → -
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).
Attachment #725596 - Flags: review?(mbanner) → review-
(Assignee)

Comment 23

4 years ago
Created attachment 766673 [details] [diff] [review]
patch v2

OK, would this work for you?
Attachment #725596 - Attachment is obsolete: true
Attachment #725596 - Flags: review?(neil)
Attachment #766673 - Flags: review?(mbanner)
Comment on attachment 766673 [details] [diff] [review]
patch v2

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

Looks good, thanks.
Attachment #766673 - Flags: review?(mbanner) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/9ff0c7ce1ea2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(neil)
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0
You need to log in before you can comment on or make changes to this bug.