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

RESOLVED FIXED in Thunderbird 58.0

Status

defect
--
critical
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: wsmwk, Assigned: alta88)

Tracking

({crash})

Thunderbird 58.0
x86
Windows 7
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr52? affected)

Details

(crash signature)

Attachments

(5 attachments, 6 obsolete attachments)

#43 crash for 52.2.1. Most users crash only once.

report bp-f530cda7-0418-47d2-9c56-e20f50170717 "Could not reach inbox after sending email."  User has  no extensions other than calendar

0 	mozglue.dll	MOZ_CrashPrintf	mfbt/Assertions.cpp:50
1 	xul.dll	InvalidArrayIndex_CRASH(unsigned int, unsigned int)	xpcom/glue/nsTArray.cpp:26
2 	xul.dll	nsTArray_Impl<RefPtr<nsNntpMockChannel>, nsTArrayInfallibleAllocator>::ElementAt(unsigned int)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/objdir-tb/dist/include/nsTArray.h:1033
3 	xul.dll	nsMsgDBView::UpdateDisplayMessage(unsigned int)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/mailnews/base/src/nsMsgDBView.cpp:1105
4 	xul.dll	nsMsgDBView::LoadMessageByViewIndex(unsigned int)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/mailnews/base/src/nsMsgDBView.cpp:1135
5 	xul.dll	nsMsgGroupView::LoadMessageByViewIndex(unsigned int)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/mailnews/base/src/nsMsgGroupView.cpp:970
6 	xul.dll	nsMsgDBView::SelectionChanged()	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/mailnews/base/src/nsMsgDBView.cpp:1212
7 	xul.dll	NS_InvokeByIndex	xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_msvc.asm:54
8 	xul.dll	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)	js/xpconnect/src/XPCWrappedNative.cpp:1344
9 	xul.dll	XPCNativeSet::FindMember(jsid, XPCNativeMember**, unsigned short*)	js/xpconnect/src/XPCInlines.h:316
10 	xul.dll	XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:999
11 	xul.dll	XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:999
12 	xul.dll	XPCWrappedNative_Trace(JSTracer*, JSObject*)	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:598
13 	xul.dll	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)	js/src/vm/Interpreter.cpp:459
14 	xul.dll	InternalCall	js/src/vm/Interpreter.cpp:504
15 	xul.dll	Interpret	js/src/vm/Interpreter.cpp:2922
16 	xul.dll	Interpret	js/src/vm/Interpreter.cpp:2922
17 	xul.dll	RefPtr<mozilla::dom::DragEvent>::assign_with_AddRef(mozilla::dom::DragEvent*)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/objdir-tb/dist/include/mozilla/RefPtr.h:54
18 	xul.dll	XPCWrappedNative::InitTearOff(XPCWrappedNativeTearOff*, XPCNativeInterface*, bool)	js/xpconnect/src/XPCWrappedNative.cpp:1216
19 	xul.dll	PLDHashTable::SearchTable<1>(void const*, unsigned int)	xpcom/glue/PLDHashTable.cpp:362
20 	xul.dll	js::BarrierMethods<JSObject*>::exposeToJS(JSObject*)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/objdir-tb/dist/include/js/RootingAPI.h:618 

linux example bp-f9788a40-aae7-4d33-97c2-fec470170729

bp-cd7a7458-ccf2-40e2-8540-59d480170722 "Continuing notifications of "Chrome://messaging/content/tabrail.xml/1407" and other ending code numbers"
Oldest build I find crashing is 50.0b3 20161027202105 bp-bf6c0431-4acb-4d76-8a88-40a8f2170413

Interesting that there are no 45.x crashes
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #1)
> Oldest build I find crashing is 50.0b3 20161027202105
> bp-bf6c0431-4acb-4d76-8a88-40a8f2170413
> 
> Interesting that there are no 45.x crashes

This check is by bug 1159244 that is Gecko 50.
Posted patch dbview-whitespace.patch (obsolete) — Splinter Review
Attachment #8911293 - Flags: review?(jorgk)
Posted patch msgDBView-whitespace.patch (obsolete) — Splinter Review
There are 4 places that crash in msgDBView due to invalid index. But first, all the *view code needs to be readable.. Note that braces around single lines in for/while cases are intentional if the for/while is multiline and difficult to read. A strict adherence would always use braces, but imo that actually reduces readability.
Attachment #8911295 - Flags: review?(jorgk)
Comment on attachment 8911293 [details] [diff] [review]
dbview-whitespace.patch

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

