1285511 - Permafailure on beta tier-2 android frontend tests due to Adjust passing method count

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Carsten Book [:Tomcat]]

looks there are perma failure with the error 

 15:18:10 INFO - Execution failed for task ':app:transformClassesWithDexForAutomationDebug'. 

from https://treeherder.mozilla.org/logviewer.html#?job_id=1280226&repo=mozilla-beta#L2492

Geoff do you know whats going on here ?

Comment 1

[Geoff Brown [:gbrown]]

Similar perma-fail for Android 4.0 Deps, checkstyle, lint, and test jobs. Failing since these jobs starting running on beta, June 6.

https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&filter-searchStr=android&fromchange=cf6ec12bd62001b93387ffb184a8841644255b5e


We don't run these jobs on mozilla-aurora - curious.

And all is well on mozilla-central.

Comment 2

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #1)
> We don't run these jobs on mozilla-aurora - curious.

Sorry, that's wrong - they run on aurora and pass.

Comment 3

[Geoff Brown [:gbrown]]

09:33:28     INFO -  UNEXPECTED TOP-LEVEL EXCEPTION:
09:33:28     INFO -  com.android.dex.DexIndexOverflowException: Cannot merge new index 71572 into a non-jumbo instruction!
09:33:28     INFO -  	at com.android.dx.merge.InstructionTransformer.jumboCheck(InstructionTransformer.java:109)
09:33:28     INFO -  	at com.android.dx.merge.InstructionTransformer.access$800(InstructionTransformer.java:26)
09:33:28     INFO -  	at com.android.dx.merge.InstructionTransformer$StringVisitor.visit(InstructionTransformer.java:72)
09:33:28     INFO -  	at com.android.dx.io.CodeReader.callVisit(CodeReader.java:114)
09:33:28     INFO -  	at com.android.dx.io.CodeReader.visitAll(CodeReader.java:89)
09:33:28     INFO -  	at com.android.dx.merge.InstructionTransformer.transform(InstructionTransformer.java:49)
09:33:28     INFO -  	at com.android.dx.merge.DexMerger.transformCode(DexMerger.java:842)
09:33:28     INFO -  	at com.android.dx.merge.DexMerger.transformMethods(DexMerger.java:813)
09:33:28     INFO -  	at com.android.dx.merge.DexMerger.transformClassData(DexMerger.java:786)
09:33:28     INFO -  	at com.android.dx.merge.DexMerger.transformClassDef(DexMerger.java:682)
09:33:28     INFO -  	at com.android.dx.merge.DexMerger.mergeClassDefs(DexMerger.java:542)
09:33:28     INFO -  	at com.android.dx.merge.DexMerger.mergeDexes(DexMerger.java:171)
09:33:28     INFO -  	at com.android.dx.merge.DexMerger.merge(DexMerger.java:189)
09:33:28     INFO -  	at com.android.dx.command.dexer.Main.mergeLibraryDexBuffers(Main.java:502)
09:33:28     INFO -  	at com.android.dx.command.dexer.Main.runMonoDex(Main.java:334)
09:33:28     INFO -  	at com.android.dx.command.dexer.Main.run(Main.java:277)
09:33:28     INFO -  	at com.android.dx.command.dexer.Main.main(Main.java:245)
09:33:28     INFO -  	at com.android.dx.command.Main.main(Main.java:106)
09:33:28     INFO -   FAILED
09:33:28     INFO -  FAILURE: Build failed with an exception.
09:33:28     INFO -  * What went wrong:
09:33:28     INFO -  Execution failed for task ':app:transformClassesWithDexForAutomationDebug'.
09:33:28     INFO -  > com.android.build.api.transform.TransformException: com.android.ide.common.process.ProcessException: org.gradle.process.internal.ExecException: Process 'command '/home/worker/workspace/build/src/java_home/bin/java'' finished with non-zero exit value 2
09:33:28     INFO -  * Try:
09:33:28     INFO -  Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
09:33:28     INFO -  BUILD FAILED
09:33:28     INFO -  Total time: 1 mins 40.356 secs
09:33:29     INFO -  gmake[5]: *** [.gradle.deps] Error 1
09:33:29     INFO -  gmake[5]: *** Deleting file `.gradle.deps'
09:33:29     INFO -  gmake[5]: Leaving directory `/home/worker/workspace/build/src/obj-firefox/mobile/android/base'
09:33:29     INFO -  gmake[4]: *** [mobile/android/base/libs] Error 2
09:33:29     INFO -  gmake[4]: Leaving directory `/home/worker/workspace/build/src/obj-firefox'
09:33:29     INFO -  gmake[3]: *** [libs] Error 2
09:33:29     INFO -  gmake[3]: Leaving directory `/home/worker/workspace/build/src/obj-firefox'
09:33:29     INFO -  gmake[2]: *** [default] Error 2
09:33:29     INFO -  gmake[2]: Leaving directory `/home/worker/workspace/build/src/obj-firefox'
09:33:29     INFO -  gmake[1]: *** [realbuild] Error 2
09:33:29     INFO -  gmake[1]: Leaving directory `/home/worker/workspace/build/src'
09:33:29     INFO -  gmake: *** [build] Error 2
09:33:29     INFO -  0 compiler warnings present.

