+++ This bug was initially created as a clone of Bug #1385573 +++ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | nsMsgDBView::UpdateDisplayMessage is still a top crash for 52.9.1 and version 60. If Bug #1385573 helped, it wasn't very much. OTOH, no beta crashes (so far) after 60.0b11 like bp-6f29e6e3-63ad-4ef9-b324-6be050181022 - there were many 60.0b11 and 60.0b10 in september and october, and then nothing[1]. Is there a patch we landed on beta that didn't hit release? A common theme in crash reports is slowness before crash, and sorting columns. bp-8d3094b0-11e8-4af9-966d-4cc2b0180625 user got Unresponsive script, clicked continue, then crashed 0 mozglue.dll MOZ_CrashPrintf mfbt/Assertions.cpp:50 1 xul.dll InvalidArrayIndex_CRASH(unsigned int, unsigned int) xpcom/ds/nsTArray.cpp:26 2 xul.dll nsTArray_Impl<mozilla::dom::CanvasRenderingContext2DUserData*, nsTArrayInfallibleAllocator>::ElementAt(unsigned int) xpcom/ds/nsTArray.h:1041 3 xul.dll nsMsgDBView::UpdateDisplayMessage(unsigned int) comm/mailnews/base/src/nsMsgDBView.cpp:1179 4 xul.dll nsMsgDBView::LoadMessageByViewIndex(unsigned int) comm/mailnews/base/src/nsMsgDBView.cpp:1210 5 xul.dll nsMsgGroupView::LoadMessageByViewIndex(unsigned int) comm/mailnews/base/src/nsMsgGroupView.cpp:1056 6 xul.dll nsMsgDBView::SelectionChanged() comm/mailnews/base/src/nsMsgDBView.cpp:1297 7 xul.dll NS_InvokeByIndex xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_msvc.asm:54 * bp-bd77c82d-162d-4d50-b639-c973c0181110 user attempted to sort gmail All Mail folder by name * bp-88fa651b-5507-4b41-be50-a5b2d0181105 also sorting column InvalidArrayIndex_CRASH | nsMsgDBView::UpdateDisplayMessage crashes report same issue sorting bp-1c912a90-90af-4af1-889e-fd36a0180914 bp-0b27dd63-f869-42e5-8008-ae2350180825 claims to have just done a compact [1] beta https://crash-stats.mozilla.com/signature/?release_channel=%21release&signature=InvalidArrayIndex_CRASH%20%7C%20nsTArray_Impl%3CT%3E%3A%3AElementAt%20%7C%20nsMsgDBView%3A%3AUpdateDisplayMessage&date=%3E%3D2018-08-24T19%3A44%3A45.000Z&date=%3C2018-11-24T17%3A44%3A45.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1

I'm confused here: > ... nsMsgDBView::UpdateDisplayMessage is still a top crash for 52.9.1 and version 60. and > ... no beta crashes (so far) after 60.0b11 So are there crashes on TB 60.3.1 or not? Oh, I see, bp-bd77c82d-162d-4d50-b639-c973c0181110 *is* on 60.3. Stack: 3 nsMsgDBView::UpdateDisplayMessage(unsigned int) comm/mailnews/base/src/nsMsgDBView.cpp:1179 4 nsMsgDBView::LoadMessageByViewIndex(unsigned int) comm/mailnews/base/src/nsMsgDBView.cpp:1210 5 nsMsgGroupView::LoadMessageByViewIndex(unsigned int) comm/mailnews/base/src/nsMsgGroupView.cpp:1056 6 nsMsgDBView::SelectionChanged() comm/mailnews/base/src/nsMsgDBView.cpp:1297 Locally backing out rev 949fc4394c9f to make the line numbers match again, we get: 1179 rv = folder->SetLastMessageLoaded(m_keys[viewPosition]); From bug 1470049 comment #19: maybe testing m_flags and m_keys arrays specifically for oob right before their accesses would help. viewPosition was already checked with IsValidIndex which checks index < (nsMsgViewIndex) m_keys.Length() https://searchfox.org/comm-central/rev/efacd58fa63199a34709dc95b05bb9ab56d45f13/mailnews/base/src/nsMsgDBView.cpp#7871 so checking that again won't help, IMHO. If it crashed at line 1167: 1167 FetchSubject(msgHdr, m_flags[viewPosition], subject); we could add an oob check for m_flags (although there is supposedly a connection between those two arrays). Or could the arrays have changed after the check at the top of the function. OK, I'll do a patch.

Please let me know how to "shape" the patch. Drop the first part? Return NS_OK from the second?

