Crash in InvalidArrayIndex_CRASH | nsMsgDBView::GetCommandStatus [Mac]

RESOLVED FIXED in Thunderbird 52.0

Status

defect
--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: wsmwk, Assigned: alta88)

Tracking

({crash})

Trunk
Thunderbird 52.0
Unspecified
macOS

Thunderbird Tracking Flags

(thunderbird50 unaffected, thunderbird51 affected, thunderbird52 fixed)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

3 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

3 years ago
Posted 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)

Comment 2

3 years ago
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

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

Comment 4

3 years ago
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

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

Comment 6

3 years ago
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

3 years ago
Attachment #8809448 - Flags: review?(jorgk)

Comment 7

3 years ago
Posted 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+

Comment 9

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

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

Comment 10

3 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

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

Comment 12

3 years ago
(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

3 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]

Comment 14

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