Comment 4

[Geoff Brown [:gbrown]]

Nick -- Do you know what's going wrong / how to fix this?

Comment 5

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

(In reply to Geoff Brown [:gbrown] from comment #4)
> Nick -- Do you know what's going wrong / how to fix this?

I know what's going wrong -- we're breaking the 64k index limit in the DEX process.  This is specific to the debug builds, since they don't Proguard before DEXing.  As for how to fix this, Gradle exposes a few approaches.  One is jumboDex; the other is multiDex.  Since this isn't actually shipping, we don't really care how we address this in the short term.  We could even enable Proguard in the debug build; in theory, that should work.  It might impact our lint pass, though.

I'd like to have mcomella or sebastian try to get one of these two working locally; if needed, I can investigate this.  (I expect just a little Gradle tweaking is needed, once we have the beta flags to include the extra code.  If needed, try adding some other large GPS libraries, just to balloon the DEX size so that you see the error.)

Comment 6

[Intermittent Failures Robot]

23 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-beta: 23

Platform breakdown:
* android-4-0-armv7-api15: 23

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1285511&startday=2016-07-04&endday=2016-07-10&tree=all

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Why are those builds so much larger (dex wise) then our normal builds? Locally I don't run into those issues?

There are multiple ways to fix that but all of them are somehow annoying; so I wonder if we could avoid this in the first place.

Comment 8

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

(In reply to Sebastian Kaspari (:sebastian) from comment #7)
> Why are those builds so much larger (dex wise) then our normal builds?
> Locally I don't run into those issues?
> 
> There are multiple ways to fix that but all of them are somehow annoying; so
> I wonder if we could avoid this in the first place.

I bet it is Adjust: https://dxr.mozilla.org/mozilla-central/source/mobile/android/confvars.sh?q=path%3Aandroid%2Fconfvars&redirect_type=single#55, which will reference a bunch of GPS ads garbage.  After that, you might compare AppConstants to see what else is compiled in.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Nick Alexander :nalexander from comment #8)
> (In reply to Sebastian Kaspari (:sebastian) from comment #7)
> > Why are those builds so much larger (dex wise) then our normal builds?
> > Locally I don't run into those issues?

I noticed we have multidex enabled for local builds:
  https://dxr.mozilla.org/mozilla-central/rev/88bebcaca249aeaca9197382e89d35b02be8292e/mobile/android/app/build.gradle#57

> I bet it is Adjust:

Interesting – I was never able to get Adjust building with gradle, but I'd also believe that Adjust is the cause.

(In reply to Nick Alexander :nalexander from comment #5)
> I know what's going wrong -- we're breaking the 64k index limit in the DEX
> process.  This is specific to the debug builds, since they don't Proguard
> before DEXing.

fwiw, the tier 2 test job is listed under opt, but I imagine we might accidentally run debug gradle builds in the "opt" label.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #9)
> fwiw, the tier 2 test job is listed under opt, but I imagine we might
> accidentally run debug gradle builds in the "opt" label.

The gradle task is "app:testAutomationDebugUnitTest":
  https://dxr.mozilla.org/mozilla-central/rev/88bebcaca249aeaca9197382e89d35b02be8292e/testing/mozharness/configs/builds/releng_sub_android_configs/64_test.py#9

So it is a debug build.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a961a102a55a

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88f2005fe330&selectedJob=23762134

I couldn't get the "test" job to work in this run or comment 11, but checkstyle & lint ran [1] and there is no crash – I think my patch, which enables multidex on automation, should fix the issue. I'll touch up the comments and post it.

[1]: Looking at treeherder, checkstyle & lint also permafail.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1285511 - Improve existing build flavor comments.

We went over the dex limit on the android "test" job so this is a quick fix -
see the code comments for further details.

Review commit: https://reviewboard.mozilla.org/r/64106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64106/

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Side concern: I believe multidex can cause performance issues so this is something to revisit if we use gradle for 1) production builds or 2) performance-oriented tests – bug 1286677.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8770724 [details]
Bug 1285511 - Improve existing build flavor comments.

https://reviewboard.mozilla.org/r/64106/#review61882

Mh, this should fix the build - but do we run this build? To get multidex support it seems like we need to either inherit MultiDexApplication or call MultiDex.install() at runtime:
https://developer.android.com/studio/build/multidex.html#mdex-gradle

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Sebastian Kaspari (:sebastian) from comment #15)
> Mh, this should fix the build - but do we run this build? To get multidex
> support it seems like we need to either inherit MultiDexApplication or call
> MultiDex.install() at runtime:

Looking at the docs, they do state this is required and I agree we should add this.

To explain why my test job passed, in my treeherder run (comment 12), the APK was not run on a device/emulator. Even if I could run the "Test" job, it also does not run the APK on device/emulator – it runs robolectric tests on desktop. So perhaps by adding this to only gradle, we successfully compile but would crash (or have unexpected behavior) at runtime.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Sebastian Kaspari (:sebastian) from comment #15)
> To get multidex
> support it seems like we need to either inherit MultiDexApplication or call
> MultiDex.install() at runtime:

I agree; calling MultiDex.install seems simpler.

Since we don't want to include the multidex support library in all of our builds, our goal: call `Multidex.install` in gradle builds [1] but not in "moz.build"s.

With my limited build knowledge, I see a few ways of doing this:
  a1) Perform pre-processing branching on which build system we're using
  a2) Include different files for a certain class implementation depending on which build system we're using
  a3) Include a file in one build system but not the other. Use reflection to access the class: we can tell which build system we're on based on whether on not it exists.

