619204 - Make use of mozcrash to add handling of crash reports to Mozmill

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Jeff Hammel]

After the landing of bug 616383, we disable crashreporter via the
environment variable MOZ_CRASHREPORTER_NO_REPORT . We should figure
out what we really want to do with crashreports for mozmill.

For what mochitests does, see:

http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#576

http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/mochitest/test_crash_submit.xul

http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/mochitest/test_crash_notify_no_report.xul

Ideally, we should converge on a set of code and methodology that may
be reused by all (python) harnesses

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

For some of our upcoming projects that would have a good value. Looking forward.

Comment 2

[cmtalbert]

We should absolutely report minidump stacks from the stackwalker

Comment 3

[cmtalbert]

This unfortunately won't make 2.0.  We'll look into this later.

Comment 4

[Jeff Hammel]

see also: https://bugzilla.mozilla.org/show_bug.cgi?id=675688

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is something we REALLY need. Sadly too late for 2.0 but it should be on our list for 2.1! For example see bug 898194 which is a crash only happening while accessing FTP sites through Squid and the new background thumbnail generation. That was hard to track down.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

If possible I would really like to have it for 2.1. Lets see if we can get this done the next days.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We hit this problem a lot in the past couple of days, that Mozmill cannot inform us about crash reports. It causes everyone from us to manually isntall Firefox on a test node and check for that. To not waste our time we should at least print out if Firefox has been crashed. We can use the mozcrash package for that. I will have a look into this.

If we will have it ready for 2.0.6, we should include the feature even in a limited form.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So it seems like that the patch on bug 616383 is not always working. At least for me on Linux I still see the crash reporter coming up. Exporting the MOZ_CRASHREPORTER_NO_REPORT before starting mozmill fixes it. So most likely the env variable is not correctly passed to Firefox.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So the problem is that the environment variable in mozmill itself gets set after the creation of the runner class. That's why the variable is not inherited and doesn't have any effect. It really has to be set before the runner instantiation.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, so the full mozcrash feature I will not implement for Mozmill 2.0.6. There seem to be more work necessary as what we have time for the 2.0.6 release. So what I would like to do now is to:

* Restore the functionality of mozmill to disable the crash reporter
* If a crash happened the generated minidump files will be printed to the console

That's already an improvement to the situation as we have right now, with nothing working. Ted, quick question to you, is there a way to let the crash reporter silently send reports? I would love to not print out the minidump files but the crash report URLs instead. Or is that not possible?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So I talked with Ted and we figured out the best way would be to get the symbols via ftp.m.o and not download them via the symbol server. The latter symbols are for debuggers and way too large. Sadly, there is no crashreporter-symbols.zip file existent for nightly builds, but only for tinderbox builds. So I filed bug 973901 to get this mapping in place.

That means until bug 973901 is fixed we might want to have a lightweight solution, which can at least print out the minidump files if those are around. That would be an indication of a crash. In a follow-patch we can then analyze the stack and print it out instead.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

With the upcoming patch we will see something like that:

TEST-START | /mozilla/code/mozmill/test1.js | setupModule
TEST-START | /mozilla/code/mozmill/test1.js | testNewPageLoaded
PROCESS-CRASH | __init__.py | application crashed [Unknown top frame]
Crash dump filename: /home/henrik/.mozilla/firefox/1068yp1k.mozmill/minidumps/3b93eb8e-7710-3b80-6849973d-350e71b0.dmp
No symbols path given, can't process dump.
MINIDUMP_STACKWALK not set, can't process dump.
mozcrash INFO | Saved dump as /home/henrik/.mozilla/firefox/Crash Reports/pending/3b93eb8e-7710-3b80-6849973d-350e71b0.dmp

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v1

This will add preliminary support for handling crashes which should be enough for Mozmill 2.0.6. A follow-up patch will be necessary for the final fix. I will contribute that for 2.1.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I will file a new bug to get the minidump stackwalk to work. This will block the 2.1 release, and will also get a test.

Comment 15

[cmtalbert]

Comment on attachment 8377653 [details] [diff] [review]
Patch v1

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

This looks fine as long as those environment variables are set for each process restart - I can't tell if that is the case from the context of the diff. Generally, if mozmill is running a process these env variables should always be set.

r+ with that looked into.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v2

Andrew, would you mind reviewing the interdiff? The following parts have been updated:

* We check for crashes whenever we have a disconnect. Given that mozmill can restart the browser it wasn't a good place in stop().
* We correctly add a failing frame to the results array with the right message, so that the crash can also be seen in the report.
* Make use of mozcrash 0.12 which will be released in a bit

Comment 17

[Andrew Halberstadt [:ahal]]

Comment on attachment 8379055 [details] [diff] [review]
Patch v2

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

