1509685 - Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | nsMsgDBView::UpdateDisplayMessage

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Wayne Mery (:wsmwk)]

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

Comment 1

[Jorg K (CEST = GMT+2)]

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.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1509685-bound-check.patch

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

Comment 3

[alta88]

(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 4

[alta88]

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.

Comment 5

[Jorg K (CEST = GMT+2)]

(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?

Comment 6

[alta88]

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.

Comment 7

[Pulsebot]

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

Comment 8

[Jorg K (CEST = GMT+2)]

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

Comment 9

[alta88]

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

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1509685-view-crashes-take2.patch

Like so?

Comment 11

[alta88]

Comment on attachment 9028435 [details] [diff] [review]
1509685-view-crashes-take2.patch

Review of attachment 9028435 [details] [diff] [review]:
-----------------------------------------------------------------

Like that.

Comment 12

[Pulsebot]

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

Comment 13

[:aceman]

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 ?

Comment 14

[Jorg K (CEST = GMT+2)]

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.

Comment 15

[Jorg K (CEST = GMT+2)]

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

Comment 16

[Jorg K (CEST = GMT+2)]

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

Comment 17

[Jorg K (CEST = GMT+2)]

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