And some speculative alternatives:
  b1) BuildConfig is a gradle-specific class you can access in code. If we can get moz.build to ignore it or return a specific value (rather than refuse to compile), we'd know we're in a gradle build and we can branch in the code on that.
  b2) Use reflection to identify the build system, access BuildConfig, or something

---

a3 can cause performance issues at runtime (reflection at startup is particularly bad) so a1 & a2 are preferred. a1 is probably better for simplicity.

It looks like there is a build flag, MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE [2] which is set when "--with-gradle" is specified [3]. --with-gradle is currently specified for all of our tier 2 gradle builds (that I'm familiar with, at least) [4].

Given that most developers (if any) use `--with-gradle` currently, we won't be able to use this flag to add multidex to local builds but since that isn't an issue currently, I think we're okay to land it with preprocessing on MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE.

[1]: Preferably gradle builds with the automation flavor but let's start simple.
[2]: https://dxr.mozilla.org/mozilla-central/search?q=MOZ_BUILD_MOBILE_ANDROID&redirect=false
[3]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/gradle.configure#20
[4]: https://dxr.mozilla.org/mozilla-central/search?q=path%3Amobile%2Fandroid%2Fconfig%2Fmozconfigs+with-gradle&redirect=false

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92a381a03d4a

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d26a842da83a

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Push in comment 18 is for Nightly, comment 19 is for Beta – all green! :) (except that someone broke lint on Beta)

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1285511 - Add multidex support to automation builds.

I initially tested my code with a branch around the multidex dependency:

if (mozconfig.substs.MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE) {
  compile ...multidex...
}

