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

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 1

[Wayne Mery (:wsmwk)]

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

Comment 2

[alta88]

Attachment: threadIndex.patch

Comment 3

[alta88]

Attachment: threadIndex.patch [landed in comment #5]

include the real fix..

Comment 4

[Kent James (:rkent)]

Comment on attachment 8805150 [details] [diff] [review]
threadIndex.patch [landed in comment #5]

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

LGTM

Comment 5

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/db4ffff3e7f8770a3633251e5eb63ce7837d3564
Landed on closed/busted tree, will follow-up with try run.

Comment 6

[Jorg K (CEST = GMT+2)]

Did I hear anyone say "uplift"?

Comment 7

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f2fd71ad418ed4cc1b40b3033fccb756f81e2d92

Comment 8

[alta88]

Comment on attachment 8805150 [details] [diff] [review]
threadIndex.patch [landed in comment #5]

yes, to match Bug 1159244 landing in 51.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8805150 [details] [diff] [review]
threadIndex.patch [landed in comment #5]

Great, I'll see how my push goes and then uplift.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8805150 [details] [diff] [review]
threadIndex.patch [landed in comment #5]

Great, I'll see how my push goes and then uplift.

Comment 11

[Wayne Mery (:wsmwk)]

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.

Comment 12

[Jorg K (CEST = GMT+2)]

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

Comment 13

[Wayne Mery (:wsmwk)]

(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

Comment 15

[Jorg K (CEST = GMT+2)]

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.

Comment 16

[alta88]

Attachment: threadIdx.patch [landed in comment #18]

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)

Comment 17

[Jorg K (CEST = GMT+2)]

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.

Comment 18

[Jorg K (CEST = GMT+2)]

 https://hg.mozilla.org/comm-central/rev/6bbafc1273eed73e3ef6192416d7eaf343a2ebb6

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Can we uplift this to beta ? I have been crashing on this.
Thanks

Comment 20

[Jorg K (CEST = GMT+2)]

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.

Comment 21

[Jorg K (CEST = GMT+2)]

Plus I have more beta uplifts pending, so this can go with them.

Comment 22

[Wayne Mery (:wsmwk)]

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.

Comment 23

[Jorg K (CEST = GMT+2)]

Beta (TB 51):
https://hg.mozilla.org/releases/comm-beta/rev/46b0ecea63521f6e646607cc3913827858ed04c1
https://hg.mozilla.org/releases/comm-beta/rev/eb2be954400c0eff2a5a56644ce7d110412439f9

Comment 24

[Wayne Mery (:wsmwk)]

A couple 51.0b1 crashes, but then no crashes starting 51.0b2. So v.fixed.  Nice!