#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.

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.

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

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

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

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.

includes 2 interim changes manually, but should apply.

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

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.

recent crash in 57, the 'fourth' of 4 is actually in msgSearchDBView.

a few more places that could use a check.

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).

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 ;-))

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

I've done something similar since it was easier to edit the patch ;-) https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=867fb0189d37556d1b6fd3418d8747f3bafa32ff

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?

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

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

Umm, I'm not the view person to ask here.

I think the only thing that can happen is an index of -1, so let's see if this harmless patch does it.

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.

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.

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.

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 ?

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.