I removed it because it seemed unnecessary - the code shouldn't be
included if it's not referenced.

I tested:
 * Locally with a build that did not exceed the method limit, pre-Lollipop
 * On try, with fx-team
 * On try, with beta

Review commit: https://reviewboard.mozilla.org/r/65714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65714/

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8770724 [details]
Bug 1285511 - Improve existing build flavor comments.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64106/diff/1-2/

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8770724 [details]
Bug 1285511 - Improve existing build flavor comments.

https://reviewboard.mozilla.org/r/64106/#review63116

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8773073 [details]
Bug 1285511 - Add multidex support to automation builds.

https://reviewboard.mozilla.org/r/65714/#review63122

::: mobile/android/base/AppConstants.java.in:358
(Diff revision 1)
> +    public static void maybeInstallMultiDex(final Context context) {
> +//#ifdef MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE
> +        if (BuildConfig.FLAVOR.equals("automation")) {
> +            MultiDex.install(context);
> +        }
> +//#else
> +        // Do nothing.
> +//#endif

It's a bit weird to have a method in a class name  xxxConstants, but then again we are doing this in AdjustConstants.java.in too:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AdjustConstants.java.in

Not really worth having yet another *.java.in file

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/04533ff743455eb7ecdf1aa5fcf4f9935bf54d12
Bug 1285511 - Improve existing build flavor comments. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/6972bef6693c9a78d5e7bb45722fd4990d517f1d
Bug 1285511 - Add multidex support to automation builds. r=sebastian

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8770724 [details]
Bug 1285511 - Improve existing build flavor comments.

This request applies to all changesets push in this bug.

Approval Request Comment
[Feature/regressing bug #]: All Java code added between the current Beta release and the previous.
[User impact if declined]: This only affects our builds with gradle on Beta (and presumably release), which are not deployed to users. However, these builds run our checkstyle, lint, and unit tests so users could, in theory, get a less reliable build if we make changes that break these tests on the Beta tree.

[Describe test coverage new/current, TreeHerder]: Pushed to try on fx-team tip and beta tip
[Risks and why]: Low – we write minimal code that can affect production builds so it's unlikely we'll cause problems. It's more likely this fix won't stop the permafailure and we'll have to do more work to fix it. However, if the defines don't work as expected, in the worse case, we'd be including the `MultiDex.install` line in production code. However, I don't think we include it the multidex support library as a dependency so I'd expect this not to compile.
[String/UUID change made/needed]: None

Comment 27

[Intermittent Failures Robot]

13 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-beta: 13

Platform breakdown:
* android-4-0-armv7-api15: 13

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1285511&startday=2016-07-18&endday=2016-07-24&tree=all

Comment 28

[Gerry Chang [:gchang]]

Hi Michael,
Given this is late beta and we are about to have RC, can we let this ride the train on 49?

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/04533ff74345
https://hg.mozilla.org/mozilla-central/rev/6972bef6693c

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Gerry Chang [:gchang] from comment #28)
> Hi Michael,
> Given this is late beta and we are about to have RC, can we let this ride
> the train on 49?

The end result of this bug is that our unit tests are permafailing on Beta but given that we won't be making many changes to Beta (or release, when the trees transfer), I'm okay letting this ride the trains.

Given that my push to Beta passed (comment 18 or 19 – one of them is to fx-team), it's unlikely that we'll have unit test failures for Beta. I'll send out an email to notify the larger group.

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #30)
> Given that my push to Beta passed (comment 18 or 19 – one of them is to
> fx-team), it's unlikely that we'll have unit test failures for Beta. I'll
> send out an email to notify the larger group.

To be more explicit: it's unlikely that we currently have unit test failures on Beta (since they'd be obscured by the permafailures).

Comment 32

[Gerry Chang [:gchang]]

Comment on attachment 8770724 [details]
Bug 1285511 - Improve existing build flavor comments.

This patch fixes permafailure, however, it's too late for beta. Let's let it ride the trains.

Comment 33

[Jan Henning [:JanH]]

Just to note that this is breaking localOldDebug builds for me (see bug 1289585).

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2d8d82f4786b
https://hg.mozilla.org/releases/mozilla-aurora/rev/fb657c8ddd97