Stack of the IMAP thread: msgimap!nsTArray<unsigned int>::ElementAt+0x31 msgimap!nsTArray<unsigned int>::operator[]+0x13 msgimap!nsImapFlagAndUidState::GetMessageFlagsFromUID+0x132 msgimap!nsImapProtocol::GetFlagsForUID+0x20 msgimap!nsImapProtocol::ProcessStoreFlags+0x2ab msgimap!nsImapProtocol::ProcessSelectedStateURL+0x1692 msgimap!nsImapProtocol::ProcessCurrentURL+0xad6 msgimap!nsImapProtocol::ImapThreadMainLoop+0x128 msgimap!nsImapProtocol::Run+0x90 xpcom_core!nsThread::ProcessNextEvent+0x1f3 xpcom_core!NS_ProcessNextEvent_P+0x51 xpcom_core!nsThread::ThreadFunc+0xca nspr4!_PR_NativeRunThread+0xd9 nspr4!pr_root+0x17 MSVCR71D!_beginthreadex+0x196 kernel32!BaseThreadStart+0x37 In nsImapFlagAndUidState::GetMessageFlagsFromUID, fUIDs has zero length (fPartialUIDFetch is 1) and the code tries to access element zero. In case it's relevant, I have attachment 419597 [details] [diff] [review] applied.

The patch for bug 524902 has landed on the trunk - it uses an nsTArray instead of just a c++ pointer, so invalid array references will cause assertions. It also clears out the array when we switch selected folders, which may exacerbate the situation, though switching to a larger folder would in theory have the same issue. We'll just have to keep an eye on this...I have not seen the assertion yet.

So if a cleared array is to be expected, then does GetMessageFlagsFromUID need to be fixed to expect an empty array?

