889076 - use the profile dir to store minidumps in the crashreporter xpcshell tests

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use the profile dir

still need to figure out the test in unit_ipc. The others work fine.

Comment 2

[Mihnea Dobrescu-Balaur (:mihneadb)]

:jdm, maybe you know how to fix this?

Comment 3

[Josh Matthews [:jdm]]

Could you clarify what's not working and what you're trying to achieve?

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

I want to run all xpcshell tests concurrently. For the crashreporter tests, this meant changing the path where the minidump is stored to the profile dir, since every test has its own profile dir, while cwd is the same for all of them.

This change works for all the tests except for the one in unit_ipc,

Comment 5

[Josh Matthews [:jdm]]

And how does the failure manifest for the tests in unit_ipc?

Comment 6

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: log of the failure in unit_ipc

:jdm, I attached the log.

Comment 7

[Josh Matthews [:jdm]]

Ok, so the symptom is that no minidump is found in the profile directory (the log helpfully obscures this due to bug 890677).

Comment 8

[Mihnea Dobrescu-Balaur (:mihneadb)]

Right, and it has something to do with the fact that this is run in a new process, I guess. It is the only difference, the others work.

You can just apply the patch from bug 887054 and try it yourself.

Comment 9

[Josh Matthews [:jdm]]

Interestingly enough, when I pasted the "get the profile dir" snippet into a random xpcshell test that runs in a content process, it threw an error because profd is redeclared. Try renaming it and see if anything changes?

Comment 10

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use the profile dir

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 772367 [details] [diff] [review]
use the profile dir

Review of attachment 772367 [details] [diff] [review]:
-----------------------------------------------------------------

I kind of wish we had a do_get_tempdir() that returned the same directory but didn't set up the profile directory service provider, since tests like these don't actually need it, but it's not actively harmful.

::: toolkit/crashreporter/test/unit/crasher_subprocess_head.js
@@ +6,5 @@
> +// get the profile dir
> +let env = Components.classes["@mozilla.org/process/environment;1"].getService(Components.interfaces.nsIEnvironment);
> +let file = Components.classes["@mozilla.org/file/local;1"].createInstance(Components.interfaces.nsILocalFile);
> +file.initWithPath(env.get("XPCSHELL_TEST_PROFILE_DIR"));
> +let _profd = file;

You could just s/file/profd/ and drop the second variable assignment.

Comment 12

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use the profile dir

Comment 13

[Mihnea Dobrescu-Balaur (:mihneadb)]

Comment on attachment 772816 [details] [diff] [review]
use the profile dir

updated

Comment 14

[Mihnea Dobrescu-Balaur (:mihneadb)]

try run: https://tbpl.mozilla.org/?tree=Try&rev=b9ab7fd28b73

Comment 15

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use the new do_get_tempdir method in head.js

Not sure if I should also make this change in the crashreporter tests themselves.
It seems like it's enough to do it in the xpcshell/head.js file.

Comment 16

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #15)
> Created attachment 773471 [details] [diff] [review]
> use the new do_get_tempdir method in head.js
> 
> Not sure if I should also make this change in the crashreporter tests
> themselves.
> It seems like it's enough to do it in the xpcshell/head.js file.

Actually we don't even have access to head.js in crasher_subprocess_head.js.

Comment 17

[(not currently active) Ted Mielczarek]

Yeah, not really worthwhile, the subprocess isn't actually running xpcshell tests, it's just there to crash.

Comment 18

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: changed to the new do_get_tempdir() implementation

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 773556 [details] [diff] [review]
changed to the new do_get_tempdir() implementation

Review of attachment 773556 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks!

Comment 20

[Mihnea Dobrescu-Balaur (:mihneadb)]

https://tbpl.mozilla.org/?tree=Try&rev=c6e5a016e740

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f64c241a4f2f

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f64c241a4f2f

Comment 23

[(not currently active) Ted Mielczarek]

I missed it in review, but this patch broke getting crash stacks for xpcshell tests :-( I pushed a fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/971beabe0237

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/971beabe0237

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/70b5b81a8707