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 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 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.
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.
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 on attachment 622359 [details] [diff] [review] Proposed fix v2 looks reasonable.