Closed Bug 433307 Opened 16 years ago Closed 16 years ago

Move nsMsgBiffManager from nsVoidArray to nsTArray

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: memory-footprint)

Attachments

(3 files)

nsMsgBiffManager currently allocates an nsVoidArray on startup and deallocates it on shutdown. In addition it allocates items for nsBiffEntry. Replacing with a nsTArray<nsBiffEntry> we'd be able to drop the of the explicit allocations.

Note that I'm also taking this opportunity to tidy some of the existing code in that class up a bit.
Attachment #320515 - Flags: superreview?(bienvenu)
Attachment #320515 - Flags: review?(bienvenu)
Comment on attachment 320515 [details] [diff] [review]
The fix (diff -w)

Mark, this all looks good, except for the printf memory leak lines:

-        mBiffArray->RemoveElementAt(i);
+        printf("EX MEMORY LEAK !!!!!!!!!!!!!! %d bytes\n****\n****\n****\n****\n", sizeof(nsBiffEntry));
+        mBiffArray.RemoveElementAt(i);

Do we really want that?
Attachment #320515 - Flags: superreview?(bienvenu)
Attachment #320515 - Flags: superreview+
Attachment #320515 - Flags: review?(bienvenu)
Attachment #320515 - Flags: review+
Patch checked in (without printf, which was wrong anyway). 
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Comment on attachment 320515 [details] [diff] [review]
The fix (diff -w)

>-    nsBiffEntry *current = (nsBiffEntry*)mBiffArray->ElementAt(i);
>+    nsBiffEntry &current = mBiffArray[i];
...
>-        mBiffArray->RemoveElementAt(i);
>+        mBiffArray.RemoveElementAt(i);
>         i--; //Because we removed it we need to look at the one that just moved up.
>         SetNextBiffTime(current, currentTime);
>         AddBiffEntry(current);
I crash here because you remove the current element, and if that's the only element in an nsTArray then it frees its memory. (With a void array the data would still exist for long enough, but technically it's an invalid read. Don't ask me what happens if you're not removing the last element...)
Comment on attachment 320515 [details] [diff] [review]
The fix (diff -w)

>+nsresult nsMsgBiffManager::AddBiffEntry(nsBiffEntry &biffEntry)
> {
>+  PRUint32 i;
>+  PRUint32 count = mBiffArray.Length();
>   for(i = 0; i < count; i++)
>   {
>+    if (biffEntry.nextBiffTime < mBiffArray[i].nextBiffTime)
>       break;
>   }
>   PR_LOG(MsgBiffLogModule, PR_LOG_ALWAYS, ("inserting biff entry at %d\n", i));
>+  mBiffArray.InsertElementAt(i, biffEntry);
>   return NS_OK;
> }
There's a bug where someone wants to add InsertElementSorted to nsTArray which would obsolete this function :-)
I think this should fix the crash - if we take a copy of the element rather than a reference, then it doesn't matter if nsTArray trashes the memory or not when we remove the element.
Attachment #321046 - Flags: superreview?(neil)
Attachment #321046 - Flags: review?(neil)
Comment on attachment 321046 [details] [diff] [review]
[checked in] Possible fix for crash

Well this seems to fix my crash but I'm not sure that this is the most appropriate fix, so asking a second opinion.
Attachment #321046 - Flags: superreview?(neil)
Attachment #321046 - Flags: superreview?(bienvenu)
Attachment #321046 - Flags: review?(neil)
Attachment #321046 - Flags: review+
Comment on attachment 321046 [details] [diff] [review]
[checked in] Possible fix for crash

seems OK, the alternative is to just pull out the server and nextBiffTime from the biff entry into local vars
Attachment #321046 - Flags: superreview?(bienvenu) → superreview+
FYI, my default server isn't getting biffed - I don't know if the original patch here is the cause, or if perhaps the latest patch will fix the problem...
(In reply to comment #9)
> FYI, my default server isn't getting biffed - I don't know if the original
> patch here is the cause, or if perhaps the latest patch will fix the problem...
> 
I'll check in this patch as is. I'll then see if I can come up with a unit test to prove the biff manager is working correctly. I'm a bit concerned about the length of time the test will take (as there is a 30 second time between biffs), so I might have to override that by a function call and doing a c++ test case. However, I'll give it a try and find out.
Flags: in-testsuite?
Attachment #320516 - Attachment description: The fix (full patch) → [checked in] The fix (full patch)
Attachment #321046 - Attachment description: Possible fix for crash → [checked in] Possible fix for crash
Comment on attachment 321046 [details] [diff] [review]
[checked in] Possible fix for crash

>diff -r 837e0647b579 mailnews/base/src/nsMsgBiffManager.cpp
>+    // Take a copy of the entry rather than the a reference so that we can

Nit: |the a| :-/
your latest patch did fix the problem, Mark - so biff is working for me again.
Depends on: 434061
Product: Core → MailNews Core
This FIXED bug is flagged with in‑testsuite?   It would be great if assignee or someone else can clear the flag if a test is not appropriate.  And if appropriate, create a test and plus the flag to finish off the bug.
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: