Move nsMsgBiffManager from nsVoidArray to nsTArray

RESOLVED FIXED in mozilla1.9

Status

MailNews Core
Backend
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

({memory-footprint})

Trunk
mozilla1.9
memory-footprint

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

10 years ago
Created attachment 320515 [details] [diff] [review]
The fix (diff -w)

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)
(Assignee)

Comment 1

10 years ago
Created attachment 320516 [details] [diff] [review]
[checked in] The fix (full patch)

Comment 2

10 years ago
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+
(Assignee)

Comment 3

10 years ago
Patch checked in (without printf, which was wrong anyway). 
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Target Milestone: --- → mozilla1.9

Comment 4

10 years ago
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 5

10 years ago
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 :-)
(Assignee)

Comment 6

10 years ago
Created attachment 321046 [details] [diff] [review]
[checked in] Possible fix for crash

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 7

10 years ago
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 8

10 years ago
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+

Comment 9

10 years ago
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...
(Assignee)

Comment 10

10 years ago
(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?
(Assignee)

Updated

10 years ago
Attachment #320516 - Attachment description: The fix (full patch) → [checked in] The fix (full patch)
(Assignee)

Updated

10 years ago
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| :-/

Comment 12

10 years ago
your latest patch did fix the problem, Mark - so biff is working for me again.

Updated

10 years ago
Depends on: 434061
Product: Core → MailNews Core

Comment 13

9 years ago
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.
(Assignee)

Updated

3 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.