Closed
Bug 1354809
Opened 7 years ago
Closed 7 years ago
Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | nsMsgGroupView::LoadMessageByViewIndex
Categories
(Thunderbird :: General, defect)
Tracking
(thunderbird_esr5257+ fixed, thunderbird56 wontfix, thunderbird57 fixed, thunderbird58 fixed)
VERIFIED
FIXED
Thunderbird 58.0
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)
45.01 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
3.66 KB,
patch
|
jorgk-bmo
:
review+
jorgk-bmo
:
approval-comm-beta+
jorgk-bmo
:
approval-comm-esr52+
|
Details | Diff | Splinter Review |
#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
Reporter | ||
Comment 1•7 years ago
|
||
This has gained new life in 52.1.1 - #29 crash. #20 for beta
Keywords: topcrash-thunderbird
whitespace only: comments, arg alignment, formatting.
Attachment #8910321 -
Flags: review?(jorgk)
Comment 3•7 years ago
|
||
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.
Assignee: nobody → alta88
Attachment #8910321 -
Attachment is obsolete: true
Attachment #8910389 -
Flags: review+
Attachment #8910390 -
Flags: review?(jorgk)
Comment 7•7 years ago
|
||
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 8•7 years ago
|
||
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
Comment 10•7 years ago
|
||
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
Assignee | ||
Comment 12•7 years ago
|
||
up to wsmwk.
Comment 13•7 years ago
|
||
Beta (TB 56): https://hg.mozilla.org/releases/comm-beta/rev/7159bd45a4f6813f96ad665986bd4e9da2ced430
status-thunderbird56:
--- → fixed
status-thunderbird57:
--- → fixed
status-thunderbird_esr52:
--- → affected
Updated•7 years ago
|
Attachment #8910390 -
Flags: approval-comm-esr52?
Attachment #8910390 -
Flags: approval-comm-beta+
Comment 14•7 years ago
|
||
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 → ---
Comment 15•7 years ago
|
||
Backout from Beta (TB 56): https://hg.mozilla.org/releases/comm-beta/rev/f39bbe313d6de4c83c2fccd85ebe7e972e67926e
Reporter | ||
Comment 16•7 years ago
|
||
(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.
Comment 17•7 years ago
|
||
(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.
Assignee | ||
Comment 18•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8910428 -
Attachment is obsolete: false
Updated•7 years ago
|
Attachment #8910390 -
Attachment is obsolete: true
Attachment #8910390 -
Flags: approval-comm-esr52?
Attachment #8910390 -
Flags: approval-comm-beta+
Comment 19•7 years ago
|
||
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 20•7 years ago
|
||
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
Comment 21•7 years ago
|
||
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: 7 years ago → 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•7 years ago
|
status-thunderbird58:
--- → fixed
Target Milestone: Thunderbird 57.0 → Thunderbird 58.0
Comment 22•7 years ago
|
||
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+
Updated•7 years ago
|
Whiteboard: TB 57 beta 1 => TB 52.5 ESR
Comment 23•7 years ago
|
||
Beta (TB 57): https://hg.mozilla.org/releases/comm-beta/rev/43dcc1c3ffae31ff1f7db724f7b351dc75481169
Updated•6 years ago
|
Attachment #8911166 -
Flags: approval-comm-esr52? → approval-comm-esr52+
Comment 25•6 years ago
|
||
Tb 52.5 ESR (should be tracking 57+): https://hg.mozilla.org/releases/comm-esr52/rev/fbe612a8be94
Updated•6 years ago
|
tracking-thunderbird_esr52:
--- → 57+
You need to log in
before you can comment on or make changes to this bug.
Description
•