xpcshell-tests: each test leaks 3 nsStringBuffer, after (harness) bug 483062 landing

VERIFIED FIXED in mozilla1.9.3a1

Status

Testing
XPCShell Harness
--
major
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

(Blocks: 1 bug, {mlk, regression})

Trunk
mozilla1.9.3a1
mlk, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed1.9.2b1], URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Comment 2

9 years ago
Created attachment 380851 [details] [diff] [review]
(Av1) Add |crashReporter.enabled = false;| workaround

[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?
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #380851 - Flags: review?(ted.mielczarek)
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.
Attachment #380851 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Updated

9 years ago
Blocks: 488596
Keywords: regression
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 4

9 years ago
Created attachment 380991 [details] [diff] [review]
(Bv1) Fix test_crashreporter.js

*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|.
Attachment #380991 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

9 years ago
Blocks: 484033
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.
Attachment #380991 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 6

9 years ago
Created attachment 382324 [details] [diff] [review]
(Bv2) Fix and extend test_crashreporter.js

Bv1, with comment 6 suggestion(s),
plus test the whole API.
Attachment #380991 - Attachment is obsolete: true
Attachment #382324 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 7

9 years ago
Ping for review: also wanted for bug 488596.
Attachment #382324 - Flags: review?(ted.mielczarek) → review+
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
(Assignee)

Comment 10

9 years ago
Created attachment 384438 [details] [diff] [review]
(Bv2a) Fix and extend test_crashreporter.js
[Checkin: Comment 14]

(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.)
Attachment #382324 - Attachment is obsolete: true
Attachment #384438 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 11

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

Comment 12

9 years ago
(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).
Attachment #384438 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 14

9 years ago
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
Attachment #384438 - Attachment description: (Bv2a) Fix and extend test_crashreporter.js → (Bv2a) Fix and extend test_crashreporter.js [Checkin: Comment 14]
(Assignee)

Comment 15

9 years ago
Created attachment 384519 [details] [diff] [review]
(Cv1) test_crashreporter.js: synchronize from TestCrashReporterAPI.cpp
[Checkin: Comment 17]

(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.
Attachment #384519 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

9 years ago
Whiteboard: [ToDo: comment 13 CPP part]
Attachment #384519 - Flags: review?(ted.mielczarek) → review+
(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.
(Assignee)

Comment 17

9 years ago
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
Attachment #384519 - Attachment description: (Cv1) test_crashreporter.js: synchronize from TestCrashReporterAPI.cpp → (Cv1) test_crashreporter.js: synchronize from TestCrashReporterAPI.cpp [Checkin: Comment 17]
(Assignee)

Comment 18

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

Comment 19

9 years ago
Created attachment 393291 [details] [diff] [review]
(Dv1) xpcshell.cpp: call crashReporter->SetEnabled(PR_FALSE)

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.
Attachment #380851 - Attachment is obsolete: true
Attachment #393291 - Flags: review?(benjamin)

Comment 20

8 years ago
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
Attachment #393291 - Flags: review?(benjamin) → review?(ted.mielczarek)
(Assignee)

Comment 21

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

Comment 23

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

Comment 24

8 years ago
(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?

Comment 25

8 years ago
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.
Attachment #393291 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 27

8 years ago
Created attachment 395461 [details] [diff] [review]
(Dv2) xpcshell.cpp: call crashReporter->SetEnabled(PR_FALSE)
[Checkin: See comment 29 & 31]

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"...
Attachment #393291 - Attachment is obsolete: true
Attachment #395461 - Flags: review?(ted.mielczarek)
Attachment #395461 - Flags: review?(benjamin)
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
Attachment #395461 - Flags: review?(ted.mielczarek)
Attachment #395461 - Flags: review?(benjamin)
Attachment #395461 - Flags: review+
(Assignee)

Comment 29

8 years ago
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/
Attachment #395461 - Attachment description: (Dv2) xpcshell.cpp: call crashReporter->SetEnabled(PR_FALSE) → (Dv2) xpcshell.cpp: call crashReporter->SetEnabled(PR_FALSE) [Checkin: See comment 29]
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [ToDo: comment 13 CPP part] → [c-n: comment 29 to m-1.9.2]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.3a1
(Assignee)

Comment 30

8 years ago
V.Fixed, per tinderboxes
Status: RESOLVED → VERIFIED
(Assignee)

Comment 31

8 years ago
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
Attachment #395461 - Attachment description: (Dv2) xpcshell.cpp: call crashReporter->SetEnabled(PR_FALSE) [Checkin: See comment 29] → (Dv2) xpcshell.cpp: call crashReporter->SetEnabled(PR_FALSE) [Checkin: See comment 29 & 31]
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [c-n: comment 29 to m-1.9.2] → [fixed1.9.2b1]
You need to log in before you can comment on or make changes to this bug.