691425 - allow for reftest/mochitest android crashes to be dumped in the logs just like desktop crashes

Status: RESOLVED FIXED

Description

[Joel Maher ( :jmaher ) (UTC-4)]

Created attachment 564270 [details] [diff] [review]
collect minidumps from the remote device (1.0)

on mobile automation we don't collect minidumps from the device upon a crash.  This patch allows us to pull minidumps from the device and use the existing tools to parse them.

In order for this to work we need to:
* download the symbols with the build
* unpack the symbols and adjust the command lines to have a --symbols=<path>
* download the minidump_stackwalk application from: http://hg.mozilla.org/build/tools/file/tip/breakpad/
* set MINIDUMP_STACKWALK environment variable to point to the location of the downloaded binary.

Comment 2

[Joel Maher ( :jmaher ) (UTC-4)]

Created attachment 564303 [details] [diff] [review]
collect minidumps from the remote device talos (1.0)

same patch but for talos.  I tested this and I couldn't find valid crashes while testing, so I have to assume this work.  

Luckily in our talos framework we have the minidump_stackwalk binary, we just need to set the symbols path during remotePerfConfigurator.py.

Comment 3

[William Lachance (:wlach) (use needinfo!)]

Comment on attachment 564303 [details] [diff] [review]
collect minidumps from the remote device talos (1.0)


>+        minidumpdir = os.path.join(profile_dir, 'minidumps')
>+        if ((browser_config['remote'] == True) and (browser_config['host'] <> '')):

I'm a bit confused about the check for browser_config['host'] here. You don't necessarily have one in the case of devicemanagerADB (or at least, we don't currently fail earlier if you don't give one). I see that we're doing this check elsewhere, so I guess at least this is consistent with that. Still, it makes me uneasy.

(my personal opinion is that we should allow browser_config['host'] to be None/'' and just not expect it)

>+            dumpdir = tempfile.mkdtemp()
>+            self._ffprocess.testAgent.getDirectory(profile_dir + '/minidumps/', dumpdir)
>+            minidumpdir = dumpdir

Two more things:

1. AFAICT I don't think you need the 'dumpdir' temporary here. You should just be able to assign straight to minidumpdir and use that.
2. We should probably at least print a warning if getDirectory fails (returns None), since otherwise it will just appear in the output as if we succeeded but couldn't find any stack traces.

>             utils.noisy("Found crashdump: " + dump)
>             if browser_config['symbols_path']:
>+                print "%s" % ' '.join([stackwalkbin, dump, browser_config['symbols_path']])

Did you mean to include this or is it just a debugging statement left in by mistake? I guess if we want to include it we should use utils.debug or something to print it out (as well as describing what it is).

Comment 4

[Joel Maher ( :jmaher ) (UTC-4)]

Created attachment 564933 [details] [diff] [review]
collect minidumps from the remote device talos (1.1)

updated to address feedback.  As per the getDirectory() call returning a warning, I decided not to do that.  The main reason is that directory will be empty except when we have a minidump.  No need to have more messages flooding the log file that have nothing to do with how the test is run.  If there is an internal error getting the directory, the devicemanager will print an error.

Comment 5

[William Lachance (:wlach) (use needinfo!)]

Comment on attachment 564933 [details] [diff] [review]
collect minidumps from the remote device talos (1.1)

The fixups look good to me. I guess you're probably right wrt the error thing with getDirectory. I wish we could disambiguate errors vs. not being able to find anything, but perhaps a solution to that is out of scope of this bug (and probably isn't that big a deal, realistically speaking).

Comment 6

[cmtalbert]

Comment on attachment 564270 [details] [diff] [review]
collect minidumps from the remote device (1.0)

The only comment I have here is that shutil.rmtree will throw if it encounters any error when trying to remove that directory.  On a linux-y OS I wouldn't expect to hit any errors.  On windows, you could hit it since any access of a file in that directory will lock the directory.  So you might want a try/catch around that. But be sure to output a message saying that we're not removing the file as not removing the file enough times could run a foopy out of disk space.

Since the foopies aren't windows boxes, this is not a huge concern for automation, but the try/catch might be nice since we never know what OS developers have.

r+ with nit

Comment 7

[Joel Maher ( :jmaher ) (UTC-4)]

landed on talos:
http://hg.mozilla.org/build/talos/rev/433f070d1868

Comment 8

[Joel Maher ( :jmaher ) (UTC-4)]

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

Comment 9

[Ed Morley [:emorley]]

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