Closed Bug 1314347 Opened 4 years ago Closed 4 years ago

Crash in InvalidArrayIndex_CRASH | nsMsgDBView::GetCommandStatus [Mac]


(MailNews Core :: Backend, defect)

Not set


(thunderbird50 unaffected, thunderbird51 affected, thunderbird52 fixed)

Thunderbird 52.0
Tracking Status
thunderbird50 --- unaffected
thunderbird51 --- affected
thunderbird52 --- fixed


(Reporter: wsmwk, Assigned: alta88)



(Keywords: crash)

Crash Data


(2 files)

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
Attached patch index.patchSplinter Review
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]);
Assignee: nobody → alta88
Attachment #8809448 - Flags: review?(jorgk)
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]);
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.
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.
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.
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.
Attachment #8809448 - Flags: review?(jorgk)
Attached patch One line fix.Splinter Review
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.
Attachment #8809780 - Flags: review?(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.
Attachment #8809780 - Flags: review?(rkent) → review+

Wayne, uplift? To where?
Closed: 4 years ago
Flags: needinfo?(vseerror)
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
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
Flags: needinfo?(vseerror)
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.
(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.

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 ;-(
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.
Summary: Crash in InvalidArrayIndex_CRASH | nsMsgDBView::GetCommandStatus → Crash in InvalidArrayIndex_CRASH | nsMsgDBView::GetCommandStatus [Mac]
a=me for SM2.48 for whichever parts need uplifting
Blocks: SM2.48
You need to log in before you can comment on or make changes to this bug.