(In reply to comment #2) > So if a cleared array is to be expected, then does GetMessageFlagsFromUID need > to be fixed to expect an empty array? Ah, thx, I think I see the issue now. Presumably fUids is empty, and msgIndex is 0 here: // next, move msgIndex up to the first slot > than uid. while ((PRUint32) uid < fUids[msgIndex]) msgIndex++; I'll fix that, and any other issues I see, and try to come up with an xpcshell test.

I can't do xpcshell tests because we don't have condstore support in our imap fake server. Some day I'm going to have to add that. Now that we're using nsTArray, there's not much sense in keeping track of how much space we've allocated when nsTArray will tell us how big it is, so I've gotten rid of that extra code.

I wonder whether GreatestIndexLtEq would help here. I see it's new for 1.9.2

Yes, I guess that would get rid of the while loop - I can try that when I get my environment set up again - I think the rest of the patch is still useful, though.

Comment on attachment 420339 [details] [diff] [review] proposed fix > fNumberOfMessagesAdded = 0; >- fNumberOfMessageSlotsAllocated = numberOfMessages; So, looking at this more closely, fUids.Length() actually seems closer in meaning to fNumberOfMessagesAdded and fNumberOfMessageSlotsAllocated corresponds to fUids.Capacity() instead. We could eliminate the latter too as nsTArray also performs is own capacity management. >+ if (!numberOfMessages) >+ numberOfMessages = kImapFlagAndUidStateSize; [This is never true as there is only one caller that passes in 100.] >+ fFlags.InsertElementsAt(0, numberOfMessages, 0); >+ fUids.InsertElementsAt(0, numberOfMessages, 0); This would be equivalent to constructing fFlags/fUids i.e. nsImapFlagAndUidState::nsImapFlagAndUidState(int numberOfMessages) : fUids(numberOfMessages), fFlags(numberOfMessages) { > if (zeroBasedIndex + 1 > fNumberOfMessagesAdded) > fNumberOfMessagesAdded = zeroBasedIndex + 1; This would have to change to use InsertElements to ensure that fUids/fFlags have an element at that index, but just performing length management (to zeroBasedIndex + 1) rather than capacity management.

Neil, this isn't quite the same as what you suggested, but I think calling SetLength() does the right thing...

I'm writing some c++ test code for this, since I didn't want to expose the ability to create flag state objects to xpcom.

added some tweaks to fix some assertions I was seeing...

Comment on attachment 423090 [details] [diff] [review] fix with unit test >+ if (zeroBasedIndex < fUids.Length()) >+ *aResult = fUids[zeroBasedIndex]; > else >+ *aResult = 0xFFFFFFFF; // so that value is non-zero and we don't ask for bad msgs Nit: could use fUids.SafeElementAt(zeroBAsedIndex, 0xFFFFFFFF); > imapMessageFlagsType returnFlags = kNoImapMsgFlag; >- if (zeroBasedIndex < fNumberOfMessagesAdded) >+ if (zeroBasedIndex < fUids.Length()) > returnFlags = fFlags[zeroBasedIndex]; > > *result = returnFlags; And again here. >+ for (counter = 0; counter < (PRUint32) fUids.Length(); counter++) Nit: Length() is already PRUint32 >+ if (zeroBasedIndex >= fUids.Length()) > { >- PRInt32 sizeToGrowBy = NS_MAX(kImapFlagAndUidStateSize, >- fNumberOfMessagesAdded - fNumberOfMessageSlotsAllocated); >- fNumberOfMessageSlotsAllocated += sizeToGrowBy; >- fUids.InsertElementsAt(fUids.Length(), sizeToGrowBy, 0); >- fFlags.InsertElementsAt(fFlags.Length(), sizeToGrowBy, 0); >+ fUids.SetLength(zeroBasedIndex + 1); >+ fFlags.SetLength(zeroBasedIndex + 1); The old code zeroed out the intervening slots, the new one does not. Problem? > imapMessageFlagsType nsImapFlagAndUidState::GetMessageFlagsFromUID(PRUint32 uid, PRBool *foundIt, PRInt32 *ndx) What about my suggestion to use GreatestIndexLtEq? >- while ((PRUint32) uid < fUids[msgIndex]) [Hmm, isn't uid already a PRUint32?]

(In reply to comment #11) > (From update of attachment 423090 [details] [diff] [review]) > >+ if (zeroBasedIndex < fUids.Length()) > >+ *aResult = fUids[zeroBasedIndex]; > > else > >+ *aResult = 0xFFFFFFFF; // so that value is non-zero and we don't ask for bad msgs > Nit: could use fUids.SafeElementAt(zeroBAsedIndex, 0xFFFFFFFF); I like that, thx. > > >+ if (zeroBasedIndex >= fUids.Length()) > > { > >- PRInt32 sizeToGrowBy = NS_MAX(kImapFlagAndUidStateSize, > >- fNumberOfMessagesAdded - fNumberOfMessageSlotsAllocated); > >- fNumberOfMessageSlotsAllocated += sizeToGrowBy; > >- fUids.InsertElementsAt(fUids.Length(), sizeToGrowBy, 0); > >- fFlags.InsertElementsAt(fFlags.Length(), sizeToGrowBy, 0); > >+ fUids.SetLength(zeroBasedIndex + 1); > >+ fFlags.SetLength(zeroBasedIndex + 1); > The old code zeroed out the intervening slots, the new one does not. Problem? It would be a problem...I'll try to see if I can add that to the unit test. > > > imapMessageFlagsType nsImapFlagAndUidState::GetMessageFlagsFromUID(PRUint32 uid, PRBool *foundIt, PRInt32 *ndx) > What about my suggestion to use GreatestIndexLtEq? I looked at that, and I can still try it, but the comparator stuff put me off...also, from looking at the code, I don't think it does a binary search.

This addresses the comments, and extends the unit test to detect the case where we're not initializing the fUids when making space for new uids.

I'm going to spin up a try server build with this patch for ludo to try, since I think the crash he has been seeing is related to the issues this patch fixes, and I'd like to see if the crash goes away with the patch.

Comment on attachment 424112 [details] [diff] [review] patch w/o extraneous files This is looking good, I'll test it out tomorrow. >-nsImapFlagAndUidState::nsImapFlagAndUidState(PRInt32 numberOfMessages) >+nsImapFlagAndUidState::nsImapFlagAndUidState(PRInt32 numberOfMessages) Nit: A space crept in here; git-apply said there were 5 lines in total. >+ *foundIt = fUids.GreatestIndexLtEq(uid, nsDefaultComparator<PRUint32, PRUint32>(), (PRUint32 *) ndx); Nice code replacement :-) >+ imapMessageFlagsType returnFlags = (*foundIt) ? fFlags[*ndx] : 0; kNoImapMsgFlag perhaps?

ludo, here's a link to the mac try server build with the patch - http://s3.mozillamessaging.com/build/try-server/2010-01-28_13:30-bienvenu@nventure.com-1264714116/bienvenu@nventure.com-1264714116-mail-try-mac.dmg

(In reply to comment #17) > ludo, here's a link to the mac try server build with the patch - > http://s3.mozillamessaging.com/build/try-server/2010-01-28_13:30-bienvenu@nventure.com-1264714116/bienvenu@nventure.com-1264714116-mail-try-mac.dmg Running it right now - will let you know monday/tuesday if this is gone.

(In reply to comment #19) > Running it right now - will let you know monday/tuesday if this is gone. haven't crashed while using that build.

we definitely want this for beta 1.

(In reply to comment #21) > we definitely want this for beta 1. /me agrees ! mark could you sr ?

I took a look at this last night. It generally seems ok, however when I backed out the code changes I couldn't get the unit test to fail... any ideas?

yeah, it just depends on how memory is initialized/uninitialized in terms of the test failing. I thought I had tweaked it so it fails on a windows debug build, but it's been a while, so my memory is a bit fuzzy. I may have been focusing on making the test fail with the related 3.01 issue.

fix checked in.