Closed Bug 1311672 Opened 3 years ago Closed 3 years ago

Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | nsMsgThreadedDBView::RemoveByIndex Trying to move a lot of emails in a directory.

Categories

(MailNews Core :: Backend, defect, critical)

Unspecified
All
defect
Not set
critical

Tracking

(thunderbird50 wontfix, thunderbird51 fixed, thunderbird52 fixed)

VERIFIED FIXED
Thunderbird 52.0
Tracking Status
thunderbird50 --- wontfix
thunderbird51 --- fixed
thunderbird52 --- fixed

People

(Reporter: sylvestre, Assigned: alta88)

References

Details

(Keywords: crash, regression, Whiteboard: [regression:TB50])

Crash Data

Attachments

(2 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-5e434453-5c8d-4bd4-bdb6-55c372161020.
=============================================================

Trying to move a lot of emails in a directory.
stack is not nice. bp-5e434453-5c8d-4bd4-bdb6-55c372161020
 18 	libxul.so	js::jit::DoCallFallback	js/src/jit/BaselineIC.cpp:5979
19 		@0x7fb50f3d9a95	
20 		@0x7fb4c604d517	

likely related to bug 1296680.
if you can reproduce...
See Also: → 1296680, 617073
Summary: Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | nsMsgThreadedDBView::RemoveByIndex → Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | nsMsgThreadedDBView::RemoveByIndex Trying to move a lot of emails in a directory.
Attached patch threadIndex.patch (obsolete) — Splinter Review
Assignee: nobody → alta88
Attachment #8805119 - Flags: review?(rkent)
include the real fix..
Attachment #8805119 - Attachment is obsolete: true
Attachment #8805119 - Flags: review?(rkent)
Attachment #8805150 - Flags: review?(rkent)
Comment on attachment 8805150 [details] [diff] [review]
threadIndex.patch [landed in comment #5]

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

LGTM
Attachment #8805150 - Flags: review?(rkent) → review+
Component: General → Backend
Product: Thunderbird → MailNews Core
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/db4ffff3e7f8770a3633251e5eb63ce7837d3564
Landed on closed/busted tree, will follow-up with try run.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
Keywords: checkin-needed
Did I hear anyone say "uplift"?
Comment on attachment 8805150 [details] [diff] [review]
threadIndex.patch [landed in comment #5]

yes, to match Bug 1159244 landing in 51.
Attachment #8805150 - Flags: approval-comm-aurora?
Comment on attachment 8805150 [details] [diff] [review]
threadIndex.patch [landed in comment #5]

Great, I'll see how my push goes and then uplift.
Attachment #8805150 - Flags: approval-comm-aurora? → approval-comm-aurora+
jorg, big thanks for keeping track of these arrayindex patches.  Looking at crash-stats I'd actually prefer this patch not be put on aurora for 51 - so that it is not in the first 51 beta. I anticipate asking for uplift for the *second* beta.  That way we can better judge which crash signatures go away, and also make it clearer to associate beta tester results (both positive and negative) to specific patches.

Marking 52 as affected until we can confirm with sylvestre or other means that the crash is gone.
Wayne, you really want to delay this until TB 51 beta 2 and let people crash in the meantime?
Alta suggested to uplift this to TB 51 asap to go with bug 1159244 which has already landed (comment #8).

Since C-C is mostly back, I intend to do the Aurora uplifts soon (maybe today).
Flags: needinfo?(vseerror)
(In reply to Jorg K (GMT+1) from comment #12)
> Wayne, you really want to delay this until TB 51 beta 2 and let people crash
> in the meantime?

Yes, I am looking for a delay. This signature ranks only #27 50.0b2. And so far there isn't a single crash in 50.0b3.

So what I'd like to see for this bug, is see what is happening in 51.0b1 when it comes out, and go from there.

Conversely, we do the patch from bug 1314348 as originally scheduled, for 51 aurora before the merge, and hence it will be in 51.0b1
Flags: needinfo?(vseerror)
See Also: 1296680
Duplicate of this bug: 1296680
Comment on attachment 8805150 [details] [diff] [review]
threadIndex.patch [landed in comment #5]

I'm not sure I like Wayne's approach, but let's go with it anyway. So no uplift to TB 51 Aurora, but a late uplift to TB 51 Beta.
Attachment #8805150 - Flags: approval-comm-aurora+ → approval-comm-beta?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
there's another place that needs to be checked; only real change here is

-      if (numThreadChildren > 0)
+      // Above RemoveByIndex may now make index out of bounds.
+      if (IsValidIndex(index) && numThreadChildren > 0)
Attachment #8809520 - Flags: review?(jorgk)
Comment on attachment 8809520 [details] [diff] [review]
threadIdx.patch [landed in comment #18]

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

Thanks. I can land this together with bug 1314347 once my question is answered.
Attachment #8809520 - Flags: review?(jorgk) → review+
Attachment #8805150 - Attachment description: threadIndex.patch → threadIndex.patch [landed in comment #5]
 https://hg.mozilla.org/comm-central/rev/6bbafc1273eed73e3ef6192416d7eaf343a2ebb6
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Attachment #8809520 - Attachment description: threadIdx.patch → threadIdx.patch [landed in comment #18]
Attachment #8809520 - Flags: approval-comm-beta?
Can we uplift this to beta ? I have been crashing on this.
Thanks
Flags: needinfo?(vseerror)
I wanted to uplift this all along (comment #12), but Wayne wants to have a delay (comment #13).

Personally I find it cruel to let people crash, so I'd uplift this straight away if Wayne agrees.
Plus I have more beta uplifts pending, so this can go with them.
I don't buy the cruelty label for one second unless someone is being hammered by crashes - which doesn't seem to be the case in bug comments or in crash-stats. According to crash stats only 4 users are affected (granted not all user report crashes so no doubt more than 4 users are affected - sylvestre perhaps one of them).   So I stand by the rationale in my request in comment 13, which I can elaborate on if you feel it's urgent.  And of course, the fix has been available in aurora for several weeks.
Flags: needinfo?(vseerror)
Keywords: regression
OS: Linux → All
See Also: → 1159244
Whiteboard: [regression:TB50]
Attachment #8809520 - Flags: approval-comm-beta? → approval-comm-beta+
Attachment #8805150 - Flags: approval-comm-beta? → approval-comm-beta+
A couple 51.0b1 crashes, but then no crashes starting 51.0b2. So v.fixed.  Nice!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.