Closed Bug 483062 Opened 11 years ago Closed 11 years ago

figure out how to get crash stacks from xpcshell tests

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: ted, Assigned: ted)

References

Details

(Whiteboard: [.js test parts wanted on m-1.9.1: depend on bug 484033] [fixed1.9.1.2: comments 25 & 27; fixed1.9.1.4: Gv1-191])

Attachments

(2 files, 2 obsolete files)

bsmedberg implemented local breakpad processing in automation.py in bug 481732. This works for Mochitest/Reftest because they both run the browser, so Breakpad "just works". xpcshell doesn't actually call xre_main, so it doesn't install the exception handler, so no Breakpad love. It's still got all the crashreporter code available though, so we should figure out how to make it work.

The unfortunate part (that I just realized) is that the singleton that implements nsICrashReporter is the xulAppInfo singleton, which isn't available in xpcshell. We could:
1) Add some methods to nsICrashReporter such that you can do everything that the nsAppRunner code does in xre_main (basically expose all of: http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.h)
then:
a) move the implementation of that interface to a service in the crashreporter code, off of the xulAppInfo singleton
or
b) fix bug 475965 (which would be nice for other reasons), so we could just get at the existing implementation from xpcshell.

Then, once this is done, the xpcshell test harness could simply get the crash reporter service, set the exception handler, and check for minidumps on crash (and process them, if MINIDUMP_STACKWALK is set in the environment and a symbol path was passed in) just like automation.py does now. (Maybe we could share the code, even!)
(In reply to comment #0)
> a) move the implementation of that interface to a service in the crashreporter
> code, off of the xulAppInfo singleton
> or
> b) fix bug 475965 (which would be nice for other reasons), so we could just get
> at the existing implementation from xpcshell.

After actually re-reading bug 475965 and thinking about it, these steps aren't really necessary. The singleton is already available in xpcshell in libxul builds, and the harness can easily detect this and only enable crashreporting in those situations, which is just fine as our unit test builds are libxul builds anyway. I think I got confused because I tried this on my x86-64 Linux box, and we don't have Breakpad on x86-64. Duh. This should be as simple as fixing step 1) from my previous comment.
Depends on: 484033
I've basically implemented all of part 1 in bug 484033. Should be straightforward to make the xpcshell harness use that to generate minidumps on crash, and the Python harness look for and process them like automation.py does now.
Assignee: nobody → ted.mielczarek
Status: NEW → ASSIGNED
This works for me. The patch is primarily refactoring. I added an automationutils.py module that is shared between automation.py and runxpcshelltests.py, and moved the checkForCrashes function into there. There's just one bit in this patch that I wouldn't land with:
+# To pickup automationutils.py
+#XXX: this is ugly
+PYTHONPATH = $(topsrcdir)/build
+export PYTHONPATH

I chatted with bsmedberg about this on IRC, but we didn't come to a conclusion as to how to allow runxpcshelltests.py to import that module cleanly.
Attachment #369119 - Flags: review?(benjamin)
Comment on attachment 369119 [details] [diff] [review]
get stacktraces from xpcshell tests

Might as well toss it in your queue, despite not wanting to commit this version because of the PYTHONPATH bit.
Comment on attachment 369119 [details] [diff] [review]
get stacktraces from xpcshell tests

>-          log.info("TEST-UNEXPECTED-FAIL | (automation.py) | Missing 'kill' utility to kill process with pid=%s. Kill it manually!", pid)
>+          log.info("TEST-UNEXPECTED-FAIL | automation.py | Missing 'kill' utility to kill process with pid=%s. Kill it manually!", pid)

You may (or not) drop these changes, as I plan to change this temporary "file"  format anyway in bug 469518.
Version: unspecified → Trunk
Comment on attachment 372442 [details] [diff] [review]
Use pythonpath.py instead of PYTHONPATH in the environment, rev. 1

+while True:
+    try:
+        arg = sys.argv[1]

Any reason not to use OptionParser here? Is it just too heavyweight to bother with?

I realize we're not using this extensively, but do you think the second process launch will be an issue on Windows?
Attachment #372442 - Flags: review?(ted.mielczarek) → review+
* I didn't use OptionParser because it tries to eat flags in the [args...] bits:

pythonpath.py -I foo -I bar script.py -I baz whatever

