708114 - Disable Android StrictMode for release and beta builds

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Chris Peterson [:cpeterson]]

1. We currently enable StrictMode for ALL builds. We should probably enable it only for nightly and local builds. We can add a StrictMode flag to AndroidManifest.xml or check for channel != release or beta.

2. We currently select the default VmPolicy, which checks for nothing. We should select the strict VmPolicy, which detects leaks of SQLite cursors, Activities, and other closable objects.

3. Our current StrictMode policy is to just log a warning to logcat. We should consider ThreadPolicy penaltyDialog() and VmPolicy penaltyDeath(). These penalties won't affect user experience too badly if we only enable StrictMode for nightly and local builds.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: 708114-part1-add-enableStrictMode-method.patch

This patch comments out StrictMode (so it is NOT enabled in released builds). It also updates the StrictMode policy flags to include more checks.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: 708114-part2-add-StrictMode-flag.patch

This patch adds an "enableStrictMode" flag to Fennec's branding resources. This allows us to automatically ENABLE StrictMode for local and Nightly builds, but DISABLE it for Aurora, Beta, and Official builds.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 580557 [details] [diff] [review]
708114-part1-add-enableStrictMode-method.patch

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

looks fine.  a nit and a clarification.  Please post another patch.

::: mobile/android/base/GeckoApp.java
@@ +1425,5 @@
> +	/**
> +	 * Enable Android StrictMode checks (for supported OS versions).
> +	 * http://developer.android.com/reference/android/os/StrictMode.html
> +	 */
> +    private void enableStrictMode()

Align the comments with the start of the method.  For example:

/**
 * Enable Android StrictMode checks (for supported OS versions).
 * http://developer.android.com/reference/android/os/StrictMode.html
 */

private void enableStrictMode()

@@ +1440,5 @@
> +
> +        StrictMode.setVmPolicy(new StrictMode.VmPolicy.Builder()
> +                               .detectAll()
> +                               .penaltyLog()
> +                                // TODO: Uncomment after bug 709330 and bug 709331 are fixed! // .penaltyDeath()

I am not sure we ever want to use penaltyDeath().  I would find that highly annoying.  Any reason to just not ignore this?

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 580560 [details] [diff] [review]
708114-part2-add-StrictMode-flag.patch

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

I don't like the file name |bools.xml|.  Maybe defaults.xml or prefs.xml?  The point is that the value type isn't what is important.  What is important is that it changes our running configuration (so prefs or defaults kinda makes sense).

Looks good.  nit, rename bools, plus the makefile stuff.  Put up another patch for final review.

::: mobile/android/base/GeckoApp.java
@@ +1257,4 @@
>              mLastScreen = savedInstanceState.getByteArray(SAVED_STATE_SCREEN);
>          }
>  
> +        // Only enable StrictMode for nightly and local builds.

This comment may not always be true -- like in the case if we change those bools.xml files.  Also, the reader of this code really doesn't need to know anything about the release targets.  Lets change the comment to reflect that.

For example:

// Enable strict mode if |enableStrictMode| is set is resources

Or something...

::: mobile/android/base/Makefile.in
@@ +429,4 @@
>  	$(NSINSTALL) -D res/layout-v11
>  	$(NSINSTALL) $(srcdir)/resources/layout-v11/* res/layout-v11/
>  
> +$(RES_VALUES): $(subst res/,$(srcdir)/resources/,$(RES_VALUES)) $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/res/values/bools.xml

Not sure why we need this change.  Why is this different from the styles.xml for or the other |values| resources

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: 708114-part1-add-enableStrictMode-method-v2.patch

* Align comments.
* Remove penaltyDeath().

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: 708114-part2-add-StrictMode-flag-v2.patch

* Rename bools.xml branding.xml.
* Declare RES_VALUES' dependencies explicitly.

Comment 7

[Chris Peterson [:cpeterson]]

@dougt:

> I don't like the file name |bools.xml|.  Maybe defaults.xml or prefs.xml?

How do you feel about the name branding.xml?

Fennec's apk aggregates the values xml files from mobile/android/base/resources/values/ and $(MOZ_BRANDING_DIRECTORY)/res/values/ into a single res/values output directory. Thus, we should ensure all these values xml files have unique filenames. prefs.xml, defaults.xml, and bools.xml are pretty generic names, while branding.xml would clearly indicate the file's source and purpose.


> > +$(RES_VALUES): $(subst res/,$(srcdir)/resources/,$(RES_VALUES)) $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/res/values/bools.xml
>
> Not sure why we need this change.  Why is this different from the styles.xml for or the other |values| resources

RES_VALUES is a list of the *output* files (in the apk's directory structure). The original Makefile rule opportunistically assumes the input resource files are in a single directory (mobile/android/base/resources/values/). The new branding values resource files are in different directories, so I added the new file as a direct dependency.

Comment 8

[Doug Turner (:dougt)]

branding is a bit overloaded.  I means UI differences in the product.  Also when branding is official, it is the code changes that the C macro MOZ_OFFICIAL defines.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 581123 [details] [diff] [review]
708114-part1-add-enableStrictMode-method-v2.patch

+        if (Build.VERSION.SDK_INT < Build.VERSION_CODES.GINGERBREAD) {
+                return;
+        }

the spacing was a bit off here (8 spaces.  needs to be 4.)   I fixed this locally, and will push in a sec.

Comment 10

[Chris Peterson [:cpeterson]]

> branding is a bit overloaded.  I means UI differences in the product.

Is there another term used for branding- or channel-specific differences? Your example name |prefs.xml| might be apt, since Firefox developers use the phrase "pref a feature on or off". And that's what my code change is doing.

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: 708114-part2-add-StrictMode-flag-v3.patch

Rename branding.xml to prefs.xml.

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 581124 [details] [diff] [review]
708114-part2-add-StrictMode-flag-v2.patch

looks fine.  i took the option of renaming branding.xml to defaults.xml

Comment 13

[Doug Turner (:dougt)]

https://hg.mozilla.org/mozilla-central/rev/3c321d2c9884
https://hg.mozilla.org/mozilla-central/rev/aabb495e026a