1010587 - set android:debuggable="true" #ifdef NIGHTLY_BUILD and MOZ_DEBUG

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, enhancement)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: set android:debuggable="true" #ifdef NIGHTLY_BUILD and MOZ_DEBUG

It'd be very helpful for debugging purposes to be able to install a build of Nightly with android:debuggable="true".

At the same time, nalexander notes that we don't want to distribute such builds to users, as the flag indicates that the app has no expectation of its data being private, and Nightly users would expect their browser's data to be private.

So here's a patch that sets android:debuggable="true" for MOZILLA_OFFICIAL builds only if both NIGHTLY_BUILD and MOZ_DEBUG are defined, which restricts the setting to builds like this one produced by tinderbox:

https://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-central-android-debug/latest/

And hopefully also the debug builds produced by this tryserver run in progress:

https://tbpl.mozilla.org/?tree=Try&rev=3b496d25cdad

Comment 1

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8422788 [details] [diff] [review]
patch v1: set android:debuggable="true" #ifdef NIGHTLY_BUILD and MOZ_DEBUG

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

If it's green on try, and if it does what you want locally, it's good for me.  mfinkle was on board, too.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Thanks for the quick review!  I'll confirm try results (including both debug and non-debug builds) before landing and post to m-f-d afterward about how I intend to use it to access the data of an existing Nightly installation on a device that isn't rooted (per <http://blog.shvetsov.com/2013/02/access-android-app-data-without-root.html>).

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8422788 [details] [diff] [review]
patch v1: set android:debuggable="true" #ifdef NIGHTLY_BUILD and MOZ_DEBUG

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

::: mobile/android/base/AndroidManifest.xml.in
@@ +76,5 @@
>                   android:name="org.mozilla.gecko.GeckoApplication"
>                   android:hardwareAccelerated="true"
>  #ifdef MOZILLA_OFFICIAL
> +#ifdef NIGHTLY_BUILD
> +#ifdef MOZ_DEBUG

You know, while Preprocessor.py is not very advanced, it does support #if defined(FOO) && defined(BAR)

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: more elegantly express conditional

(In reply to Mike Hommey [:glandium] from comment #3)
> You know, while Preprocessor.py is not very advanced, it does support #if
> defined(FOO) && defined(BAR)

Thanks, that's handy!  It makes this conditional more elegantly expressible.

https://tbpl.mozilla.org/?tree=Try&rev=e19197dbea1c

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8422813 [details] [diff] [review]
patch v2: more elegantly express conditional

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

Based on the #mobile backscroll, this is correct :)

Comment 6

[Nick Alexander :nalexander [he/him]]

For the benefit of future archaeologists: the preprocessor does not yet support arbitrary parentheses, so this cannot be parenthesized for clarity.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

The patch didn't work, as android:debuggable="true" for all the builds, including the optimized ones.  Digging into it…

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: add MOZ_DEBUG to DEFINES only if actually defined

Hmm, ok, this is a bug in Fennec's Makefile (or possibly a change to the preprocessor).

Fennec's Makefile unconditionally adds MOZ_DEBUG to its list of DEFINES:

  DEFINES += \
  …
    -DMOZ_DEBUG=$(MOZ_DEBUG) \
    $(NULL)

  - http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in?rev=a2a09fc0c39d#43

Which gets passed to the preprocessor via a -D flag that defines MOZ_DEBUG without specifying its value:

  python -m mozbuild.action.preprocessor … -DMOZ_DEBUG= …

Which causes the preprocessor's handleD function to set the context property MOZ_DEBUG to an empty string:

  def handleD(option, opt, value, parser):
      vals = value.split('=', 1)
      …
      self.context[vals[0]] = vals[1]

  - http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/preprocessor.py?rev=fe4eba8fc092#488

Which causes the preprocessor's *defined* lambda to evaluate defined(MOZ_DEBUG) to true:

  'defined': lambda tok: tok.value in context,

  - http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/preprocessor.py?rev=fe4eba8fc092#209

So MOZ_DEBUG is defined, as far as the preprocessor is concerned, for both the debug and optimized builds.  And |!defined(MOZILLA_OFFICIAL) || defined(NIGHTLY_BUILD) && defined(MOZ_DEBUG)| evaluates to True for them.

The fix is for Fennec's Makefile to add MOZ_DEBUG to DEFINES only if it's actually defined (like browser/installer/Makefile.in does).

Or possibly to make the preprocessor treat such declarations as undefined, although that seems like a stretch, given that the metavar for the -D flag is "VAR[=VAL]", so I would be more inclined to make the preprocessor raise an exception on a case like "-DVAR=", which doesn't match the declared syntax.

In any case, here's the patch with the fix to the Makefile and another try:

https://tbpl.mozilla.org/?tree=Try&rev=b7de30e9695a

Comment 9

[Myk Melez [:myk] [@mykmelez]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #8)
> In any case, here's the patch with the fix to the Makefile and another try:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=b7de30e9695a

It worked!  The debug build has android:debuggable="true", while the three opt builds have android:debuggable="false".

Comment 10

[Mike Hommey [:glandium]]

This is the same behaviour as the C preprocessor. If you want -DFOO= to work as if there is no -D, just #if FOO instead of #if defined(FOO)

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8423332 [details] [diff] [review]
patch v3: add MOZ_DEBUG to DEFINES only if actually defined

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

As the comment says, let's do this in moz.build, like we do for MOZILLA_OFFICIAL.  That will get rid of the -DMOZ_DEBUG= that you were seeing.

This will change a consumer: Assert.java, via DEBUG_BUILD, but in the right way.  Will need a try build, though.  No need for re-review if the try is green.

::: mobile/android/base/Makefile.in
@@ +43,3 @@
>    $(NULL)
>  
> +ifdef MOZ_DEBUG

I think MOZ_DEBUG should be added to the list at

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build#516

rather than handled in this Makefile.in.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: define MOZ_DEBUG in moz.build

(In reply to Nick Alexander :nalexander from comment #11)
> As the comment says, let's do this in moz.build, like we do for
> MOZILLA_OFFICIAL.  That will get rid of the -DMOZ_DEBUG= that you were
> seeing.

Sounds good, here's the patch.


> This will change a consumer: Assert.java, via DEBUG_BUILD, but in the right
> way.  Will need a try build, though.  No need for re-review if the try is
> green.

Here's the try run:

https://tbpl.mozilla.org/?tree=Try&rev=9558a06768b3

Comment 13

[Myk Melez [:myk] [@mykmelez]]

The try run is good; pushed to inbound:

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

Comment 14

[Phil Ringnalda (:philor)]

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