Closed Bug 1354809 Opened 3 years ago Closed 2 years ago

Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | nsMsgGroupView::LoadMessageByViewIndex

Categories

(Thunderbird :: General, defect, critical)

52 Branch
x86
Windows 10
defect
Not set
critical

Tracking

(thunderbird_esr5257+ fixed, thunderbird56 wontfix, thunderbird57 fixed, thunderbird58 fixed)

VERIFIED FIXED
Thunderbird 58.0
Tracking Status
thunderbird_esr52 57+ fixed
thunderbird56 --- wontfix
thunderbird57 --- fixed
thunderbird58 --- fixed

People

(Reporter: wsmwk, Assigned: alta88)

References

Details

(Keywords: crash, topcrash-thunderbird, Whiteboard: TB 57 beta 1 => TB 52.5 ESR)

Crash Data

Attachments

(2 files, 3 obsolete files)

#26 crash for Thunderbird 52.0.  First seen on 2016-10-14 according to https://crash-stats.mozilla.com/topcrashers/?product=Thunderbird&version=52.0&days=3

This bug was filed from the Socorro interface and is 
report bp-a8fab8b5-b0fd-4dfb-8360-0c2b82170310.
"Contnuees to fail/crash when using an email I had searched for to re-send or reply again. When composing the email, Thunderbird crashes. Happens frequently."

 0 	xul.dll	InvalidArrayIndex_CRASH(unsigned int, unsigned int)	xpcom/glue/nsTArray.cpp:35
1 	xul.dll	nsTArray_Impl<mozilla::NotNull<RefPtr<nsThread> >, nsTArrayInfallibleAllocator>::ElementAt(unsigned int)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/objdir-tb/dist/include/nsTArray.h:1033
2 	xul.dll	nsMsgGroupView::LoadMessageByViewIndex(unsigned int)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/base/src/nsMsgGroupView.cpp:958
3 	xul.dll	NS_InvokeByIndex	xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_msvc.asm:54
4 	xul.dll	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)	js/xpconnect/src/XPCWrappedNative.cpp:1344
5 	xul.dll	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)	js/src/vm/Interpreter.cpp:477
6 	xul.dll	XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:999
7 	xul.dll	XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:999
8 	xul.dll	XPCWrappedNative_Trace(JSTracer*, JSObject*)	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:598
9 	xul.dll	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)	js/src/vm/Interpreter.cpp:459
10 	xul.dll	InternalCall	js/src/vm/Interpreter.cpp:504
11 	xul.dll	Interpret	js/src/vm/Interpreter.cpp:2922
12 	xul.dll	Interpret	js/src/vm/Interpreter.cpp:2922
13 	xul.dll	PLDHashTable::SearchTable<1>(void const*, unsigned int)	xpcom/glue/PLDHashTable.cpp:362
14 	xul.dll	nsTArray_base<nsTArrayFallibleAllocator, nsTArray_CopyWithMemutils>::ShrinkCapacity(unsigned int, unsigned int)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/objdir-tb/dist/include/nsTArray-inl.h:206
15 	xul.dll	mozilla::RestyleManagerBase::ProcessRestyledFrames(nsStyleChangeList&)	layout/base/RestyleManagerBase.cpp:1374
16 	xul.dll	nsTArray_base<nsTArrayFallibleAllocator, nsTArray_CopyWithMemutils>::ShrinkCapacity(unsigned int, unsigned int)	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/objdir-tb/dist/include/nsTArray-inl.h:206
17 	xul.dll	nsTArray_Impl<RefPtr<mozilla::dom::MediaStreamTrackConsumer>, nsTArrayInfallibleAllocator>::~nsTArray_Impl<RefPtr<mozilla::dom::MediaStreamTrackConsumer>, nsTArrayInfallibleAllocator>()	C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/objdir-tb/dist/include/nsTArray.h:872
18 	xul.dll	mozilla::RestyleTracker::DoProcessRestyles()	layout/base/RestyleTracker.cpp:399
19 	xul.dll	mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*)	dom/events/EventListenerManager.cpp:1287
This has gained new life in 52.1.1 - #29 crash.  #20 for beta
Attached patch nsMsgGroupView-whitespace.patch (obsolete) — Splinter Review
whitespace only: comments, arg alignment, formatting.
Attachment #8910321 - Flags: review?(jorgk)
Comment on attachment 8910321 [details] [diff] [review]
nsMsgGroupView-whitespace.patch

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

Nice clean-up, but please consider addressing a few more spots I pointed out below.

