crash in nsImapProtocol::GetMessageSize via nsIMAPBodyShell::Generate. m_hostSessionList is stale

RESOLVED FIXED in Thunderbird 65.0

Status

defect
--
critical
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: jorgk, Assigned: mkmelin)

Tracking

({crash, testcase-wanted, topcrash-thunderbird})

Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr60? fixed, thunderbird64 fixed, thunderbird65 fixed)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

7 months ago
+++ This bug was initially created as a clone of Bug #825513 +++

bp-8acb8015-f617-4390-b678-096a20181115 60.3.1
bp-1a474dcf-5cc5-4538-8d42-8dd4f0181115 60.3.0

From bug 825513 comment #48:
uint32_t nsImapProtocol::GetMessageSize(const char * messageId, bool idsAreUids)
{
  const char *folderFromParser = GetServerStateParser().GetSelectedMailboxName();
  if (!folderFromParser || !messageId || !m_runningUrl || !m_hostSessionList)
    return 0;

  char *folderName = nullptr;
  uint32_t size;

  nsIMAPNamespace *nsForMailbox = nullptr;
  m_hostSessionList->GetNamespaceForMailboxForHost(GetImapServerKey(), <=== line 4574
                                                   folderFromParser,
                                                   nsForMailbox);

I don't see how that line can crash now since we're testing for null right above.

But maybe m_hostSessionList has some stale/free'ed value, see the 0xffffffffe5e5e5e5 in the reports.

I don't particularly like this line in the code:
  m_hostSessionList = aHostSessionList; // no ref count...host session list has life time > connection
And in the .h file:
  nsIImapHostSessionList * m_hostSessionList;

Something like this is asking for trouble. So something our forebears didn't consider :-(
Reporter

Comment 1

7 months ago
Magnus, you're taking this? I'd try making this a "smart" pointer and removing the comment. Of course locking down the object may lead to undesired effects elsewhere.
Assignee

Comment 2

7 months ago
Will look at it.
Assignee: nobody → mkmelin+mozilla
Assignee

Comment 3

7 months ago
Compiles and seems to run ok.
Attachment #9025647 - Flags: review?(jorgk)
Assignee

Updated

7 months ago
Status: NEW → ASSIGNED
Reporter

Comment 4

7 months ago
Comment on attachment 9025647 [details] [diff] [review]
bug1507718_m_hostSessionList.patch

Thanks. I wonder why it wasn't don't like this in the first place and what else will break now.
Attachment #9025647 - Flags: review?(jorgk) → review+

Comment 5

7 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/92b165df282f
crash in nsImapProtocol::GetMessageSize(). Make m_hostSessionList an nsCOMPtr. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Reporter

Updated

7 months ago
Target Milestone: --- → Thunderbird 65.0
Reporter

Updated

7 months ago
Attachment #9025647 - Flags: approval-comm-esr60?
Attachment #9025647 - Flags: approval-comm-beta+
Reporter

Comment 7

7 months ago
Comment on attachment 9025647 [details] [diff] [review]
bug1507718_m_hostSessionList.patch

Doing an early uplift here so we can try this ourselves.
Attachment #9025647 - Flags: approval-comm-esr60? → approval-comm-esr60+
The crash volume has definitely decreased, starting in late Nov, but challenging to determine how much from this bug, vs bug 825513 - and suspect some of the decrease comes from lower crash rate of this signature for version 60.* compared to version 52.9.1 - from which users migrated in significant numbers in Nov and Dec.

the v60.4.0 crashes are all like bp-c3054805-0228-439e-a57f-abedd0190105
0 	xul.dll	nsImapProtocol::GetMessageSize(char const*, bool)	comm/mailnews/imap/src/nsImapProtocol.cpp:4573
1 	xul.dll	nsIMAPBodyShell::Generate(char*)	comm/mailnews/imap/src/nsIMAPBodyShell.cpp:221
2 	xul.dll	nsImapServerResponseParser::ProcessOkCommand(char const*)	comm/mailnews/imap/src/nsImapServerResponseParser.cpp:423
3 	xul.dll	nsImapServerResponseParser::ParseIMAPServerResponse(char const*, bool, char*)	comm/mailnews/imap/src/nsImapServerResponseParser.cpp:256
4 	xul.dll	nsImapProtocol::ParseIMAPandCheckForNewMail(char const*, bool)	comm/mailnews/imap/src/nsImapProtocol.cpp:1945
5 	xul.dll	nsImapProtocol::FetchMessage(nsTString<char> const&, nsIMAPeFetchFields, char const*, unsigned int, unsigned int, char*)	comm/mailnews/imap/src/nsImapProtocol.cpp:3667
6 	xul.dll	nsImapProtocol::FetchTryChunking(nsTString<char> const&, nsIMAPeFetchFields, bool, char*, unsigned int, bool)	comm/mailnews/imap/src/nsImapProtocol.cpp:3700
7 	xul.dll	nsImapProtocol::FallbackToFetchWholeMsg(nsTString<char> const&, unsigned int)	comm/mailnews/imap/src/nsImapProtocol.cpp:3400
8 	xul.dll	nsIMAPBodyShell::Generate(char*)	comm/mailnews/imap/src/nsIMAPBodyShell.cpp:224
9 	xul.dll	nsImapProtocol::ProcessSelectedStateURL()	comm/mailnews/imap/src/nsImapProtocol.cpp:2759
10 	xul.dll	nsImapProtocol::ProcessCurrentURL()	comm/mailnews/imap/src/nsImapProtocol.cpp:1784
11 	xul.dll	nsImapProtocol::ImapThreadMainLoop()	comm/mailnews/imap/src/nsImapProtocol.cpp:1403
12 	xul.dll	nsImapProtocol::Run()	comm/mailnews/imap/src/nsImapProtocol.cpp:1062
You need to log in before you can comment on or make changes to this bug.