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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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

Comment 2

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

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

Comment 3

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

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.

Comment 4

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

Attachment: allow for environment variables to be passed to robotium.config (1.0)

this patch solves the robocop.apk and mochitest part of this.  I will post another patch which will fix the talos bits.

Comment 5

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

Attachment: print the environment from talos to robotium.config (1.0)

this adds the environment variables for robocop based talos tests.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 7

[Jeff Hammel]

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

Comment 8

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

Attachment: allow for environment variables to be passed to robotium.config (2.0)

address the review comments.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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

Comment 10

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

Attachment: print the environment from talos to robotium.config (2.0)

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.

Comment 11

[Jeff Hammel]

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?

Comment 12

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

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 13

[Jeff Hammel]

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?

Comment 14

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

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.

Comment 15

[Jeff Hammel]

(I've also usually used ';'s to delimit env variables in the past, but of course that is no better nor worse than ',', really)

Comment 16

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

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.

Comment 17

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

Attachment: use talos variables instead of mochitest variables (1.0)

yeah, I am no cool

Comment 18

[William Lachance (:wlach)]

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. ;)

Comment 19

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

http://hg.mozilla.org/build/talos/rev/8960d6a0b2c2

Comment 20

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

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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