perhaps related to bug 1264302 / bug 1216951 and friends. exists in 58 beta and 56 beta. bp-05147860-e5c7-4940-842e-ac06c0180308 59.0b2 ============================================================= Top 10 frames of crashing thread: 0 xul.dll nsImapProtocol::HandleMessageDownLoadLine C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapProtocol.cpp:3995 1 xul.dll nsIMAPBodypart::GenerateBoundary C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsIMAPBodyShell.cpp:484 2 xul.dll nsIMAPBodypartMultipart::Generate C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsIMAPBodyShell.cpp:988 3 xul.dll nsIMAPBodypartMultipart::Generate C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsIMAPBodyShell.cpp:985 4 xul.dll nsIMAPBodypartMultipart::Generate C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsIMAPBodyShell.cpp:985 5 xul.dll nsIMAPBodypartMessage::Generate C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsIMAPBodyShell.cpp:842 6 xul.dll nsIMAPBodyShell::Generate C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsIMAPBodyShell.cpp:263 7 xul.dll nsImapServerResponseParser::ProcessOkCommand C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:422 8 xul.dll nsImapServerResponseParser::ParseIMAPServerResponse C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:255 9 xul.dll nsImapProtocol::ParseIMAPandCheckForNewMail C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapProtocol.cpp:1944 =============================================================

Looking at the beta source, it's crashing on the first line quoted here: if (((m_downloadLineCache->CurrentUID() != GetServerStateParser().CurrentResponseUID()) && !m_downloadLineCache->CacheEmpty()) || (m_downloadLineCache->SpaceAvailable() < lineLength + 1) ) FlushDownloadCache(); No idea, perhaps 'm_downloadLineCache' is null here? A reproducible case would help.

Jorg, thanks. I will try to get testcase

(In reply to Wayne Mery (:wsmwk) from comment #2) > Jorg, thanks. I will try to get testcase Nothing actionable so far, unfortunately

bp-4d8842b8-3cf7-4cc1-9d25-59dd30180928 is crashing at https://dxr.mozilla.org/comm-esr60/source/mailnews/imap/src/nsImapProtocol.cpp#3850 uint32_t lineLength = strlen(messageLine); That would make messageLine null? The precondition/assertion doesn't do anything on non-debug builds.

I don't expect to find a testcase

(In reply to Magnus Melin [:mkmelin] from comment #4) > bp-4d8842b8-3cf7-4cc1-9d25-59dd30180928 is crashing at > https://dxr.mozilla.org/comm-esr60/source/mailnews/imap/src/nsImapProtocol. > cpp#3850 > > uint32_t lineLength = strlen(messageLine); > > That would make messageLine null? The precondition/assertion doesn't do > anything on non-debug builds.

What's the question? The code is: const char *messageLine = line; uint32_t lineLength = strlen(messageLine); and 'line' is passed in. As Magnus said, something is fishy with argument checking in that function. Magnus, would you like to propose a patch?

I suppose this would be it. Checked the callers and didn't find any that didn't check. There's a few that pass in a member variable, so maybe it's one of those who will mess around with the member inbetween the check and the calling of this.

Comment on attachment 9013262 [details] [diff] [review] bug1444389_line_null_crash.patch Review of attachment 9013262 [details] [diff] [review]: ----------------------------------------------------------------- Well, this looks a bit like a band-aid, but I don't have a better idea. That interface is truly terrible, but sadly the three parameter variant is called twice in nsImapServerResponseParser.cpp :-( - So not easy to remove. ::: mailnews/imap/src/nsImapProtocol.cpp @@ +3842,5 @@ > // ensured *before* invoking this method). > void nsImapProtocol::HandleMessageDownLoadLine(const char *line, bool isPartialLine, > char *lineCopy) > { > + if (line == nullptr) Do you prefer this, or !line? Or maybe more modern: MOZ_ASSERT(line, "line must not be null"); if (!line) return; or if (NS_WARN_IF(line == nullptr)) return; @@ +3844,5 @@ > char *lineCopy) > { > + if (line == nullptr) > + { > + NS_WARNING("line must be set"); I'd go for the option above. What does "be set" mean?

I have someone who crashes a lot, but we don't have a clear testcase nor steps. Hopefully he will be willing to test a patched build.

That is, in addition to Richard ... if he chooses to help test

Looks like the official thing to do is NS_ENSURE_ARG_POINTER(arg) which is NS_ENSURE_TRUE(arg, NS_ERROR_INVALID_POINTER). Since we don't have a return value, I used NS_ENSURE_TRUE_VOID().

Comment on attachment 9015082 [details] [diff] [review] bug1444389_line_null_crash.patch (JK) [Triage Comment]

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/d3a3e12d2654 Fix crash in nsImapProtocol::HandleMessageDownLoadLine(). r=jorgk

Comment on attachment 9015082 [details] [diff] [review] bug1444389_line_null_crash.patch (JK) [Triage Comment]

TB 60.3 ESR: https://hg.mozilla.org/releases/comm-esr60/rev/dec101dffad0

TB 63.0b2: https://hg.mozilla.org/releases/comm-beta/rev/90c267d6d48f