1314347 - Crash in InvalidArrayIndex_CRASH | nsMsgDBView::GetCommandStatus [Mac]

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Wayne Mery (:wsmwk)]

this InvalidArrayIndex signature remains in 50.0b3 even with bug 1302478 landed.
The signature was #4 crash for 50.0b2

bp-40bece50-25a8-4460-927e-bd8832161101. The user has several addons, but not e mailsummaries.  And somewhat odd, it's a Mac crash - we didn't have a single Mac crash for this sig in 50.0b2

0 	XUL	InvalidArrayIndex_CRASH(unsigned long, unsigned long)	xpcom/glue/nsTArray.cpp:35
1 	XUL	nsMsgDBView::GetCommandStatus(int, bool*, int*)	/builds/slave/tb-rel-c-beta-m64_bld-00000000/build/objdir-tb/x86_64/dist/include/nsTArray.h:996
2 	XUL	NS_InvokeByIndex	xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:180

Comment 1

[alta88]

Attachment: index.patch

some whitespace/comment/wrap changes (since i won't work on code formatted by wolves), but the only real change is:

-    *selectable_p = haveSelection && JunkControlsEnabled(selection[0]);
+    *selectable_p = selection.Length() && JunkControlsEnabled(selection[0]);

Comment 2

[Jorg K (CEST = GMT+2)]

Hmm, I must be your most hated reviewer, but I don't understand the change.

We is the fix not:
*selectable_p =  haveSelection && selection.Length() && JunkControlsEnabled(selection[0]);
or better
*selectable_p =  haveSelection && numIndices && JunkControlsEnabled(selection[0]);

Comment 3

[alta88]

i don't want to play ping pong on this, it's a waste of my time. if you really won't r+ unless it's changed to numIndices, then say change it to numIndices or whatever.

Comment 4

[Jorg K (CEST = GMT+2)]

That was not the primary question.

Code was:
*selectable_p = haveSelection && JunkControlsEnabled(selection[0]);

New suggested code is:
 *selectable_p = selection.Length() && JunkControlsEnabled(selection[0]);

Kindly explain why it is OK to remove haveSelection here. My suggestion was:
*selectable_p =  haveSelection && selection.Length() && JunkControlsEnabled(selection[0]);
(or replace selection.Length() as a time-waster).

But I won't r+ this unless I understand why it's OK to remove haveSelection. haveSelection is calculated earlier on and I don't see that haveSelection and selection.Length() are necessarily the same.

And if I'm missing something, please explain that to me.

Comment 5

[alta88]

because it doesn't matter what haveSelection is if selection.Length() is 0, it would just be verbosity. honestly, this is basic and it's not going to work for me if every nit has to be explained.

Comment 6

[Jorg K (CEST = GMT+2)]

Sorry, it is not obvious to me that if selection.Length() > 0 that then haveSelection is necessarily 'true', since
haveSelection = NonDummyMsgSelected(indices, numIndices);

I looked at nsMsgDBView::NonDummyMsgSelected and even there it is not clear when it will return 'true'.

If you don't want to explain this, please find a reviewer who understands it better without asking questions.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: One line fix.

Based on Alta88 investigation I'm presenting my version of the patch in order to move things forward. No offense intended, but somehow we couldn't agree on the best solution. I'm happy to land this in Alta88's name to honour his work.

Comment 8

[Kent James (:rkent)]

Comment on attachment 8809780 [details] [diff] [review]
One line fix.

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

I can't figure out why removing haveSelection is obvious, either. It seems like leaving it is safer if the only goal is a crash fix.

In handling alta88's contribution, I don't like to land patches in someone's name that they disagree with. Instead, add the comment "(based on analysis by alta88)" in the comment field unless he gives a feedback+ or review+ to your patch.

Comment 9

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/228b399d8933abaa10c96f7616f5f98fb452821e

Wayne, uplift? To where?

Comment 10

[Wayne Mery (:wsmwk)]

Thanks for asking. This will have had no bake time on c-c before merge, and it's only #9 crash for beta. So I think the prudent thing to do is similar to bug 1311672 - land on c-c and it can bake a couple weeks c-c and aurora, then uplift to beta for what will then be building 51.0b2

Comment 11

[alta88]

it is *blindingly* obvious that the only way a zero length |selection| array's 0 index gets evaluated and crashes is if haveSelection is *falsely* true.

i certainly don't want any association with a mindlessly cargo culted patch like this.

and it should be equally obvious no sane person is going to tolerate this sort of review, so let's not wonder why there are no contributors.

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to alta88 from comment #11)
> it is *blindingly* obvious that the only way a zero length |selection|
> array's 0 index gets evaluated and crashes is if haveSelection is *falsely*
> true.
Sure.

But I have asked you explain why we can't have length==5 and haveSelection==false.
In this case your patch would have changed existing behaviour and I fail to understand the consequences.

I haven't started programming yesterday and neither has Kent and we both don't understand your patch. I have asked you to explain but you refused.

Contributors most likely get scared by people without patience ;-(

Comment 13

[Wayne Mery (:wsmwk)]

Just noticed this is #1 crash for Mac (I hadn't sorted the mac crashes in a while). Which would ordinarily make me suspect it's same as an equally high ranked windows crasher like bug 1314644 and bug 1314348. That said and on the record, let's stick with the plan in comment 10.

Comment 14

[Ian Neal]

a=me for SM2.48 for whichever parts need uplifting