Crash in InvalidArrayIndex_CRASH | nsMsgDBView::GetCommandStatus [Mac]

RESOLVED FIXED in Thunderbird 52.0

Status

MailNews Core
Backend
--
critical
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: wsmwk, Assigned: alta88)

Tracking

({crash})

Trunk
Thunderbird 52.0
Unspecified
Mac OS X
crash

Thunderbird Tracking Flags

(thunderbird50 unaffected, thunderbird51 affected, thunderbird52 fixed)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
Created attachment 8809448 [details] [diff] [review]
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]);
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]);
(Assignee)

Comment 3

2 years ago
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.
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8809448 - Flags: review?(jorgk)
Created attachment 8809780 [details] [diff] [review]
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.
Attachment #8809780 - Flags: review?(rkent)

Comment 8

2 years ago
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+
https://hg.mozilla.org/comm-central/rev/228b399d8933abaa10c96f7616f5f98fb452821e

Wayne, uplift? To where?
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(vseerror)
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
(Reporter)

Comment 10

2 years ago
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)
(Assignee)

Comment 11

2 years ago
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.
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 ;-(
(Reporter)

Comment 13

2 years ago
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]
status-thunderbird50: --- → unaffected
status-thunderbird51: --- → affected
status-thunderbird52: --- → fixed

Comment 14

11 months ago
a=me for SM2.48 for whichever parts need uplifting

Updated

11 months ago
Blocks: 1351984
You need to log in before you can comment on or make changes to this bug.