Closed
Bug 433307
Opened 16 years ago
Closed 16 years ago
Move nsMsgBiffManager from nsVoidArray to nsTArray
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9
People
(Reporter: standard8, Assigned: standard8)
References
Details
(Keywords: memory-footprint)
Attachments
(3 files)
12.11 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
13.07 KB,
patch
|
Details | Diff | Splinter Review | |
688 bytes,
patch
|
neil
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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•16 years ago
|
||
Comment 2•16 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•16 years ago
|
||
Patch checked in (without printf, which was wrong anyway).
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9
Comment 4•16 years ago
|
||
Comment on attachment 320515 [details] [diff] [review] The fix (diff -w) >- nsBiffEntry *current = (nsBiffEntry*)mBiffArray->ElementAt(i); >+ nsBiffEntry ¤t = 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•16 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•16 years ago
|
||
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•16 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•16 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•16 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•16 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•16 years ago
|
Attachment #320516 -
Attachment description: The fix (full patch) → [checked in] The fix (full patch)
Assignee | ||
Updated•16 years ago
|
Attachment #321046 -
Attachment description: Possible fix for crash → [checked in] Possible fix for crash
Comment 11•16 years ago
|
||
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•16 years ago
|
||
your latest patch did fix the problem, Mark - so biff is working for me again.
Updated•16 years ago
|
Product: Core → MailNews Core
Comment 13•15 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•9 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•