::: mailnews/base/src/nsMsgGroupView.cpp
@@ +376,2 @@
>        foundThread->m_threadKey = (nsMsgKey)
> +      PL_HashString(NS_LossyConvertUTF16toASCII(hashKey).get());

nit: looks like this should be indented. I'd prefer the cast in front, so:
foundThread->m_threadKey =
  (nsMsgKey)...
You think adding braces makes this more legible?

@@ +512,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsMsgGroupView::CopyDBView(nsMsgDBView *aNewMsgDBView,
> +                           nsIMessenger *aMessengerInstance, 

nit: trailing space.

@@ +632,5 @@
>            if (insertedAtThreadRoot)
>              msgIndexInThread++;
>            // If this header is the new parent of the thread... AND
>            // If we are not using a dummy row, this means we need to append our
>            //  old node as the first child of the new root.

nit: extra space left here.

@@ +640,5 @@
>            //  "content" node down.)
>            // Example mini-diagrams, wrapping the to-add thing with ()
>            //  No dummy row; we had: [A], now we have [B], we want [B (A)].
>            //  Dummy row; we had: [A A], now we have [B A], we want [B (B) A].
>            //  (Coming into this we're adding 'B')

nit: Not sure why there are double spaces.

@@ +648,4 @@
>              //  insert it. (offset msgIndexInThread=1 is the right thing; we are
>              //  non-dummy.)
>              thread->GetChildHdrAt(msgIndexInThread, &newHdr);
> +          } // Nothing to do for dummy case, we're already inserting 'B'.

nit: you've moved other comments like this, so maybe this should move as well.
Attachment #8910321 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+2) from comment #3)
> Comment on attachment 8910321 [details] [diff] [review]
> nsMsgGroupView-whitespace.patch
> 
> Review of attachment 8910321 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice clean-up, but please consider addressing a few more spots I pointed out
> below.
> 
> ::: mailnews/base/src/nsMsgGroupView.cpp
> @@ +376,2 @@
> >        foundThread->m_threadKey = (nsMsgKey)
> > +      PL_HashString(NS_LossyConvertUTF16toASCII(hashKey).get());
> 
> nit: looks like this should be indented. I'd prefer the cast in front, so:
> foundThread->m_threadKey =
>   (nsMsgKey)...

Fixed.

> You think adding braces makes this more legible?

The rules seems to be that if the |if| requires braces, so does the else even if it's one line. And braces seem to go even if
there one line plus comments.  I sort of agree with that.

> 
> @@ +512,5 @@
> >  }
> >  
> >  NS_IMETHODIMP
> > +nsMsgGroupView::CopyDBView(nsMsgDBView *aNewMsgDBView,
> > +                           nsIMessenger *aMessengerInstance, 
> 
> nit: trailing space.
> 
> @@ +632,5 @@
> >            if (insertedAtThreadRoot)
> >              msgIndexInThread++;
> >            // If this header is the new parent of the thread... AND
> >            // If we are not using a dummy row, this means we need to append our
> >            //  old node as the first child of the new root.
> 
> nit: extra space left here.
> 
> @@ +640,5 @@
> >            //  "content" node down.)
> >            // Example mini-diagrams, wrapping the to-add thing with ()
> >            //  No dummy row; we had: [A], now we have [B], we want [B (A)].
> >            //  Dummy row; we had: [A A], now we have [B A], we want [B (B) A].
> >            //  (Coming into this we're adding 'B')
> 
> nit: Not sure why there are double spaces.
> 
> @@ +648,4 @@
> >              //  insert it. (offset msgIndexInThread=1 is the right thing; we are
> >              //  non-dummy.)
> >              thread->GetChildHdrAt(msgIndexInThread, &newHdr);
> > +          } // Nothing to do for dummy case, we're already inserting 'B'.
> 
> nit: you've moved other comments like this, so maybe this should move as
> well.

It was to explain the else case, so fixed for that.
Attached patch nsMsgGroupView-whitespace.patch (obsolete) — Splinter Review
Assignee: nobody → alta88
Attachment #8910321 - Attachment is obsolete: true
Attachment #8910389 - Flags: review+
Attached patch nsMsgGroupView-validIndex.patch (obsolete) — Splinter Review
Attachment #8910390 - Flags: review?(jorgk)
Thanks for further clean-up. I took the liberty to do this:
1) Remove braces in one case where if and else consisted of one statement
   and there were no comments. We agree on the rules, so let's apply them ;-)
