Last Comment Bug 708114 - Disable Android StrictMode for release and beta builds
: Disable Android StrictMode for release and beta builds
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P2 normal (vote)
: ---
Assigned To: Chris Peterson [:cpeterson]
:
Mentors:
Depends on:
Blocks: 710078
  Show dependency treegraph
 
Reported: 2011-12-06 15:16 PST by Chris Peterson [:cpeterson]
Modified: 2012-01-09 11:17 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
708114-part1-add-enableStrictMode-method.patch (2.31 KB, patch)
2011-12-09 14:43 PST, Chris Peterson [:cpeterson]
doug.turner: review-
Details | Diff | Splinter Review
708114-part2-add-StrictMode-flag.patch (3.18 KB, patch)
2011-12-09 14:47 PST, Chris Peterson [:cpeterson]
doug.turner: review-
Details | Diff | Splinter Review
708114-part1-add-enableStrictMode-method-v2.patch (3.58 KB, patch)
2011-12-12 17:27 PST, Chris Peterson [:cpeterson]
doug.turner: review+
Details | Diff | Splinter Review
708114-part2-add-StrictMode-flag-v2.patch (4.65 KB, patch)
2011-12-12 17:27 PST, Chris Peterson [:cpeterson]
no flags Details | Diff | Splinter Review
708114-part2-add-StrictMode-flag-v3.patch (4.60 KB, patch)
2011-12-12 18:33 PST, Chris Peterson [:cpeterson]
no flags Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2011-12-06 15:16:26 PST
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] 2011-12-09 14:43:37 PST
Created attachment 580557 [details] [diff] [review]
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] 2011-12-09 14:47:05 PST
Created attachment 580560 [details] [diff] [review]
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) 2011-12-09 23:15:30 PST
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) 2011-12-09 23:22:21 PST
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] 2011-12-12 17:27:00 PST
Created attachment 581123 [details] [diff] [review]
708114-part1-add-enableStrictMode-method-v2.patch

* Align comments.
* Remove penaltyDeath().
Comment 6 Chris Peterson [:cpeterson] 2011-12-12 17:27:40 PST
Created attachment 581124 [details] [diff] [review]
708114-part2-add-StrictMode-flag-v2.patch

* Rename bools.xml branding.xml.
* Declare RES_VALUES' dependencies explicitly.
Comment 7 Chris Peterson [:cpeterson] 2011-12-12 17:29:01 PST
@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) 2011-12-12 17:51:58 PST
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) 2011-12-12 18:00:53 PST
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] 2011-12-12 18:20:23 PST
> 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] 2011-12-12 18:33:37 PST
Created attachment 581140 [details] [diff] [review]
708114-part2-add-StrictMode-flag-v3.patch

Rename branding.xml to prefs.xml.
Comment 12 Doug Turner (:dougt) 2011-12-12 19:21:48 PST
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

Note You need to log in before you can comment on or make changes to this bug.