1507718 - crash in nsImapProtocol::GetMessageSize via nsIMAPBodyShell::Generate. Maybe m_hostSessionList is stale/has a freed value

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ 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 :-(

Comment 1

[Jorg K (CEST = GMT+2)]

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.

Comment 2

[Magnus Melin [:mkmelin]]

Will look at it.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: bug1507718_m_hostSessionList.patch

Compiles and seems to run ok.

Comment 4

[Jorg K (CEST = GMT+2)]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/92b165df282f
crash in nsImapProtocol::GetMessageSize(). Make m_hostSessionList an nsCOMPtr. r=jorgk

Comment 6

[Jorg K (CEST = GMT+2)]

Beta (TB 64 beta 3):
https://hg.mozilla.org/releases/comm-beta/rev/73dcb0c167eaa05273c6e01dc4634f1d9c4cdf73

Comment 7

[Jorg K (CEST = GMT+2)]

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

Doing an early uplift here so we can try this ourselves.

Comment 8

[Jorg K (CEST = GMT+2)]

TB 60.4 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/55f5634ac8652274f32a38ac4e4fabe6ac451895

Comment 9

[Wayne Mery (:wsmwk)]

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