Closed Bug 1285511 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox48 --- wontfix
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: cbook, Assigned: mcomella)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

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 ?
Flags: needinfo?(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.
Flags: needinfo?(gbrown)
See Also: → 1255926
Assignee: nobody → 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.
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.
Nick -- Do you know what's going wrong / how to fix this?
Flags: needinfo?(nalexander)
(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.)
Flags: needinfo?(s.kaspari)
Flags: needinfo?(nalexander)
Flags: needinfo?(michael.l.comella)
Assignee: gbrown → nobody
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.
(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.
(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.
(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.
Assignee: nobody → michael.l.comella
Flags: needinfo?(michael.l.comella)
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.
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/
Attachment #8770724 - Flags: review?(s.kaspari)
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.
Flags: needinfo?(s.kaspari)
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
Attachment #8770724 - Flags: review?(s.kaspari) → review+
(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.
(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
Push in comment 18 is for Nightly, comment 19 is for Beta – all green! :) (except that someone broke lint on Beta)
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/
Attachment #8770724 - Attachment description: Bug 1285511 - Enable multidex in automation builds. → Bug 1285511 - Improve existing build flavor comments.
Attachment #8773073 - Flags: review?(s.kaspari)
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/
Attachment #8770724 - Flags: review+ → review?(s.kaspari)
Comment on attachment 8770724 [details]
Bug 1285511 - Improve existing build flavor comments.

https://reviewboard.mozilla.org/r/64106/#review63116
Attachment #8770724 - Flags: review?(s.kaspari) → review+
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
Attachment #8773073 - Flags: review?(s.kaspari) → review+
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
Attachment #8770724 - Flags: approval-mozilla-beta?
Attachment #8770724 - Flags: approval-mozilla-aurora?
Hi Michael,
Given this is late beta and we are about to have RC, can we let this ride the train on 49?
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/mozilla-central/rev/04533ff74345
https://hg.mozilla.org/mozilla-central/rev/6972bef6693c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
(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.
Flags: needinfo?(michael.l.comella)
(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 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.
Attachment #8770724 - Flags: approval-mozilla-beta?
Attachment #8770724 - Flags: approval-mozilla-beta-
Attachment #8770724 - Flags: approval-mozilla-aurora?
Attachment #8770724 - Flags: approval-mozilla-aurora+
Depends on: 1289585
Just to note that this is breaking localOldDebug builds for me (see bug 1289585).
Summary: Permafailure on beta tier-2 android frontend tests → Permafailure on beta tier-2 android frontend tests due to passing method count
Summary: Permafailure on beta tier-2 android frontend tests due to passing method count → Permafailure on beta tier-2 android frontend tests due to Adjust passing method count
You need to log in before you can comment on or make changes to this bug.