2) Removed a space after a case, I think it should be (type)x and not
   (type) x.
3) Removed the empty else block. Difficult question where to
   put the comment.
So let's move onto the meat.
Attachment #8910389 - Attachment is obsolete: true
Attachment #8910428 - Flags: review+
Comment on attachment 8910390 [details] [diff] [review]
nsMsgGroupView-validIndex.patch

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

Thanks, looks plausible. I guess we can't reproduce the crash, right?

::: mailnews/base/src/nsMsgGroupView.cpp
@@ +1043,1 @@
>    if (m_flags[aViewIndex] & MSG_VIEW_FLAG_DUMMY)

So that crashed using an invalid index here?

@@ +1109,3 @@
>    {
>      // This case is all we care about at this point.
>      if (m_flags[msgIndex] & MSG_VIEW_FLAG_ISTHREAD)

Same here, right?
Attachment #8910390 - Flags: review?(jorgk) → review+
no. and in fact testing an out of bounds array index used to be ok (returns a truthy false), but due to core tightening up of these things it will now crash.

the first case is the crash case, the second case could crash and is fixed in advance.  but all of these m_keys, m_flags, m_levels arrays and accesses in *view code are candidates for crashes unless tested for length instead of index existing.
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/51a7568e31f5
Fix nsMsgGroupView.cpp formatting, comments, arg alignment (whitespace only changes). r=jorgk
https://hg.mozilla.org/comm-central/rev/d0354f63f052
Fix crash in nsMsgGroupView::LoadMessageByViewIndex(). r=jorgk
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Uplift? Please request.
Target Milestone: --- → Thunderbird 57.0
up to wsmwk.
Attachment #8910390 - Flags: approval-comm-esr52?
Attachment #8910390 - Flags: approval-comm-beta+
Sorry, I had to back this out since it caused this test failure:
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/folder-display/test-summarization.js | test-summarization.js::test_summary_updates_when_new_message_added_to_collapsed_thread
I checked that the test passes with this backed out locally. I guess the second hunk is at fault here.

Backout:
https://hg.mozilla.org/comm-central/rev/cc49055e5926dbdc2c04ed42decccfcf0db0f716
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to alta88 from comment #12)
> up to wsmwk.

not totally up to me. 
But with 57 beta in the not too distant future, there's really not strong reason to uplift to 56 beta - even though it's a 52 topcrash, it's only #18. strike 1.
A strong reason for uplifting is whether it's a recent regression. it is not. strike 2.
<= couple days on trunk - strike 3.
IMO we should skip for 56, whether it builds/tests or not.
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #16)
> IMO we should skip for 56, whether it builds/tests or not.
Yes, backed out on TB 56 beta.
For causing trouble, the entire function is now evicted.. You will notice that there is only one caller of ThreadIndexOfMsg(), on new message addition, and it always calls with second arg nsMsgViewIndex_None, meaning the msgGroupView override function always delegates to the msgDBView function.  Making the msgGroupView func useless.

Passes the prior failing test locally, X tests pass, and new messages in grouped view are received and added to the right bucket normally with this patch.
Attachment #8910428 - Attachment is obsolete: true
Attachment #8911166 - Flags: review?(jorgk)
Attachment #8910428 - Attachment is obsolete: false
Attachment #8910390 - Attachment is obsolete: true
Attachment #8910390 - Flags: approval-comm-esr52?
Attachment #8910390 - Flags: approval-comm-beta+
Thanks, very plausible. Try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=666b51b4a5ab6cbd022c6e9d28e053781820bf07

Better safe than sorry, fortunately Linux tests work again.
Comment on attachment 8911166 [details] [diff] [review]
nsMsgGroupView-validIndex2.patch

Try is fine and I also followed your argument in the code more closely now.
Attachment #8911166 - Flags: review?(jorgk) → review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2a13941913ba
Fix crash in nsMsgGroupView::LoadMessageByViewIndex(), take 2. r=jorgk
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Thunderbird 57.0 → Thunderbird 58.0
Comment on attachment 8911166 [details] [diff] [review]
nsMsgGroupView-validIndex2.patch

OK, let's consider "take 2" for uplift ;-)
Attachment #8911166 - Flags: approval-comm-esr52?
Attachment #8911166 - Flags: approval-comm-beta+
Whiteboard: TB 57 beta 1 => TB 52.5 ESR
Duplicate of this bug: 1415383
Attachment #8911166 - Flags: approval-comm-esr52? → approval-comm-esr52+
No 52.5.0 reports on crash-stats
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.