This is a 84KB patch, I think I'll save the 344BK one for another day ;-(

::: mailnews/base/src/nsMsgThreadedDBView.cpp
@@ +118,5 @@
>    m_prevFlags.Clear();
>    m_prevLevels.Clear();
>    m_havePrevView = false;
> +  nsresult getSortrv = NS_OK;
> +  // ### TODO m_db->GetSortInfo(&sortType, &sortOrder);

s/###/XXX/.

@@ +710,2 @@
>      return NS_OK;
> +  }

No need for braces here, but you think the condition is to long that one misses the block.

::: mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp
@@ +166,5 @@
>    NS_ENSURE_ARG_POINTER(aStatus);
>    NS_ENSURE_ARG_POINTER(aHdrChanged);
>  
>    nsMsgViewIndex index = FindHdr(aHdrChanged);
> +  // Message does not appear in view>

> ?
Attachment #8911293 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+2) from comment #5)
> Comment on attachment 8911293 [details] [diff] [review]
> dbview-whitespace.patch
> 
> Review of attachment 8911293 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a 84KB patch, I think I'll save the 344BK one for another day ;-(
> 

No rush; I appreciate it and also that this is more painful to review than do..

> ::: mailnews/base/src/nsMsgThreadedDBView.cpp
> @@ +118,5 @@
> >    m_prevFlags.Clear();
> >    m_prevLevels.Clear();
> >    m_havePrevView = false;
> > +  nsresult getSortrv = NS_OK;
> > +  // ### TODO m_db->GetSortInfo(&sortType, &sortOrder);
> 
> s/###/XXX/.
> 
done.

> @@ +710,2 @@
> >      return NS_OK;
> > +  }
> 
> No need for braces here, but you think the condition is to long that one
> misses the block.
> 

if it were without a ! it would work; i tried to like it without braces but couldn't get there..  eyecatchers are important.

> ::: mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp
> @@ +166,5 @@
> >    NS_ENSURE_ARG_POINTER(aStatus);
> >    NS_ENSURE_ARG_POINTER(aHdrChanged);
> >  
> >    nsMsgViewIndex index = FindHdr(aHdrChanged);
> > +  // Message does not appear in view>
> 
> > ?

fixed.
Attachment #8911293 - Attachment is obsolete: true
Attachment #8911325 - Flags: review+
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/16c7195a1573
whitespace only changes for nsMsgThreadedDBView.cpp, nsMsgXFViewThread.cpp, nsMsgXFVirtualFolderDBView.cpp. r=jorgk
Attachment #8911325 - Attachment description: dbview-whitespace.patch → dbview-whitespace.patch [landed: comment #8]
Posted patch msgDBView-whitespace.patch (obsolete) — Splinter Review
I'm doing some string changes in bug 1402750, so this patch applies when they land.
Comment on attachment 8912017 [details] [diff] [review]
msgDBView-whitespace.patch

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

OK, I've looked through the 10.000+ line patch and found more room for improvement, all nits of course. It took me 1h 35min. I hope that's what you were after.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +219,4 @@
>    }
>  
>    if (mMessengerStringBundle)
> +    res = mMessengerStringBundle->GetStringFromName(NS_ConvertUTF16toUTF8(aStringName).get(),

Nit: That line is still too long after your change.

@@ +414,5 @@
>    nsCString emailAddress;
>    nsString name;
> +  ExtractFirstAddress(EncodedHeader(author, headerCharset.get()),
> +                      name,
> +                      emailAddress);

Nit: Inconsistent. Either one line per argument or the last two can go onto the same line.

@@ +442,5 @@
>    nsresult rv = aHdr->GetAccountKey(getter_Copies(accountKey));
>  
>    // Cache the account manager?
> +  nsCOMPtr<nsIMsgAccountManager>
> +    accountManager(do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv));

The original actually followed the standard. I'm not so keen on having the type with nothing on the first line.

@@ +491,3 @@
>      GetCachedName(recipients, currentDisplayNameVersion, cachedRecipients);
>  
> +    // Recipients have already been cached,check if the addressbook