::: mozmill/mozmill/__init__.py
@@ +38,5 @@
>  ADDONS = [extension_path, jsbridge.extension_path]
>  JSBRIDGE_TIMEOUT = 60.
>  
> +# Environment variables
> +ENVIRONMENT = {

Why is this a global variable? I think we should avoid globals unless you plan to import this from another file for some reason.

@@ +44,5 @@
> +    'MOZ_CRASHREPORTER_NO_REPORT': '1',
> +    # Disable the crash reporter for Gnome
> +    'GNOME_DISABLE_CRASH_DIALOG': '1',
> +    # Disable the crash reporter for Windows
> +    'XRE_NO_WINDOWS_CRASH_DIALOG': '1',

I've never heard of these two env variables. They shouldn't be necessary if MOZ_CRASHREPORTER_NO_REPORT is set, please remove them.

@@ +482,4 @@
>  
>          return self.results
>  
> +    def check_for_crashes(self):

Delete this function and call self.runner.check_for_crashes() instead. You'll need to pass symbols_path into the the runner's constructor once it's available. If you need the save_path thing, I'm fine with adding it as an optional parameter to mozrunner.check_for_crashes()

@@ +494,5 @@
> +            save_path = tempfile.mkdtemp()
> +
> +        test = getattr(self, "running_test", {})
> +        return mozcrash.check_for_crashes(minidump_path,
> +                                          None,  # We cannot handle symbols right now

Not having symbols means having completely useless stack traces.. but I assume you know that.

@@ +523,4 @@
>              self.runner.wait(timeout=self.jsbridge_timeout)
>  
>      def report_disconnect(self, message=None):
> +        if self.check_for_crashes():

self.runner.check_for_crashes()

@@ +573,5 @@
>              self.start_runner()
>              self.stop_runner()
>  
> +        # Check for remaining crashes
> +        self.check_for_crashes()

self.runner.check_for_crashes()

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andrew Halberstadt [:ahal] from comment #17)
> @@ +38,5 @@
> >  ADDONS = [extension_path, jsbridge.extension_path]
> >  JSBRIDGE_TIMEOUT = 60.
> >  
> > +# Environment variables
> > +ENVIRONMENT = {
> 
> Why is this a global variable? I think we should avoid globals unless you
> plan to import this from another file for some reason.

We are using this in two different places. Once in the Mozmill (API) class and in the Mozmill CLI class. So I don't want to have to declare the required env variables twice.

> > +    'MOZ_CRASHREPORTER_NO_REPORT': '1',
> > +    # Disable the crash reporter for Gnome
> > +    'GNOME_DISABLE_CRASH_DIALOG': '1',
> > +    # Disable the crash reporter for Windows
> > +    'XRE_NO_WINDOWS_CRASH_DIALOG': '1',
> 
> I've never heard of these two env variables. They shouldn't be necessary if
> MOZ_CRASHREPORTER_NO_REPORT is set, please remove them.

I have taken this from the automationutils.py file, which is also using those environment variables. If those are not necessary why they have been put into this module?

http://mxr.mozilla.org/mozilla-central/source/build/automationutils.py#447

> > +    def check_for_crashes(self):
> 
> Delete this function and call self.runner.check_for_crashes() instead.
> You'll need to pass symbols_path into the the runner's constructor once it's
> available. If you need the save_path thing, I'm fine with adding it as an
> optional parameter to mozrunner.check_for_crashes()

Oh, I haven't known that mozrunner has this method. I will check that and file a follow-up bug to get the save_path param added. This is important for us. Also I cannot remove this method from Mozmill as you suggest here, given that we also have some custom code to run (save_path, and test values) before we can actually check for crashes.

> > +            save_path = tempfile.mkdtemp()
> > +
> > +        test = getattr(self, "running_test", {})
> > +        return mozcrash.check_for_crashes(minidump_path,
> > +                                          None,  # We cannot handle symbols right now
> 
> Not having symbols means having completely useless stack traces.. but I
> assume you know that.

Yes, see my previous comments on this bug. There is no easy way to retrieve symbols for Nightly and Aurora builds from ftp.m.o. I filed bug 973901. Also we are waiting for auto-submission of crash reports via bug 974649. So handling symbols and getting the stack is work to be done in bug 973978.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v3

Updated patch for the changes in mozrunner. Just to note, this patch cannot be landed before the next version of mozrunner has been released.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v3.1

Included the mozrunner 5.35 dependency in setup.py

Comment 21

[Andrew Halberstadt [:ahal]]

(In reply to Henrik Skupin (:whimboo) [away 02/24 - 02/28] from comment #18)
> > > +    'MOZ_CRASHREPORTER_NO_REPORT': '1',
> > > +    # Disable the crash reporter for Gnome
> > > +    'GNOME_DISABLE_CRASH_DIALOG': '1',
> > > +    # Disable the crash reporter for Windows
> > > +    'XRE_NO_WINDOWS_CRASH_DIALOG': '1',
> > 
> > I've never heard of these two env variables. They shouldn't be necessary if
> > MOZ_CRASHREPORTER_NO_REPORT is set, please remove them.
> 
> I have taken this from the automationutils.py file, which is also using
> those environment variables. If those are not necessary why they have been
> put into this module?
> 
> http://mxr.mozilla.org/mozilla-central/source/build/automationutils.py#447

I don't know. All I know is that setting MOZ_CRASHREPORTER_NO_REPORT disables the craspreporter dialog. None of the other test harnesses have them set, and they work fine across platforms.

Comment 22

[Andrew Halberstadt [:ahal]]

Comment on attachment 8379662 [details] [diff] [review]
Patch v3.1

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

I would still remove those environment variables, but if you really want to leave them I won't argue :)

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, I have removed the additional env variables. We will see how it works out. If we have problems with the system wide crash reporters, we will can add them back.

https://github.com/mozilla/mozmill/commit/fb1d59df9138817c426f85b4e52f232eb447a506 (master)
https://github.com/mozilla/mozmill/commit/8d42c5f6444f69fdcb8c992299c816be7442e434 (hotfix-2.0)

Andrei, can you or someone else please have additional eyes on this feature and verify it over the curse of the next week? For testing you want to install the Nightly Tester Tools which have the crashme feature available. Just have a test with a longer sleep for verifying this.

Comment 24

[Andrei Eftimie]

This is what I've seen with manually induced crashes using Nightly Tester Tools.

An example crash recorded in the console while running a mozmill test:
> 2014-02-27 10:27:43.387 firefox[2533:303] Mozilla has caught an Obj-C exception [OMGWTF: Hay guise!]
> PROCESS-CRASH | /Users/andrei.eftimie/work/mozilla/bugs/619204/mozmill-tests/firefox/tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test2.js | application crashed [Unknown top frame]
> Crash dump filename: /var/folders/9l/sn_p3bw914s360j602z20jsc0000gq/T/tmpjVPr3t.mozrunner/minidumps/A5ACF6F8-C5D0-41F5-9444-C50F9D8F01AD.dmp
> No symbols path given, can't process dump.
> MINIDUMP_STACKWALK not set, can't process dump.
> mozcrash INFO | Saved dump as /Users/andrei.eftimie/Library/Application Support/Firefox/Crash Reports/pending/A5ACF6F8-C5D0-41F5-9444-C50F9D8F01AD.dmp

A couple things stand out.

1) The dump is always empty. Both the minidump and the crash report dump.
Going later in about crashes, I can't click on the crashes generated with NTT while running under mozmill.
If I crash it manually with NTT from outside of mozmill this works as expected.

Henrik, is this expected?


2) [enhancement] The message "PROCESS-CRASH" is white. While we now have most messages color coded, we should paint this red

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andrei Eftimie from comment #24)
> 1) The dump is always empty. Both the minidump and the crash report dump.

What do you mean with empty? The minidump file should not have a size of 0 bytes. Also I don't see that the .extra file gets saved. You might want to re-run setup_development to update all packages.

> Going later in about crashes, I can't click on the crashes generated with
> NTT while running under mozmill.

You should not even see those crashes on that page for whatever reason. That's still something we have to figure out or get the stack walking fixed (probably mozmill 2.1). For now you could run the crashreporter binary directly with the minidump as argument.

> 2) [enhancement] The message "PROCESS-CRASH" is white. While we now have
> most messages color coded, we should paint this red

The output is done by mozcrash and not by ourself, so there is no way to colorize the output. We would have to refactor mozcrash first, so that consumers can output custom lines.

Comment 26

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> (In reply to Andrei Eftimie from comment #24)
> > 1) The dump is always empty. Both the minidump and the crash report dump.
> 
> What do you mean with empty? The minidump file should not have a size of 0
> bytes. Also I don't see that the .extra file gets saved. You might want to
> re-run setup_development to update all packages.

I've retested today with a new venv and it is working as expected. I have the .extra file as well.

> > Going later in about crashes, I can't click on the crashes generated with
> > NTT while running under mozmill.
> 
> You should not even see those crashes on that page for whatever reason.
> That's still something we have to figure out or get the stack walking fixed
> (probably mozmill 2.1). For now you could run the crashreporter binary
> directly with the minidump as argument.

This is interesting. I've done another crash. I see it in about:crashes, I've clicked on it and it correctly submitted it to crash-stats: https://crash-stats.mozilla.com/report/index/9827de36-e0ed-4958-b7ea-f0ff72140304

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Good to hear that all is working now! Also it is interesting that you can see the reports and submit them. Not sure if that is a glitch on my Linux machine, or only Linux is affected here. We will see once the code is out and we have to do those steps on our production machines.