* there isn't another process... "execfile" is a python internal function which reads and executes a script as if it were the main script.
Ah, I didn't know that! The only caveat I see then is that if you call it like that, the script inherits your locals/globals:
http://docs.python.org/library/functions.html#execfile
ok, this passes an explicit globals dict which avoids leaking variables
Attachment #372442 - Attachment is obsolete: true
Comment on attachment 372460 [details] [diff] [review]
Use pythonpath.py instead of PYTHONPATH in the environment, rev. 1.1
[Checkin: Comment 12 & 25]

Looks good. I'll refresh my other patch here to use this.
Attachment #372460 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/862693caa320 with a totally bogus checking bug# :-(
Attachment #369119 - Flags: review?(benjamin)
Uses pythonpath.py. I had to tweak that script to add __file__ to globals.
Attachment #369119 - Attachment is obsolete: true
Attachment #373191 - Flags: review?(benjamin)
Attachment #373191 - Flags: review?(benjamin) → review+
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/6461b3507d98

Should take this on 1.9.1 to keep the test harnesses in sync (and also for its usefulness) once we're assured that it's working well on trunk.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Although now that I say that, I realize that this depends on changes to nsICrashReporter, which will be a PITA to port to branch (but doable).
And another followup bustage fix because it fell down in parallel builds:
http://hg.mozilla.org/mozilla-central/rev/1825128515b7
Whiteboard: [Would want this on 1.9.1, if possible]
Target Milestone: --- → mozilla1.9.2a1
Yet another followup to fix running tests on packaged builds:
http://hg.mozilla.org/mozilla-central/rev/bacdfbdea382
Depends on: 495730
I landed just the pythonpath.py bits of this on 1.9.1 in order to port another patch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0ca2f203b8b0
I would probably like to take the automationutils bits on branch as well, just to keep the harnesses closer in sync, even if we don't actually get this whole bug fixed there (because of the interface changes).
Could you land the (empty) automationutils.py part on m-1.9.1?
Then I could use this file in bug 469523.
Feel free to land whatever you want from this bug on 1.9.1. We just can't land bug 484033 directly, we would need to make a branch interface to avoid interface changes.
I'm not so sure what parts of the build changes is needed just for automationutils.py. That's why I was asking you to land whatever is not "crash related"...
This entire patch, minus the head.js parts. I can look into landing it, but it won't be soon.
Comment on attachment 372460 [details] [diff] [review]
Use pythonpath.py instead of PYTHONPATH in the environment, rev. 1.1
[Checkin: Comment 12 & 25]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/290b443109e1
Attachment #372460 - Attachment description: Use pythonpath.py instead of PYTHONPATH in the environment, rev. 1.1 → Use pythonpath.py instead of PYTHONPATH in the environment, rev. 1.1 [Checkin: Comment 12 & 25]
(In reply to comment #19)
> I landed just the pythonpath.py bits of this on 1.9.1 in order to port another
> patch:
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0ca2f203b8b0

To be explicit, that link was from bug 501657 comment 7,
and this bug check-in was:

(In reply to comment #25)
> (From update of attachment 372460 [details] [diff] [review])
> 
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/290b443109e1
Attachment #373191 - Attachment description: get stacktraces from xpcshell tests, rev 2 → get stacktraces from xpcshell tests, rev 2 [Checkin: See comment 14+16+17+18]
Depends on: 483856
Comment on attachment 373191 [details] [diff] [review]
get stacktraces from xpcshell tests, rev 2
[Checkin: See comment 14+16+17+18 & 27+28]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b00c583f2adc
Port just the pythonpath.py bits [...]
Attachment #373191 - Attachment description: get stacktraces from xpcshell tests, rev 2 [Checkin: See comment 14+16+17+18] → get stacktraces from xpcshell tests, rev 2 [Checkin: See comment 14+16+17+18 & 27]
Comment on attachment 373191 [details] [diff] [review]
get stacktraces from xpcshell tests, rev 2
[Checkin: See comment 14+16+17+18 & 27+28]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2a662f1193a4
(Gv1-191) All but bug 484033 related parts
Attachment #373191 - Attachment description: get stacktraces from xpcshell tests, rev 2 [Checkin: See comment 14+16+17+18 & 27] → get stacktraces from xpcshell tests, rev 2 [Checkin: See comment 14+16+17+18 & 27+28]
Flags: in-testsuite-
Whiteboard: [Would want this on 1.9.1, if possible] → [.js test parts wanted on m-1.9.1: depend on bug 484033] [fixed1.9.1.2: comments 25 & 27; fixed1.9.1.4: Gv1-191]
No longer depends on: 483856
Depends on: 513195
You need to log in before you can comment on or make changes to this bug.