Nit: Space after , - missing full stop.

@@ +508,5 @@
>    nsTArray<nsString> names;
>    nsTArray<nsCString> emails;
>    ExtractAllAddresses(EncodedHeader(unparsedRecipients, headerCharset.get()),
> +                      names,
> +                      UTF16ArrayAdapter<>(emails));

Nit: Inconsistent, as above.

@@ +539,5 @@
>        else
>          CopyUTF8toUTF16(curAddress, recipient);
>      }
>  
> +    // Add ', ' between each recipient

Nit: Full stop.

@@ +1348,5 @@
> +  else if (!mSuppressCommandUpdating &&
> +           mCommandUpdater &&
> +           (!mRemovingRow || GetSize() == 0))
> +  {
> +    // o.t. push an updated.

What does this say?

@@ +2004,5 @@
> +/**
> + * CUSTOM COLUMNS.
> + */
> +
> +// Add a custom column handler

Nit: Full stop.

@@ +2175,5 @@
>  
>    aValue.Truncate();
>  
> +  // Attempt to retreive a custom column handler. If it exists call it and
> +  // return

Full stop.

@@ +2373,5 @@
> +        ExpandAndSelectThreadByIndex(row, false);
> +      }
> +      else if (colID[1] == 'a')
> +      {
> +        // ### Do we want to keep this behaviour but switch it to tags?

XXX

@@ +2455,5 @@
>    m_sortType = sortType;
>  
>    nsresult rv;
> +  nsCOMPtr<nsIMsgAccountManager>
> +    accountManager = do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv);

The original actually followed the standard. I'm not so keen on having the type with nothing on the first line.

@@ +2982,5 @@
>    if (NS_FAILED(rv))
> +    return false;
> +
> +  // Filter after the fact is implement using search so if you can't search,
> +  // you can't filter after the fact

Full stop.

@@ +3228,5 @@
> +                                   messageArray,
> +                                   destFolder,
> +                                   isMove,
> +                                   nullptr /* listener */,
> +                                   window, true /*allowUndo*/);

/* allow Undo */

@@ +3301,3 @@
>      // all messages in the view are from the same folder (no
>      // more junk status column in the 'search messages' dialog
>      // like in earlier versions...)

Full stop.

@@ +3633,5 @@
>    if (m_deletingRows)
>      mIndicesToNoteChange.AppendElements(indices, numIndices);
>  
> +  rv = m_folder->DeleteMessages(messageArray, window, deleteStorage,
> +                                false, nullptr, true /*allow Undo*/);

/* allow Undo */

@@ +3839,3 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    // This routine is only reached if the user someone touched the UI

Full stop.

@@ +3849,1 @@
>  

Full stop, lose empty line.

@@ +4110,5 @@
>    int32_t manualMarkMode;
>    (void)spamSettings->GetManualMarkMode(&manualMarkMode);
> +  NS_ASSERTION(manualMarkMode == nsISpamSettings::MANUAL_MARK_MODE_MOVE ||
> +               manualMarkMode == nsISpamSettings::MANUAL_MARK_MODE_DELETE,
> +              "bad manual mark mode");

shift one space right.

@@ +4333,2 @@
>  const int kMaxSubjectKey = 160;
> +// Also used for account.

Hmm. Inline comments are permissible like in this case. Moving it to the line above is no benefit here.

@@ +4640,1 @@
>          stringPtr++;

Move comment above the if.

@@ +5090,2 @@
>    SaveSortInfo(sortType, sortOrder);
> +  // Figure out how much memory we'll need, and the malloc it.

then

@@ +5114,5 @@
>    nsCOMArray<nsIMsgFolder> *folders = GetFolders();
>  
>    IdKey** pPtrBase = (IdKey**)PR_Malloc(arraySize * sizeof(IdKey*));
>    NS_ASSERTION(pPtrBase, "out of memory, can't sort");
>    if (!pPtrBase) return NS_ERROR_OUT_OF_MEMORY;

New line.

