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

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Mark Banner (:standard8)]

Attachment: 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  libpthread-2.11.so + 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  libxul.so!nsTArray<unsigned int, nsTArrayDefaultAllocator>::ElementAt [try-comm-central::1b45629067a3 : 558 + 0x37]
    rip = 0x007d7b75   rsp = 0xd673dc80
    Found by: stack scanning
 2  libxul.so!nsTArray<unsigned int, nsTArrayDefaultAllocator>::operator[] [try-comm-central::1b45629067a3 : 591 + 0x10]
    rip = 0x007d65b4   rsp = 0xd673dca0
    Found by: stack scanning
 3  libxul.so!nsMsgDatabase::MarkAllRead [nsMsgDatabase.cpp : 2662 + 0x10]
    rip = 0x01c1afe9   rsp = 0xd673dcc0
    Found by: stack scanning
 4  libxul.so!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  libxul.so!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.

Comment 1

[David :Bienvenu]

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.

Comment 2

[David :Bienvenu]

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

[Mark Banner (:standard8)]

Attachment: 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.

Comment 4

[David :Bienvenu]

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

looks reasonable.

Comment 5

[Mark Banner (:standard8)]

Checked in: https://hg.mozilla.org/comm-central/rev/f73fb84d8279