Closed Bug 445999 Opened 16 years ago Closed 16 years ago

nsCStringArray::operator= leaks all strings that were in the destination array

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ted, Assigned: jruderman)

References

()

Details

(Keywords: memory-leak)

Attachments

(4 files, 1 obsolete file)

This has been leaking consistently since last night. philor suggests that this is from bug 445005.

The first orange build was changeset 68ddc8f23721, the previous green build was changeset c32a70debffa, so it would definitely be one of:

68ddc8f23721	Centralize win32 theme implementation - bug 444013 r=vlad
e05e892261a8	Disable double buffering when Vista compositor is enabled - bug 444005 r=vlad
c5e52a5ef92b	Cleanup unused transparency code in nsWindow.cpp - Bug 444679 r=vlad
10e63a02fbc9	Bug 431861 - filename not defined in symbolstore.py's GetCleanRoot (or GetCleanRoot assumes the presence of an @ in a CVS/Root) r=ted
661cce8d7fd0	Bug 307319 - Certificate details show incorrect public key information r=rrelyea
884012d735c9	Bug 413219 - Allow building GtkMozEmbed without MOZILLA_INTERNAL_API r=bsmedberg 
2ac85f53e36b	Bug 427448 - Applescript curl & ptit query worked in FF2 fails in FF3 r=josh, sr=roc
e888893e48ba	Bug 378518 - Remove support for tag names in XBL extends attribute r=Enn, sr=sicking
46f960ae5994	Bug 443870 - Add JSAutoSuspendRequest to match JSAutoRequest r=brendan
2935954b1bb2	Bug 445005 - "Would you like to save..." label now shown on unknownContentType popup This also adds a test to ensure that the unkownContentType dialog displays the right information with given files. r=gavin, r=sdwilsh 

We need to back out the offender at this point, as it's been orange all night, and people are checking in anyway, which is super bad.
That test unregisters every listener that it registers.  It's quite possible that the code that it tests (which had no tests up until that point) has a leak that we can now see because it's finally being exercised.
It's academic, it's making the only mac unit test box perma-orange, which is going to mask other failures, and condition people to check in on orange. Back out or disable the tests for now, and file a bug on the leak. We can't just leave it orange.
What about upping the allowed leak amount?  We *finally* have test coverage on some code that happens to trigger a leak in code too.

IMHO, it's just dumb to not allow a test to be checked in because it catches a leak.  We have coverage to prevent regressions, and we've clearly had this leak for a while - it's just now being triggered.
Attachment #330251 - Flags: review? → review?(rcampbell)
Just noting that running:
python runtests.py --chrome --test-path=../chrome/toolkit/mozapps/downloads/tests/chrome/test_unkownContentType_dialog_layout.xul --leak-threshold=8

and then exiting results in this outputting:
TEST-PASS | runtests-leaks | WARNING leaked 8 bytes during test execution
TEST-PASS | runtests-leaks | leaked 1 instance of nsStringBuffer with size 8 bytes
Attachment #330251 - Flags: review?(rcampbell) → review+
Checking in steps.py;
new revision: 1.8; previous revision: 1.7
Keep this open to track down the underlying leak, or file a new bug, your call.
Assignee: nobody → sdwilsh
Attached file refcnt leak balance tree (obsolete) —
I'd attach the log, but it's over a gig in size.  This is the balance tree - if it's useful to anyone.  I couldn't get any information out of it.
You need to pipe that output through fix-macosx-stack.pl for the trace to make any sense.
(In reply to comment #9)
> You need to pipe that output through fix-macosx-stack.pl for the trace to make
> any sense.
The wiki seems to be out of date then.  What steps do I need to do to get something useful?  Running that script just seems to print what is normally printed at the end of a test run.
So, assuming that first line is the command you ran, you'd do this:

make-tree.pl --object 0x195A3D50 < /Code/moz/central/mozilla/obj-i386-apple-darwin9.2.0/_tests/testing/mochitest/mochitesttestingprofile/leaks-report.log | fix-macosx-stack.pl
so the leak seems to originate from nsMIMEInfoBase::AppendExtension (commenting the AppendExtension call in nsInternetConfigService.mm line 345 fixes it).

I tried to replace "info->AppendExtension(temp);" by "info->AppendExtension(NS_LITERAL_STRING("foo"));" to check if there was something wrong with temp, but that doesn't help. I'm stuck at that point.
Attachment #330276 - Attachment is obsolete: true
In the steps from my dup, nsOSHelperAppService::GetMIMEInfoFromOS does

  byType->CopyBasicDataTo(byExt); 

with one extension ("exe") already in byExt.  Dunno why it does that, but nsCStringArray::operator= is clearly being buggy by leaking all the strings already in the destination array.  This fixes my dup and hopefully also fixes this bug.
Attachment #337182 - Flags: review?(benjamin)
Assignee: sdwilsh → jruderman
Severity: blocker → normal
Component: General → XPCOM
QA Contact: general → xpcom
Summary: qm-moz2mini01 leaking a nsStringBuffer → nsCStringArray::operator= leaks all strings that were in the destination array
Keywords: mlk
Attachment #337182 - Flags: review?(benjamin) → review+
We should reduce the OS X leak allowance back to zero when this lands.
Patch checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Jesse - what about comment 15?
mrbkap points out that the previous patch makes things worse in the self-assignment case.  This patch fixes that by bailing early for self-assignment.
Attachment #337981 - Flags: review?(benjamin)
I checked that in even though it doesn't have review yet, since it could be worse than the original leak bug.
Shawn, this did fix the leak on Mac unit test Tinderboxen :)  Can you reduce the leak allowance or explain how to do it (e.g. which repository to check into)?
Applying the reversal of attachment 330251 [details] [diff] [review] to cvs trunk (you may need to check out an additional module whose name escapes me).
Bug 448416 moved the leak threshold from tools/buildbotcustom/unittest/steps.py (which lives in CVS) to mozilla2-unittest/master.cfg (which lives in hg).

I hope it's ok for me to check in there, and I hope this works:

http://hg.mozilla.org/build/buildbot-configs/rev/9b1948464cf3
Should be fine, but the buildbot master will need to be reconfigured.
buildbot master reconfig'd to pick up the change, should take affect on the next cycle.
Attachment #337981 - Flags: review?(benjamin) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: