http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242121695.1242126019.22966.gz&fulltext=1 Linux mozilla-central unit test on 2009/05/12 02:48:15 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242121695.1242126557.23988.gz&fulltext=1 OS X 10.5.2 mozilla-central unit test on 2009/05/12 02:48:15 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242121695.1242128385.26439.gz&fulltext=1 WINNT 5.2 mozilla-central unit test on 2009/05/12 02:48:15 No bug. http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242126510.1242132002.31348.gz&fulltext=1 Linux mozilla-central unit test on 2009/05/12 04:08:30 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242126510.1242132126.31532.gz&fulltext=1 OS X 10.5.2 mozilla-central unit test on 2009/05/12 04:08:30 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242126510.1242133322.760.gz&fulltext=1 WINNT 5.2 mozilla-central unit test on 2009/05/12 04:08:30 Each test leaks 3 nsStringBuffer. Regression timeframe: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-05-12+02%3A45%3A14&enddate=2009-05-12+04%3A05%3A31 1 changeset: bug 483062

Interesting, must be leaking in the crashreporter code itself? I guess xpcshell doesn't cleanup the same way the normal app code does.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090531 Minefield/3.6a1pre] (mozilla-central-win32-unittest/1243808976) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/ab395a1916be) At least, this confirms the leak comes from the crash reporter :-) This workaround un-regresses the leak detection results, at the cost of stopping the CR early (ftb) :-| Someone (else) should "debug" the crashreporter leak (cause) itself. ***** Fwiw, http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/unit/test_crashreporter.js explicitly stops the CR. Should it reenable it at its end?

Comment on attachment 380851 [details] [diff] [review] (Av1) Add |crashReporter.enabled = false;| workaround I don't think we want to do this, since we won't catch shutdown crashes then. In fact, we should make test_crashreporter.js re-enable the crashreporter at the end so we can get a stack for bug 488596.

*head.js functions do not have a text param: use dump(). *Backup and restore crashreporter state, per previous comments. *Do serverURL and minidumpPath need this too? *Add 2 "missing" checks. *Get rid of |var u|.

Comment on attachment 380991 [details] [diff] [review] (Bv1) Fix test_crashreporter.js Seems like overkill. The test harness enables crash reporting if possible, and if it's not available, this test won't run anyway.

Bv1, with comment 6 suggestion(s), plus test the whole API.

Ping for review: also wanted for bug 488596.

Comment on attachment 382324 [details] [diff] [review] (Bv2) Fix and extend test_crashreporter.js + ////////// + // Check behavior when disabled. + ////////// nit: use block-style comments /**/ here (and elsewhere) + // (Ftr, crashreporter was enabled by </testing/xpcshell/head.js>.) I'd just make this say "crash reporting is enabled by default in ... " - for (var i=0; i<testspecs.length; i++) { - var u = ios.newURI(testspecs[i], null, null); - cr.serverURL = u; + for (var i = 0; i < testspecs.length; ++i) { Do you really need to change the coding style here for no particular reason? The comment changes in the IDL seem sort of unrelated. Why are they lumped in here? r=me, although I'd rather not have unrelated things tossed in here.

Also, related to the original topic of this bug, the leak, it's likely to just be the fact that we're not calling UnsetExceptionHandler before exiting xpcshell: http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#657 Those variables are defined here: http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#140

(In reply to comment #8) > - for (var i=0; i<testspecs.length; i++) { > - var u = ios.newURI(testspecs[i], null, null); > - cr.serverURL = u; > + for (var i = 0; i < testspecs.length; ++i) { > > Do you really need to change the coding style here for no particular reason? Simple nits: more readable, ... > The comment changes in the IDL seem sort of unrelated. Why are they lumped in > here? Only because I checked the IDL to find out "all" the cases the test had to cover... ("API coverage" == related, from my p-o-v.)

(In reply to comment #10) > I checked the IDL to find out "all" the cases the test had to > cover... ("API coverage" == related, from my p-o-v.) Though test_crashreporter.js test and TestCrashReporterAPI.cpp seem partially redundant (now) :-|

(In reply to comment #9) > the leak [...] likely to just be the fact that > we're not calling UnsetExceptionHandler before exiting xpcshell: Iiuc, this would need to be added somewhere in xpcshell.cpp: after 1747 result = ProcessArgs(cx, glob, argv, argc); probably just before 1769 // no nsCOMPtrs are allowed to be alive when you call NS_ShutdownXPCOM 1770 rv = NS_ShutdownXPCOM( NULL ); and the code would be +/- like { #ifdef MOZ_CRASHREPORTER // Don't care about the returned error if cr was not enabled. CrashReporter::UnsetExceptionHandler(); #endif } Right?

(In reply to comment #11) > Though test_crashreporter.js test and TestCrashReporterAPI.cpp seem partially > redundant (now) :-| We should just remove the latter now that we have the former. It doesn't test as much anyway. Write a patch? (In reply to comment #12) > Iiuc, this would need to be added somewhere in xpcshell.cpp: Write a patch for this as well? Need an XPCOM peer to review it, though. Also, I don't think it's quite that simple, I'm not sure that xpcshell can be built against headers from toolkit. You might need to grab the crashreporter service by its contract ID and call SetEnabled(false).

Comment on attachment 384438 [details] [diff] [review] (Bv2a) Fix and extend test_crashreporter.js [Checkin: Comment 14] http://hg.mozilla.org/mozilla-central/rev/2fc9cfd06219

(In reply to comment #13) > (In reply to comment #11) > > Though test_crashreporter.js and TestCrashReporterAPI.cpp seem partially > > redundant (now) :-| > > We should just remove the latter now that we have the former. I'll do that in bug 474688. > It doesn't test as much anyway. Write a patch? {{ // Defined in nsExceptionHandler.cpp, but not normally exposed namespace CrashReporter { bool GetAnnotation(const nsACString& key, nsACString& data); } run_test(test_set_minidump_path); run_test(test_annotate_crash_report_basic); run_test(test_annotate_crash_report_invalid_equals); run_test(test_annotate_crash_report_invalid_cr); run_test(test_appendnotes_crash_report); }} All is covered, except checking the resulting values (with GetAnnotation()). Will we miss that? {{ run_test(test_init_exception_handler); run_test(test_unset_exception_handler); }} I assume this is covered by enabling/disabling the cr.

(In reply to comment #15) > All is covered, except checking the resulting values (with GetAnnotation()). > Will we miss that? I don't think so, since that test isn't running now anyway. I plan on writing an xpcshell test that actually crashes at some point, so we can test more thoroughly. > {{ > run_test(test_init_exception_handler); > run_test(test_unset_exception_handler); > }} > > I assume this is covered by enabling/disabling the cr. Correct, setting .enabled={true,false} runs the same code.

Comment on attachment 384519 [details] [diff] [review] (Cv1) test_crashreporter.js: synchronize from TestCrashReporterAPI.cpp [Checkin: Comment 17] http://hg.mozilla.org/mozilla-central/rev/0520eb581850

Ftr, the leak is reported by Firefox only: (local) SeaMonkey and (tinderbox) Thunderbird don't have it, "related" to { INFO | test_crashreporter.js | Can't test crashreporter in a non-libxul build. }

Per comment 13. This fixes this bug, though I'm somewhat confused by the code and its comments: 1) |} // this scopes the nsCOMPtrs| Fwiw, it doesn't matter if I add my fix inside or after that block. Then I'm not sure why NS_ShutdownXPCOM() is not inside that block, as NS_InitXPCOM2() is... I would assume it's to auto-release the pointers, but see the following. 2) |// no nsCOMPtrs are allowed to be alive when you call NS_ShutdownXPCOM| This (code and) comment has always (in hg history) been there. Yet, bug 470971 added { 1587 nsCOMPtr<nsILocalFile> appFile; 1593 nsCOMPtr<nsIFile> appDir; 1782 appDir = nsnull; 1783 appFile = nsnull; } "outside" of the Init/Shutdown area... (And I assume it does work that way.) 3) Fwiw, if I move my fix to (just) after NS_ShutdownXPCOM() then that fixes bug 484747 too... (Though i did not even tried to understand why/how.) To summarize: I have a working fix code (thanks to Ted), but (I know little about all this and) I'm not sure where/why to place it.

Comment on attachment 393291 [details] [diff] [review] (Dv1) xpcshell.cpp: call crashReporter->SetEnabled(PR_FALSE) I'm skeptical: won't this cause us to not catch shutdown crashes in xpcshell, which I suspect are going to be pretty common? If we do this at all, it ought to be after the NS_ShutdownXPCOM call

(In reply to comment #1) > I guess xpcshell doesn't cleanup the same way the normal app code does. What would be the closer xpcshell (or the xpcshell-tests harness) can be to what the app does? (In reply to comment #20) > I'm skeptical: won't this cause us to not catch shutdown crashes in xpcshell, I would assume so... (But) How else could we avoid the leak? > If we do this at all, it ought to be after the NS_ShutdownXPCOM call That's comment 19 point 3, so no problem. (Even if I don't understand the details of this code.)

I mentioned this on IRC, but XRE_Main just calls the crashreporter methods directly from C++: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2795 http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#3512 xpcshell can't do this because it uses external linkage. The alternate approach here would be to figure out how to make the leak logging code ignore these strings, since we're only allocating them once, so it's ok to leak them.

This time, I tested Debug too: http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/shell/xpcshell.cpp?mark=1775-1793#1770 Before NS_ShutdownXPCOM(), Optimized: Fixes this bug (only), but can't catch crash during NS_ShutdownXPCOM() or later. Before NS_ShutdownXPCOM(), Debug: Debug doesn't have this bug: "Can't test crashreporter in a non-libxul build." (In reply to comment #20) > If we do this at all, it ought to be after the NS_ShutdownXPCOM call Before NS_LogTerm(), Optimized: Fixes this bug (and bug 484747), but can't catch crash during NS_LogTerm or later. Before NS_LogTerm(), Debug: Iiuc, do_GetService() (eventually tries to) re-initializes XPCOM, and I get "###!!! ASSERTION: nsTHashtable::Init() should not be called twice.: 'Error', file objdir\dist\include\nsTHashtable.h, line 327" then it looks like this code can not live after NS_ShutdownXPCOM(). (In reply to comment #22) > XRE_Main just calls the crashreporter methods directly from C++ > [...] > xpcshell can't do this because it uses external linkage. http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp?mark=3495-3498,3510-3513#3492 To be explicit (iiuc), the issue is not calling CrashReporter::UnsetExceptionHandler() or crashReporter->SetEnabled(PR_FALSE), but being able to call the cr (from xpcshell) without needing xpcom. And the latter is not possible, is it? Iiuc, nsAppRunner starts and stops the cr, and xpcom is active in between. Whereas xpcshell (tests) starts the cr from head.js which is executed in between the xpcom start and stop. Catch-22?! > The alternate approach here would be to figure out how to make the leak logging > code ignore these strings, since we're only allocating them once, so it's ok to > leak them. To tell/teach that code which object leak (number) to ignore seems complicated (just for this case)... The obvious workaround would be to set a leak threshold in the harness, but that would (probably) still mean getting the leak dump for each test :-/ Afaict, the only clean solution is the patch D (or patch A!), but it means no shutdown stack... Benjamin, Ted, how do we solve of this? (As this bug is blocking me from adding the leak error report on xpcshell tests :-<)

(In reply to comment #23) > > The alternate approach here would be to figure out how to make the leak logging > > code ignore these strings, since we're only allocating them once, so it's ok to > > leak them. > > To tell/teach that code which object leak (number) to ignore seems complicated > (just for this case)... Unless there would be a mean to mark the strings themselves to not be accounted for in the leak stats at all. Is there one?

To be specific, you should get the crashreporter service before you shut down XPCOM. Then, after you shut down XPCOM, call SetEnabled(PR_FALSE).

Comment on attachment 393291 [details] [diff] [review] (Dv1) xpcshell.cpp: call crashReporter->SetEnabled(PR_FALSE) r- based on bsmedberg's comment, but mostly right.

Dv1, with comment 25 suggestion(s): I thought I had tried that too for comment 23 :-/ I'm not sure if |if (crashReporter)| is required (but it can't hurt at least). I'm even more confused now by "// no nsCOMPtrs are allowed to be alive when you call NS_ShutdownXPCOM"...

Comment on attachment 395461 [details] [diff] [review] (Dv2) xpcshell.cpp: call crashReporter->SetEnabled(PR_FALSE) [Checkin: See comment 29 & 31] + // Prevent a '3 nsStringBuffer' leak report with an enabled cr. + // But won't catch crashes past this point :-| I wouldn't worry about the "won't catch crashes" bit, there's almost no code executed past this point. I would change this comment to read simply: // Shut down the crashreporter service to prevent leaking some strings it holds

Comment on attachment 395461 [details] [diff] [review] (Dv2) xpcshell.cpp: call crashReporter->SetEnabled(PR_FALSE) [Checkin: See comment 29 & 31] http://hg.mozilla.org/mozilla-central/rev/56240b40cf08 Dv2, with comment 28 suggestion(s), plus some more after irc with benjamin. http://hg.mozilla.org/mozilla-central/rev/86b19b98b4b5 (Ev1) xpcshell.cpp: s/null/nsnull/

V.Fixed, per tinderboxes

Comment on attachment 395461 [details] [diff] [review] (Dv2) xpcshell.cpp: call crashReporter->SetEnabled(PR_FALSE) [Checkin: See comment 29 & 31] http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f3a3c0fea83c + http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6eda775737cb