Debug MozMill tests fail with assertion 'i < Length() (invalid array index)'

RESOLVED FIXED in Thunderbird 15.0


7 years ago
7 years ago


(Reporter: standard8, Assigned: standard8)


Thunderbird 15.0
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



7 years ago
Created attachment 621294 [details] [diff] [review]
Proposed fix

We're seeing an assertion on the debug MozMill tests:

Assertion failure: i < Length() (invalid array index), at ../../dist/include/nsTArray.h:558

The top of the stack for this is:

0 + 0xee6b
    rbx = 0x00000000   r12 = 0xd9947a80   r13 = 0x00000000   r14 = 0xd83c2850
    r15 = 0x02ba4370   rip = 0xd360ee6b   rsp = 0xd673dc78   rbp = 0xd673dc90
    Found by: given as instruction pointer in context
 1!nsTArray<unsigned int, nsTArrayDefaultAllocator>::ElementAt [try-comm-central::1b45629067a3 : 558 + 0x37]
    rip = 0x007d7b75   rsp = 0xd673dc80
    Found by: stack scanning
 2!nsTArray<unsigned int, nsTArrayDefaultAllocator>::operator[] [try-comm-central::1b45629067a3 : 591 + 0x10]
    rip = 0x007d65b4   rsp = 0xd673dca0
    Found by: stack scanning
 3!nsMsgDatabase::MarkAllRead [nsMsgDatabase.cpp : 2662 + 0x10]
    rip = 0x01c1afe9   rsp = 0xd673dcc0
    Found by: stack scanning
 4!nsMsgDatabase::MarkHdrNotNew [nsMsgDatabase.cpp : 2628 + 0x6]
    rip = 0x01c1ad38   rsp = 0xd673dd30
    Found by: stack scanning
 5  0x7fffd673dd7f
    rbx = 0xd673e000   rip = 0xd673dd80   rsp = 0xd673dd38   rbp = 0x01c1ad38
    Found by: call frame info
 6!nsMsgDBFolder::MarkAllMessagesRead [nsMsgDBFolder.cpp : 1467 + 0x2c]
    rip = 0x019d98df   rsp = 0xd673dd40
    Found by: stack scanning

The issue is in MarkAllRead - we're not checking the length of the nsTArray element before trying to copy the memory from it. I suspect we just generally get away with it in release mode, but we should fix it up anyway.

I believe that before the recent bustage, this was the only thing to get our debug MozMill builders green.

I'm attaching a patch that I think will fix it, I'm not altogether sure it is doing the right thing with the return values.
Attachment #621294 - Flags: review?(dbienvenu)

Comment 1

7 years ago
Comment on attachment 621294 [details] [diff] [review]
Proposed fix

This will cause us to generate a bogus url if you try to mark all read in an imap folder with no unread messages. Although this might be harmless, I tend to think we should either make this an error, or change the callers to check for a 0 *aNumKeys, e.g., nsImapMailFolder::MarkAllMessagesRead and nsImapMailFolder::MarkThreadRead.
Attachment #621294 - Flags: review?(dbienvenu) → review-

Comment 2

7 years ago
I'd be r+ happy with a patch that added a couple lines to StoreImapFlags to make it just return an error if numKeys is 0.

Comment 3

7 years ago
Created attachment 622359 [details] [diff] [review]
Proposed fix v2

I took another look at this, the problem I have with doing a change in StoreImapFlags is that it looks like we'd still get an undo change item created which is probably not what we want. So I've just added a check to ensure that we've actually changed something.

In nsMsgDBFolder::MarkAllMessagesRead I also moved an ensure success check so that we'd re-enable notifications even if MarkAllRead failed for some reason.
Attachment #621294 - Attachment is obsolete: true
Attachment #622359 - Flags: review?(dbienvenu)

Comment 4

7 years ago
Comment on attachment 622359 [details] [diff] [review]
Proposed fix v2

looks reasonable.
Attachment #622359 - Flags: review?(dbienvenu) → review+

Comment 5

7 years ago
Checked in:
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
You need to log in before you can comment on or make changes to this bug.