Last Comment Bug 752215 - Debug MozMill tests fail with assertion 'i < Length() (invalid array index)'
: Debug MozMill tests fail with assertion 'i < Length() (invalid array index)'
Product: MailNews Core
Classification: Components
Component: Database (show other bugs)
: 12
: x86 Mac OS X
-- normal (vote)
: Thunderbird 15.0
Assigned To: Mark Banner (:standard8)
Depends on:
  Show dependency treegraph
Reported: 2012-05-05 07:04 PDT by Mark Banner (:standard8)
Modified: 2012-05-09 13:41 PDT (History)
1 user (show)
standard8: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Proposed fix (1.69 KB, patch)
2012-05-05 07:04 PDT, Mark Banner (:standard8)
mozilla: review-
Details | Diff | Splinter Review
Proposed fix v2 (3.44 KB, patch)
2012-05-09 06:17 PDT, Mark Banner (:standard8)
mozilla: review+
Details | Diff | Splinter Review

Description User image Mark Banner (:standard8) 2012-05-05 07:04:02 PDT
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.
Comment 1 User image David :Bienvenu 2012-05-05 12:52:31 PDT
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 User image David :Bienvenu 2012-05-05 20:45:38 PDT
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 User image Mark Banner (:standard8) 2012-05-09 06:17:42 PDT
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.
Comment 4 User image David :Bienvenu 2012-05-09 09:57:08 PDT
Comment on attachment 622359 [details] [diff] [review]
Proposed fix v2

looks reasonable.
Comment 5 User image Mark Banner (:standard8) 2012-05-09 13:41:56 PDT
Checked in:

Note You need to log in before you can comment on or make changes to this bug.