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

RESOLVED FIXED

Status

RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [android][tegra][mobile_unittests][inbound])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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.
Attachment #564270 - Flags: review?(ctalbert)
Duplicate of this bug: 685860
Assignee: nobody → jmaher
(Assignee)

Comment 2

7 years ago
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.
Attachment #564303 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #564303 - Flags: review? → review?(wlachance)
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).
(Assignee)

Comment 4

7 years ago
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.
Attachment #564303 - Attachment is obsolete: true
Attachment #564303 - Flags: review?(wlachance)
Attachment #564933 - Flags: review?(wlachance)
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).
Attachment #564933 - Flags: review?(wlachance) → review+

Comment 6

7 years ago
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
Attachment #564270 - Flags: review?(ctalbert) → review+
(Assignee)

Comment 8

7 years ago
inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c15691c7ff
Whiteboard: [android][tegra][mobile_unittests] → [android][tegra][mobile_unittests][inbound]

Comment 9

7 years ago
https://hg.mozilla.org/mozilla-central/rev/a1c15691c7ff
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 757838

Updated

5 years ago
Component: Infrastructure → General
You need to log in before you can comment on or make changes to this bug.