Closed Bug 1507718 Opened 2 years ago Closed 2 years ago

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


(MailNews Core :: Networking: IMAP, defect)

Not set


(thunderbird_esr60? fixed, thunderbird64 fixed, thunderbird65 fixed)

Thunderbird 65.0
Tracking Status
thunderbird_esr60 ? fixed
thunderbird64 --- fixed
thunderbird65 --- fixed


(Reporter: jorgk-bmo, Assigned: mkmelin)


(Blocks 1 open bug)


(Keywords: crash, testcase-wanted, topcrash-thunderbird)

Crash Data


(1 file)

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

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 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 :-(
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.
Will look at it.
Assignee: nobody → mkmelin+mozilla
Compiles and seems to run ok.
Attachment #9025647 - Flags: review?(jorgk)
Comment on attachment 9025647 [details] [diff] [review]

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+
Pushed by
crash in nsImapProtocol::GetMessageSize(). Make m_hostSessionList an nsCOMPtr. r=jorgk
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
Attachment #9025647 - Flags: approval-comm-esr60?
Attachment #9025647 - Flags: approval-comm-beta+
Comment on attachment 9025647 [details] [diff] [review]

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
Blocks: 1565517
Summary: crash in nsImapProtocol::GetMessageSize via nsIMAPBodyShell::Generate. m_hostSessionList is stale → crash in nsImapProtocol::GetMessageSize via nsIMAPBodyShell::Generate. Maybe m_hostSessionList is stale/has a freed value
You need to log in before you can comment on or make changes to this bug.