@@ +5385,3 @@
>      {
>        msgIndex = GetIndexOfFirstDisplayedKeyInThread(threadHdr, true);
> +      //nsMsgKey threadKey = (msgIndex == nsMsgViewIndex_None) ? nsMsgKey_None :

Polishing removed code. OK, space missing.

@@ +5493,3 @@
>    else if (!allowDummy && m_flags[viewIndex] & MSG_VIEW_FLAG_DUMMY)
> +  {
> +    // Else if we're not allowing dummies, and we found a dummy, look again

lose "Else if". We're in the else if block.

@@ +5574,5 @@
>  
>    return numInThread;
>  }
>  
> +// Returns the number of lines that would be added (> 0) or removed (< 0)

full stop.

@@ +5699,5 @@
>        NS_ENSURE_SUCCESS(rv,rv);
>      }
>  
> +    // Get the number of messages in the expanded thread so we know how many
> +    // to select

full stop.

@@ +6182,5 @@
>    msgHdr->GetThreadId(&threadId);
>    msgHdr->GetThreadParent(&threadParent);
>  
>    msgHdr->GetFlags(&flags);
> +  // ### this isn't quite right, is it?

XXX

@@ +6427,4 @@
>    if (!InsertEmptyRows(viewIndex, numChildren))
>      return NS_ERROR_OUT_OF_MEMORY;
>  
>    // ### need to rework this when we implemented threading in group views.

XXX

@@ +6475,5 @@
>          AdjustReadFlag(msgHdr, &msgFlags);
>          SetMsgHdrAt(msgHdr, viewIndex, msgKey, msgFlags & ~MSG_VIEW_FLAGS, 1);
> +        // Here, we're either flat, or we're grouped - in either case,
> +        // level is 1. Turn off thread or elided bit if they got turned on
> +        // (maybe from new only view?)

Full stop.

@@ +6502,1 @@
>    // ### fix for cross folder view case.

XXX Fix ...

@@ +6526,5 @@
>        break;
>  
> +    // Scan up to find view index of ancestor, if any.
> +    for (nsMsgViewIndex indexToTry = viewIndex;
> +         indexToTry && indexToTry-- >= startOfThread;)

Maybe a space after the ;

@@ +6552,4 @@
>    return 1;
>  }
>  
>  // ### Can this be combined with GetIndexForThread??

XXX

