minidumps not being generated from robocop test crashes (breakpad environment variables not being set properly)

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

unspecified
Firefox 19
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

I found a build that was crashing on m-c this weekend in the robocop tests.  I downloaded it and installed it on my tegra.  This crashes and generates a .dmp crash report.  

The problem is the CrashReporter activity is starting up and we are moving the reports to the default profile/pending folder.  ack!!  This shouldn't be happening when we send in MOZ_CRASHREPORTER_NO_REPORT=1 to the environment variable.  Here is what we are sending to sutagent:
FIRE PROC: ' "MOZ_CRASHREPORTER_SHUTDOWN=1,MOZ_CRASHREPORTER_NO_REPORT=1,NO_EM_RESTART=1" am instrument -w -e deviceroot /mnt/sdcard/tests -e class org.mozilla.fennec.tests.testCheck3 org.mozilla.roboexample.test/org.mozilla.fennec.FennecInstrumentationTestRunner'


It could be that since we are launching a process indirectly that we don't respect the environment variables.
Yeah I'm pretty sure those environment variables never make it to gecko. If you want specific things set in Gecko's environment you'll have to make sure it gets set in GeckoAppShell.setupGeckoEnvironment. Currently that code sets env variables that it picks out of the intent used to launch fennec.

See also https://wiki.mozilla.org/Mobile/Fennec/Android#Arguments_and_Environment_Variables
this seems to be a specific problem with our robocop based tests since we launch them another way via am instrument.  Our normal program launching sets the env vars properly as intent args
Summary: when we have a crash on android, it appears we don't collect any data from it → minidumps not being generated from robocop test crashes (breakpad environment variables not being set properly)
and this makes sense because we launch fennec from robocop:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/BaseTest.java.in#67

Solution is to do what we do for the profile and push it down via robotium.config.
this patch solves the robocop.apk and mochitest part of this.  I will post another patch which will fix the talos bits.
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Attachment #669949 - Flags: review?(bugmail.mozilla)
this adds the environment variables for robocop based talos tests.
Attachment #669951 - Flags: review?(jhammel)
Comment on attachment 669949 [details] [diff] [review]
allow for environment variables to be passed to robotium.config (1.0)

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

Minusing for the comma issue, but looks ok otherwise.

::: mobile/android/base/tests/BaseTest.java.in
@@ +67,5 @@
>          i.putExtra("args", "-no-remote -profile " + mProfile);
>  
> +        String envString = (String)config.get("envvars");
> +        if (envString != "") {
> +            String [] envStrings = envString.split(",");

nit: remove space before []

::: testing/mochitest/runtestsremote.py
@@ +395,5 @@
> +        fHandle.write("rawhost=http://%s:%s/tests\n" % (options.remoteWebServer, options.httpPort))
> +
> +        if browserEnv:
> +            envstr = ','.join(['%s=%s' % (str(key), str(value))
> +                                   for key, value in browserEnv.items()])

What if one of the values has a comma character in it? I think the options here are (1) test for a comma in the value and report an error if there is one or (2) write out each env var as a separate entry to the config file.

@@ +403,5 @@
> +        self._dm.removeFile(os.path.join(deviceRoot, "robotium.config"))
> +        self._dm.pushFile(fHandle.name, os.path.join(deviceRoot, "robotium.config"))
> +        os.unlink(fHandle.name)
> +
> +    def buildBrowserEnv(self, options):

Where is this called from?
Attachment #669949 - Flags: review?(bugmail.mozilla) → review-
Comment on attachment 669951 [details] [diff] [review]
print the environment from talos to robotium.config (1.0)

+            envstr = ','.join(['%s=%s' % (str(key), str(value))
+                                   for key, value in browser_config.get('env', {}).items()])

No need to do str(foo) if you're interpolating with '%s' which automagically does this.  Also the indentation on the second line is a bit weird.

+            fHandle.write("envvars=%s\n" % envstr)

Since you're using '=' to deliminate the env variables, maybe e.g. ':' would be better here?

r+ with nits addressed
Attachment #669951 - Flags: review?(jhammel) → review+
address the review comments.
Attachment #669949 - Attachment is obsolete: true
Attachment #670094 - Flags: review?(bugmail.mozilla)
Comment on attachment 670094 [details] [diff] [review]
allow for environment variables to be passed to robotium.config (2.0)

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

LGTM with the error message fixed

::: testing/mochitest/runtestsremote.py
@@ +399,5 @@
> +            delim = ""
> +            for key, value in browserEnv.items():
> +                try:
> +                    value.index(',')
> +                    print "Found: Error an ',' in our value, unable to process value."

Found <-> Error
Attachment #670094 - Flags: review?(bugmail.mozilla) → review+
I know you r+'d this before, this latest change now puts the code in sync with what is r+'d for desktop and will land first thing in the morning.
Attachment #669951 - Attachment is obsolete: true
Attachment #670508 - Flags: review?(jhammel)
Comment on attachment 670508 [details] [diff] [review]
print the environment from talos to robotium.config (2.0)

+                try:
+                    value.index(',')
+                    print "Error: Found an ',' in our value, unable to process value."

Um....shouldn't you check what the value is before printing the error?
Attachment #670508 - Flags: review?(jhammel) → review-
the idea is that value.index will throw an exception when we don't find a ','.  This means that if we find a ',' we have an error with the value and will not process it.  Otherwise will be be generating much more code to handle commas inside the value such as "ENV1=a,b,c,ENV2=jhammel,ENV3=jmaher".
Comment on attachment 670508 [details] [diff] [review]
print the environment from talos to robotium.config (2.0)

Ah, tricky and clever. I'll go with it, though I would have coded it differently.

I'm guessing that having a line per environment variable isn't an option here?
Attachment #670508 - Flags: review- → review+
it is an option, although with the simple parser we have in robocop it would make it more complex.  Let me add a comment to explain it a bit more and indicate the limitation and other options if we choose to enhance our robotium.config parsing inside of robocop.
(I've also usually used ';'s to delimit env variables in the past, but of course that is no better nor worse than ',', really)
http://hg.mozilla.org/build/talos/file/25032f0721d5

I landed the bits on inbound this morning and backed them out.  Both of these need to be landed in parallel.  This also means if we need to deploy talos on Aurora, we should update robocop on aurora as well.
Depends on: 801633
yeah, I am no cool
Attachment #671874 - Flags: review?(wlachance)
Comment on attachment 671874 [details] [diff] [review]
use talos variables instead of mochitest variables (1.0)

I didn't verify, but this looks more correct to me. ;)
Attachment #671874 - Flags: review?(wlachance) → review+
https://hg.mozilla.org/mozilla-central/rev/b05c80fb621f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
You need to log in before you can comment on or make changes to this bug.