(In reply to Jorg K (GMT+1) from comment #1) > I'm confused here: > > ... nsMsgDBView::UpdateDisplayMessage is still a top crash for 52.9.1 and version 60. > and > > ... no beta crashes (so far) after 60.0b11 > > So are there crashes on TB 60.3.1 or not? Oh, I see, > bp-bd77c82d-162d-4d50-b639-c973c0181110 *is* on 60.3. Stack: > 3 nsMsgDBView::UpdateDisplayMessage(unsigned int) > comm/mailnews/base/src/nsMsgDBView.cpp:1179 > 4 nsMsgDBView::LoadMessageByViewIndex(unsigned int) > comm/mailnews/base/src/nsMsgDBView.cpp:1210 > 5 nsMsgGroupView::LoadMessageByViewIndex(unsigned int) > comm/mailnews/base/src/nsMsgGroupView.cpp:1056 > 6 nsMsgDBView::SelectionChanged() > comm/mailnews/base/src/nsMsgDBView.cpp:1297 > > Locally backing out rev 949fc4394c9f to make the line numbers match again, > we get: > > 1179 rv = folder->SetLastMessageLoaded(m_keys[viewPosition]); > > From bug 1470049 comment #19: > maybe testing m_flags and m_keys arrays specifically for oob right before > their accesses would help. > > viewPosition was already checked with IsValidIndex which checks > index < (nsMsgViewIndex) m_keys.Length() > https://searchfox.org/comm-central/rev/ > efacd58fa63199a34709dc95b05bb9ab56d45f13/mailnews/base/src/nsMsgDBView. > cpp#7871 > > so checking that again won't help, IMHO. If it crashed at line 1167: > 1167 FetchSubject(msgHdr, m_flags[viewPosition], subject); > we could add an oob check for m_flags (although there is supposedly a > connection between those two arrays). > > Or could the arrays have changed after the check at the top of the function. > OK, I'll do a patch. that's what's so odd, how could index pass the initial check and then the array have changed in a few lines. none of the intervening callers does any array change as i recall. i hope we can trust the crash dump.

Comment on attachment 9027835 [details] [diff] [review] 1509685-bound-check.patch i think it would be cleaner to just use the IsValidIndex() function in those places. but up to you. my preference is for a line after the return.

(In reply to alta88 from comment #4) > i think it would be cleaner to just use the IsValidIndex() function in those > places. but up to you. That only checks the m_keys array. Are m_keys and m_flags guaranteed to have the same length?

when i looked at this in detail back then, every place m_keys was altered the next lines were m_flags and m_levels for view indentation and m_folders for multifolder searches, so i didn't see then how the arrays could get out of sync in length. and it had to be this way originally to ever work. yes, the m_flags check isn't in the function so needs to be explicit.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/35f0ac2f0816 Add more bounds checking in nsMsgDBView::UpdateDisplayMessage() to avoid crashes. r=alta88

TB 60.3.2 ESR: https://hg.mozilla.org/releases/comm-esr60/rev/189364307b2e821567aef43c0faded255f4bf4d8

there is also a crash in the array access in nsMsgDBView::LoadMessageByViewIndex(), which is reached by a different code path; it deserves the same treatment. https://crash-stats.mozilla.com/report/index/b3198174-8f35-4667-a82e-c3b020181127

Like so?

Comment on attachment 9028435 [details] [diff] [review] 1509685-view-crashes-take2.patch Review of attachment 9028435 [details] [diff] [review]: ----------------------------------------------------------------- Like that.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/8249b1aa65ab Add more bounds checking in nsMsgDBView::UpdateDisplayMessage() to avoid crashes, take 2. r=alta88

Comment on attachment 9028435 [details] [diff] [review] 1509685-view-crashes-take2.patch Review of attachment 9028435 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/src/nsMsgDBView.cpp @@ +1215,5 @@ > nsCOMPtr<nsIMessenger> messenger (do_QueryReferent(mMessengerWeak)); > NS_ENSURE_TRUE(messenger, NS_ERROR_FAILURE); > messenger->OpenURL(uri); > + if (aViewIndex >= (nsMsgViewIndex)m_keys.Length()) > + return NS_MSG_INVALID_DBVIEW_INDEX; Why is the check placed here and not sooner? Does the messenger->OpenURL(uri) call affect the viewIndex or the m_keys ?

It's a bit of paranoia :-( if (!IsValidIndex(aViewIndex)) return NS_MSG_INVALID_DBVIEW_INDEX; at the beginning already checks |index < (nsMsgViewIndex) m_keys.Length()| so we don't quite understand why it can crash here. Same goes for https://hg.mozilla.org/comm-central/rev/35f0ac2f0816 where the index is also already checked at the beginning of the function. Note that Alta88 checked that all the arrays m_keys, m_flags and m_levels should always have the same length, see comment #6. Maybe through some side effect the arrays get changed.

TB 60.4.0 ESR (missed TB 60.3.2 ESR): https://hg.mozilla.org/releases/comm-esr60/rev/7a117f25d40ecfb0bd352775eada66b6ffd6938c

Beta (TB 64 beta 4): https://hg.mozilla.org/releases/comm-beta/rev/21dec7eb5de781d821a0cd1869641ed724a1020c https://hg.mozilla.org/releases/comm-beta/rev/b2a3f7b11639f5b4ea32f7f9d7b12ebd15e18e92