@@ +6897,5 @@
>      {
> +      uint32_t viewOnlyFlags =
> +        m_flags[index] & (MSG_VIEW_FLAGS | nsMsgMessageFlags::Elided);
> +
> +      // ### what about saving the old view only flags, like IsThread and

XXX

@@ +7272,5 @@
> +    if (isRead != bRead)
> +    {
> +      // MarkHdrRead will change the unread count on the thread.
> +      db->MarkHdrRead(msgHdr, bRead, nullptr);
> +      // insert at the front.  should we insert at the end?

Insert, lose space, Should.

@@ +7594,5 @@
> +        curFolder->GetURI(viewFolderUri);
> +
> +      int32_t relPos = (motion == nsMsgNavigationType::forward) ?
> +                         1 : (m_currentlyDisplayedMsgKey != nsMsgKey_None) ?
> +                         -1 : 0;

Move -1 : 0 to the previous line or better
1 :
(m_currentlyDisplayedMsgKey != -1 : 0;

@@ +7597,5 @@
> +                         1 : (m_currentlyDisplayedMsgKey != nsMsgKey_None) ?
> +                         -1 : 0;
> +      int32_t curPos;
> +      nsresult rv;
> +      nsCOMPtr<nsIMessenger> messenger (do_QueryReferent(mMessengerWeak, &rv));

lose space after messenger.

@@ +7604,5 @@
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      // Empty viewFolderUri means we're in a search results view.
> +      if (viewFolderUri.IsEmpty() || folderUri.Equals(viewFolderUri))
> +      {
> +        nsCOMPtr <nsIMsgDBHdr> msgHdr;

lose space.

@@ +7647,5 @@
> +    (void) mTreeSelection->GetCurrentIndex(&index);
> +  else
> +    index = FindViewIndex(m_currentlyDisplayedMsgKey);
> +
> +  nsCOMPtr<nsIMessenger> messenger (do_QueryReferent(mMessengerWeak));

no space after messenger.

@@ +7802,5 @@
> +        flags & nsMsgMessageFlags::Elided)
> +    {
> +      NS_ERROR("fix this");
> +      //nsMsgKey threadId = m_keys[curIndex];
> +      //rv = m_db->GetUnreadKeyInThread(threadId, pResultKey, resultThreadId);

More dead code to polish here.

@@ +8475,5 @@
>  
>    int32_t startRange;
>    int32_t endRange;
>    nsresult rv = mTreeSelection->GetRangeAt(0, &startRange, &endRange);
> +  // Don't assert, it is legal for nothing to be selected

Full stop.

@@ +8501,5 @@
>  
>    int32_t startRange;
>    int32_t endRange;
>    nsresult rv = mTreeSelection->GetRangeAt(0, &startRange, &endRange);
> +  // Don't assert, it is legal for nothing to be selected

Full stop.
Comment on attachment 8911295 [details] [diff] [review]
msgDBView-whitespace.patch

I've done the review on the other patch. Please fix the nits and I'll put r+ onto it.
Attachment #8911295 - Flags: review?(jorgk)
includes 2 interim changes manually, but should apply.
Attachment #8911295 - Attachment is obsolete: true
Attachment #8912017 - Attachment is obsolete: true
Attachment #8912485 - Flags: review?(jorgk)
> @@ +414,5 @@
> >    nsCString emailAddress;
> >    nsString name;
> > +  ExtractFirstAddress(EncodedHeader(author, headerCharset.get()),
> > +                      name,
> > +                      emailAddress);
> 
> Nit: Inconsistent. Either one line per argument or the last two can go onto
> the same line.
> 

you'll notice there are 3 args, the first is a func with 2 args.

all else has been done.
Comment on attachment 8912485 [details] [diff] [review]
msgDBView-whitespace.patch [landed comment #19]

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

Sorry for requesting full stops where there shouldn't have been any and for complaining about the correct ExtractFirstAddress() call. Would it be possible to clarify the comment below since I really don't get it.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +1348,5 @@
> +  else if (!mSuppressCommandUpdating &&
> +           mCommandUpdater &&
> +           (!mRemovingRow || GetSize() == 0))
> +  {
> +    // o.t. push an updated.

I'm still wondering what this says.
Attachment #8912485 - Flags: review?(jorgk) → review+
it's a puzzle. i can't even hazard a wild guess.
Well, blame will say that you added that line ;-)
How about:
  // OK to push an update
or
  // OK, push an update.
or
  // Push an update.
  mCommandUpdater->UpdateCommandStatus();

IMHO a comment that no one understands might as well go since is seems to describe what the next line does.
it should be removed as it adds no value.  it's possible o.t. is an archaic (early typewriter era) shortcut for 'otherwise'.
I'll take care of it when landing. Right now we're busted.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0cf8957d505f
whitespace only changes for nsMsgDBView.cpp. r=jorgk DONTBUILD
Comment on attachment 8912485 [details] [diff] [review]
msgDBView-whitespace.patch [landed comment #19]

I landed that with the "o.t." comment removed to get it off the books.
So we're ready for the meat here.
Attachment #8912485 - Attachment description: msgDBView-whitespace.patch → msgDBView-whitespace.patch [landed comment #19]
Assignee: nobody → alta88
Attachment #8913211 - Flags: review?(jorgk)
recent crash in 57, the 'fourth' of 4 is actually in msgSearchDBView.
Attachment #8913252 - Flags: review?(jorgk)
a few more places that could use a check.
Attachment #8913252 - Attachment is obsolete: true
Attachment #8913252 - Flags: review?(jorgk)
Attachment #8913271 - Flags: review?(jorgk)
Comment on attachment 8913211 [details] [diff] [review]
msgDBView-invalidIndex.patch

This looks reasonable. The patch looks bigger than it is due to some refactoring.

Sorry about the delay, but other fires were burning much hotter (and still are).
Attachment #8913211 - Flags: review?(jorgk) → review+
Comment on attachment 8913271 [details] [diff] [review]
nsMsgSearchDBView-InvalidIndex.patch

This equally looks reasonable, sorry again about the delay.

I don't see changed behaviour here, but I'll do a try run anyway since according to you, the view code is rather brittle.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9753eb05b731bc2ee38835051d464cc936224891

Moving on to bug 519687 next? (Not intended to exert pressure ;-))
Attachment #8913271 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+2) from comment #25)
> Comment on attachment 8913271 [details] [diff] [review]
> nsMsgSearchDBView-InvalidIndex.patch
> 
> This equally looks reasonable, sorry again about the delay.
> 
> I don't see changed behaviour here, but I'll do a try run anyway since
> according to you, the view code is rather brittle.
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=9753eb05b731bc2ee38835051d464cc936224891

There are some new seeming failures in virtual folder, both x and z.  Yes, brittle is the definitive word with view code. Could you do a try without the nsMsgSearchDBView-InvalidIndex.patch..

> 
> Moving on to bug 519687 next? (Not intended to exert pressure ;-))

As with everything in view code, it became more complex. I'll return to it eventually, it's not insurmountable.
(In reply to alta88 from comment #26)
> Yes, brittle is the definitive word with view code.
You coined that ;-)

> Could you do a try without the nsMsgSearchDBView-InvalidIndex.patch..
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=58d9953634651332d91cd8caa3e79df1d7df8e18

Looking at the patches again, there are slight changes in behaviour, for example, nsMsgDBView::UpdateDisplayMessage() before always returned OK, now it can return NS_MSG_INVALID_DBVIEW_INDEX.
Right. If you have time (I won't get to it today), please update the patch to:

+  if (!mCommandUpdater || !IsValidIndex(viewPosition))
+    return NS_OK;
+

in UpdateDisplayMessage() and try. Also, IsValidIndex() tests m_keys length, while in one or two places the test is for m_levels. But the entire design mandates those two indices always match the row and the message, so that should not be an issue.
The first patch alone worked without that modification:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=58d9953634651332d91cd8caa3e79df1d7df8e18

So the fault appears to be in the second one.
Let's forget the second patch, it was an attempt at being proactive and the crash there was not in release but in 57. So please land the original msgDBView-invalidIndex.patch. None of the callers, as I recall, of UpdateDisplayMessage() used the rv and it's better to return the real error anyway. Thanks.
OK, will do.

As for the second patch which also contained some formatting improvements, I think you changed the behaviour here:
       viewThread->AddHdr(msgHdr, false, posInThread, getter_AddRefs(parent));
       nsMsgViewIndex insertIndex = GetIndexForThread(msgHdr);
+      if (!IsValidIndex(insertIndex))
+        return NS_MSG_INVALID_DBVIEW_INDEX;
+
       NS_ASSERTION(insertIndex == m_levels.Length() || !m_levels[insertIndex],
                    "inserting into middle of thread");
-      if (insertIndex == nsMsgViewIndex_None)
-        return NS_ERROR_FAILURE;

GetIndexForThread() can return nsMsgViewIndex highIndex = m_keys.Length();

So let me try something else here, I'll attach a patch in the next comment.
How about like this?
Attachment #8913271 - Attachment is obsolete: true
Attachment #8915185 - Flags: feedback?(alta88)
Ah yes, in this case the out of bounds index value is ok, it just can't be used in a direct access in the assert test. So if try is ok, f+.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8b61c0301de7
Part 1: Fix crash in nsMsgDBView::UpdateDisplayMessage. r=jorgk
https://hg.mozilla.org/comm-central/rev/40f5ba35583e
Part 2: Fix crash in nsMsgSearchDBView.cpp. r=jorgk
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 58.0
Attachment #8915185 - Flags: feedback?(alta88) → feedback+
There was some inconsistent indenting introduced in this patch: https://hg.mozilla.org/comm-central/rev/0cf8957d505f#l1.4009
Thanks, I'll get that fixed.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8cc636483513
Follow-up: Fix indentation. rs=white-space-only
Welcome back.  I have a mixed report - it's hard to judge the results in part because of the loss of crash-stats prior to dec 25, and in part because we have no record in this bug report of the version 57 crash rate.

No crashes reported for 57, 58.0b1 and 58.0b2 since dec 24 [1]. But we see 58.0b3 crashes which has the patches.  Four examples:
bp-d1cd2d52-da0c-4e62-b4da-54cba0180210
bp-50e56b80-0642-49a4-afb2-165bd0180205
bp-6038ca61-3579-4208-93f4-763600180203
bp-674dfae8-4938-42a4-bee3-7f2880180129

So it's unclear to me whether the patches should be uplifted to esr, where the signature ranks #30 for 52.6.0.

[1] https://crash-stats.mozilla.com/search/?signature=%3DInvalidArrayIndex_CRASH%20%7C%20nsTArray_Impl%3CT%3E%3A%3AElementAt%20%7C%20nsMsgDBView%3A%3AUpdateDisplayMessage&release_channel=beta&product=Thunderbird&date=%3E%3D2017-11-10T21%3A20%3A15.000Z&date=%3C2018-02-10T21%3A20%3A15.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports
I reviewed crashes of last 7 months. There are still some crashes of 60, 59 and 58 beta**, so a follow up patch may be needed? 

With no crash history prior to December it is not possible to say whether crash rate went down, but no regressive behavior reported.  #26 crash for 52.8.0 so I think the patch is simple enough and important enough we should take this on next 52.x

**bp-c28560dc-ca7c-46e2-9c8e-d29790180627 60.0b8
 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::HTMLImageElement*, nsTArrayInfallibleAllocator>::ElementAt(unsigned int)	xpcom/ds/nsTArray.h:1029
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
Flags: needinfo?(vseerror)
Flags: needinfo?(vseerror) → needinfo?(jorgk)
Umm, I'm not the view person to ask here.
Flags: needinfo?(jorgk) → needinfo?(alta88)
Posted patch index.patch (obsolete) — Splinter Review
I think the only thing that can happen is an index of -1, so let's see if this harmless patch does it.
Flags: needinfo?(alta88)
Attachment #8995821 - Flags: review?(jorgk)
Although nsMsgViewIndex is a uint_32t so it shouldn't happen. I don't how else it could be possible to be out of bounds in the m_keys array after the check.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Thanks for the patch, but it really becomes unmanageable to reopen a bug that was resolved about *one* year ago in TB 58.

As for the patch:
|index >= 0| for |typedef unsigned long nsMsgViewIndex;| should be a no-op in any self-respecting compiler, don't you think? We can get a second opinion on this but I don't think it will change anything.

More likely we start with |const nsMsgViewIndex nsMsgViewIndex_None = 0xFFFFFFFF;| and decrease it? Would that be possible? Than we're at 0xFFFFFFFE and indexing with that will certainly be out of range.

BTW, 0xFFFFFFFF *is* -1.
Status: REOPENED → RESOLVED
Closed: 2 years agoLast year
Resolution: --- → FIXED
Comment on attachment 8995821 [details] [diff] [review]
index.patch

As I said in the previous comment, this is a no-op and won't help. The problem must be elsewhere.

Looking at
https://crash-stats.mozilla.com/report/index/c28560dc-ca7c-46e2-9c8e-d29790180627
it crashes at comm/mailnews/base/src/nsMsgDBView.cpp:1179, so that's:
  rv = folder->SetLastMessageLoaded(m_keys[viewPosition]);

In the very same function it already executed
  FetchSubject(msgHdr, m_flags[viewPosition], subject);
without crashing. So the index must be good.

Before it did a
  rv = GetMsgHdrForViewIndex(viewPosition, getter_AddRefs(msgHdr));
  NS_ENSURE_SUCCESS(rv,rv);
so would that have succeeded with a bad index?

What else could be wrong? Let's find it in a *new* bug.
Attachment #8995821 - Flags: review?(jorgk) → feedback?(acelists)
Comment on attachment 8995821 [details] [diff] [review]
index.patch

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

::: mailnews/base/src/nsMsgDBView.cpp
@@ +7872,5 @@
>  bool
>  nsMsgDBView::IsValidIndex(nsMsgViewIndex index)
>  {
>    return index != nsMsgViewIndex_None &&
> +         index >= 0 &&

So when nsMsgViewIndex is 'unsigned long' why is this check useful? I know you try to rule out index == -1 but will that -1 value be considered not >=0 when in an unsigned variable? Will it not wrap to something like 0xFFFFFFFFFE and be also >=0 ?
Attachment #8995821 - Flags: feedback?(acelists)
Comment on attachment 8995821 [details] [diff] [review]
index.patch

ok ok, comment 44 implied this was nonsensical.  

unless the m_keys array is changed in the few instructions after IsValidIndex(), which tests for m_keys length, this crash makes no sense. the m_flags array should always be the same length but it's not strict proof that FetchSubject() works. it does seem to only happen in grouped view, on windows.
Attachment #8995821 - Attachment is obsolete: true
Blocks: 1509685
Depends on: 1470049
You need to log in before you can comment on or make changes to this bug.