Should remember state of checkboxes and email field in Android crashreporter client

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ted, Assigned: cjbarker)

Tracking

unspecified
mozilla28
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+)

Details

(Whiteboard: [lang=java][mentor=bnicholson])

Attachments

(1 attachment, 3 obsolete attachments)

I didn't realize this wasn't done, but on desktop we persist the state of all the checkboxes in the UI and the user's email address so the crashreporter UI does the "right thing" the next time the user encounters it.
I would really like to see this fixed.
tracking-fennec: --- → ?
tracking-fennec: ? → 23+
Assignee: nobody → bnicholson
tracking-fennec: 23+ → +
Fixing this bug will require saving/reading the state to shared prefs in this file: http://hg.mozilla.org/mozilla-central/file/85d75ed04851/mobile/android/base/CrashReporter.java
Assignee: bnicholson → nobody
Whiteboard: [lang=java][mentor=bnicholson]
Duplicate of this bug: 911464
Duplicate of this bug: 933127
Hi Brian et al,

Looks like no one has picked this up.  I see it as low-hanging fruit and a good first bug to tackle for a newbie to get their feet wet.

I don't mind taking it on.
Excellent, let me know if you have any questions!
Assignee: nobody → cjbarker
Hi Brian,

Day job/family delayed me in getting to this.   I'm now in the process of configuring my VM and DEV environment.  I just want to confirm appropriate procedures, and avoid and potential pitfalls.  I'm assuming if I follow the Android environment config I should be good to go building Gecko?

https://wiki.mozilla.org/Mobile/Fennec/Android

Also, are there steps for replicating or forcing the CrashReporter, or do I just need to put in something to trigger an Android Not Responding (ANR ala busy wait) for testing?

Lastly, are there any issues with simply running this in the emulator vs. physical HW?

Thanks.
There is an addon for causing fennec (called "crash me now") to crash, unfortunately it seems not to work any more. Ted, do you know why? 

Alternatively, you can send fennec a SIGILL to crash it it ("$> adb shell run-as org.mozilla.fennec_<USER NAME> kill -11 <PID>"
Flags: needinfo?(ted)
Apparently this, which is new:
E/GeckoLinker(18498): /data/data/org.mozilla.fennec/files/mozilla/bev18b55.default/extensions/crashme@ted.mielczarek.org/Android_arm-eabi-gcc3/libcrashme.so: Error: relocation to NULL @0x000014cc for symbol "__div0"
E/GeckoConsole(18498): [JavaScript Error: "Error: couldn't open library /data/data/org.mozilla.fennec/files/mozilla/bev18b55.default/extensions/crashme@ted.mielczarek.org/Android_arm-eabi-gcc3/libcrashme.so" {file: "resource://crashme/modules/Crasher.jsm" line: 28}]

You can kill the process with almost any common signal, I usually use kill -ABRT for testing.
Flags: needinfo?(ted)
(In reply to CJ Barker from comment #7)
> Hi Brian,
> 
> Day job/family delayed me in getting to this.   I'm now in the process of
> configuring my VM and DEV environment.  I just want to confirm appropriate
> procedures, and avoid and potential pitfalls.  I'm assuming if I follow the
> Android environment config I should be good to go building Gecko?
> 
> https://wiki.mozilla.org/Mobile/Fennec/Android

Yes, that's a great place to start, and that should (hopefully) be enough to get yourself a working build.

> Also, are there steps for replicating or forcing the CrashReporter, or do I
> just need to put in something to trigger an Android Not Responding (ANR ala
> busy wait) for testing?

Since you're making your own builds anyway, I'd just throw a RuntimeException somewhere in BrowserApp.java to force a crash.

I should have mentioned this before, but the one thing you'll want to make sure that you do is create "official" builds, since the crash reporter won't appear otherwise. To make an official build, add this line to your mozconfig:
export MOZILLA_OFFICIAL=1

(You can refer to the "Setup Fennec mozconfig" section of the link you gave for setting up a mozconfig).

> Lastly, are there any issues with simply running this in the emulator vs.
> physical HW?

The main disadvantage of using the emulator is performance. But since you're working on a simple Android view that's independent of Fennec, an emulator should work well.
Posted patch save_crashrpt_settings.patch (obsolete) — Splinter Review
Attachment #8337486 - Flags: review?(bnicholson)
It took me longer to setup my DEV environment than to write the patch ;-)

Feedback welcomed - this should be a very simple, straightforward addition for persisting settings via SharedPrefences.
Comment on attachment 8337486 [details] [diff] [review]
save_crashrpt_settings.patch

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

Thanks, this looks great! I'd suggest only some minor changes regarding the naming/location of variables.

::: mobile/android/base/CrashReporter.java
@@ +140,5 @@
>          final EditText commentsEditText = (EditText) findViewById(R.id.comment);
>          final EditText emailEditText = (EditText) findViewById(R.id.email);
>  
> +        // Load CrashReporter preferences to avoid redundant user input.
> +        final boolean isSendCrashRpt = prefs.getBoolean(GeckoApp.PREFS_SEND_CRASH_RPT, true);

Nit: s/Rpt/Report/

@@ +142,5 @@
>  
> +        // Load CrashReporter preferences to avoid redundant user input.
> +        final boolean isSendCrashRpt = prefs.getBoolean(GeckoApp.PREFS_SEND_CRASH_RPT, true);
> +        final boolean isIncludeUrl   = prefs.getBoolean(GeckoApp.PREFS_INCL_URL_CRASH, false);
> +        final boolean isAllowContact = prefs.getBoolean(GeckoApp.PREFS_ALLOW_CONTACT_CRASH, false);

Nit: These already start with verbs, so I'd just drop the "is" prefix you're using on each of them.

::: mobile/android/base/GeckoApp.java
@@ +156,5 @@
>      public static final String PREFS_NAME                  = "GeckoApp";
>      public static final String PREFS_OOM_EXCEPTION         = "OOMException";
>      public static final String PREFS_VERSION_CODE          = "versionCode";
>      public static final String PREFS_WAS_STOPPED           = "wasStopped";
> +    public static final String PREFS_SEND_CRASH_RPT        = "sendMozCrashRpt";

Nit: Please use full names/strings (PREFS_SEND_CRASH_REPORT/sendMozCrashReport)

@@ +157,5 @@
>      public static final String PREFS_OOM_EXCEPTION         = "OOMException";
>      public static final String PREFS_VERSION_CODE          = "versionCode";
>      public static final String PREFS_WAS_STOPPED           = "wasStopped";
> +    public static final String PREFS_SEND_CRASH_RPT        = "sendMozCrashRpt";
> +    public static final String PREFS_INCL_URL_CRASH        = "includeUrlCrash";

Nit: PREFS_INCLUDE_URL

@@ +159,5 @@
>      public static final String PREFS_WAS_STOPPED           = "wasStopped";
> +    public static final String PREFS_SEND_CRASH_RPT        = "sendMozCrashRpt";
> +    public static final String PREFS_INCL_URL_CRASH        = "includeUrlCrash";
> +    public static final String PREFS_ALLOW_CONTACT_CRASH   = "allowMozContactCrash";
> +    public static final String PREFS_CONTACT_EMAIL_CRASH   = "contactEmailCrash";

These prefs are used only in CrashReporter, so there's no need to have these as public static fields in GeckoApp. Let's move these CrashReporter.java, make them private static, and remove the _CRASH suffixes on the variable names above since they will be redundant.
Attachment #8337486 - Flags: review?(bnicholson) → feedback+
(In reply to Brian Nicholson (:bnicholson) from comment #13)

> These prefs are used only in CrashReporter, so there's no need to have these
> as public static fields in GeckoApp. Let's move these CrashReporter.java,
> make them private static, and remove the _CRASH suffixes on the variable
> names above since they will be redundant.

Another reason to do this: if a crash is caused by a static initializer error in GeckoApp, CrashReporter would die hard.
Attachment #8337486 - Attachment is obsolete: true
Attachment #8338260 - Flags: review?(bnicholson)
(In reply to Richard Newman [:rnewman] from comment #14)
> Another reason to do this: if a crash is caused by a static initializer
> error in GeckoApp, CrashReporter would die hard.

Agreed, however, there are a few classes loosely coupled w/ GeckoApp.java used for tracking the preference state GeckoApp.PREFS_WAS_STOPPED:  CrashReporter.java, health/BrowserHealthRecorder.java and GeckoApp.java  

If we want to truly decouple everything we'd have to refactor how the application persists stopped state and exception tracking (see GeckoApp.PREFS_WAS_STOPPED and GeckoApp.PREFS_OOM_EXCEPTION).

I believe this is out of the scope of this feature request, and thus, didn't tackle this change.
Comment on attachment 8338260 [details] [diff] [review]
V2 of patch that accounts for minor changes to variable names and scoping to CrashReporter.java

># HG changeset patch
># Parent bcf16a0d47ab4350763e69651076bea0289daeca
># User CJ Barker <cjbarker@gmail.com>
>Bug 870049 - Added SharedPreferences to persistent Crash Report client settings for checkbox state and email text input.
>
>Version 2 - Accounts for review feedback from Brian Nicholson
>* Minor variable name changes and moved preference variables @ scope to CrashReporter.java
>
>
>diff --git a/mobile/android/base/CrashReporter.java b/mobile/android/base/CrashReporter.java
>--- a/mobile/android/base/CrashReporter.java
>+++ b/mobile/android/base/CrashReporter.java
>@@ -46,16 +46,22 @@ public class CrashReporter extends Activ
>     private static final String PAGE_URL_KEY = "URL";
>     private static final String NOTES_KEY = "Notes";
>     private static final String SERVER_URL_KEY = "ServerURL";
> 
>     private static final String CRASH_REPORT_SUFFIX = "/mozilla/Crash Reports/";
>     private static final String PENDING_SUFFIX = CRASH_REPORT_SUFFIX + "pending";
>     private static final String SUBMITTED_SUFFIX = CRASH_REPORT_SUFFIX + "submitted";
> 
>+    private static final String PREFS_NAME          = "CrashReportSettings";
>+    private static final String PREFS_SEND_REPORT   = "sendReport";
>+    private static final String PREFS_INCLUDE_URL   = "includeUrl";
>+    private static final String PREFS_ALLOW_CONTACT = "allowContact";
>+    private static final String PREFS_CONTACT_EMAIL = "contactEmail";
>+
>     private Handler mHandler;
>     private ProgressDialog mProgressDialog;
>     private File mPendingMinidumpFile;
>     private File mPendingExtrasFile;
>     private HashMap<String, String> mExtrasStringMap;
> 
>     private boolean moveFile(File inFile, File outFile) {
>         Log.i(LOGTAG, "moving " + inFile + " to " + outFile);
>@@ -123,37 +129,38 @@ public class CrashReporter extends Activ
>         mPendingExtrasFile = new File(pendingDir, extrasFile.getName());
>         moveFile(extrasFile, mPendingExtrasFile);
> 
>         mExtrasStringMap = new HashMap<String, String>();
>         readStringsFromFile(mPendingExtrasFile.getPath(), mExtrasStringMap);
> 
>         // Set the flag that indicates we were stopped as expected, as
>         // we will send a crash report, so it is not a silent OOM crash.
>-        SharedPreferences prefs =
>+        SharedPreferences geckoPrefs =
>             getSharedPreferences(GeckoApp.PREFS_NAME, 0);

CJ: PREFS_WAS_STOPPED is loosely coupled to GeckoApp. This also includes exception state tracking.  See Comments in bug thread to Richard Newman about refactoring this as a separate request to decouple completely from GeckoApp.java

>-        SharedPreferences.Editor editor = prefs.edit();
>+        SharedPreferences.Editor editor = geckoPrefs.edit();
>         editor.putBoolean(GeckoApp.PREFS_WAS_STOPPED, true);
>         editor.commit();
> 
>         final CheckBox allowContactCheckBox = (CheckBox) findViewById(R.id.allow_contact);
>         final CheckBox includeUrlCheckBox = (CheckBox) findViewById(R.id.include_url);
>         final CheckBox sendReportCheckBox = (CheckBox) findViewById(R.id.send_report);
>         final EditText commentsEditText = (EditText) findViewById(R.id.comment);
>         final EditText emailEditText = (EditText) findViewById(R.id.email);
> 
>         // Load CrashReporter preferences to avoid redundant user input.
>-        final boolean isSendCrashRpt = prefs.getBoolean(GeckoApp.PREFS_SEND_CRASH_RPT, true);
>-        final boolean isIncludeUrl   = prefs.getBoolean(GeckoApp.PREFS_INCL_URL_CRASH, false);
>-        final boolean isAllowContact = prefs.getBoolean(GeckoApp.PREFS_ALLOW_CONTACT_CRASH, false);
>-        final String contactEmail    = prefs.getString(GeckoApp.PREFS_CONTACT_EMAIL_CRASH, "");
>+        SharedPreferences prefs = getSharedPreferences(PREFS_NAME, 0);
>+        final boolean sendReport   = prefs.getBoolean(PREFS_SEND_REPORT, true);
>+        final boolean includeUrl   = prefs.getBoolean(PREFS_INCLUDE_URL, false);
>+        final boolean allowContact = prefs.getBoolean(PREFS_ALLOW_CONTACT, false);
>+        final String contactEmail  = prefs.getString(PREFS_CONTACT_EMAIL, "");
> 
>-        allowContactCheckBox.setChecked(isAllowContact);
>-        includeUrlCheckBox.setChecked(isIncludeUrl);
>-        sendReportCheckBox.setChecked(isSendCrashRpt);
>+        allowContactCheckBox.setChecked(allowContact);
>+        includeUrlCheckBox.setChecked(includeUrl);
>+        sendReportCheckBox.setChecked(sendReport);
>         emailEditText.setText(contactEmail);
> 
>         sendReportCheckBox.setOnCheckedChangeListener(new CheckBox.OnCheckedChangeListener() {
>             @Override
>             public void onCheckedChanged(CompoundButton checkbox, boolean isChecked) {
>                 commentsEditText.setEnabled(isChecked);
>                 commentsEditText.requestFocus();
> 
>@@ -221,28 +228,28 @@ public class CrashReporter extends Activ
>             @Override
>             public void run() {
>                 sendReport(mPendingMinidumpFile, mExtrasStringMap, mPendingExtrasFile);
>             }
>         }, "CrashReporter Thread").start();
>     }
> 
>     private void savePrefs() {
>-        SharedPreferences prefs = getSharedPreferences(GeckoApp.PREFS_NAME, 0);
>+        SharedPreferences prefs = getSharedPreferences(PREFS_NAME, 0);
>         SharedPreferences.Editor editor = prefs.edit();
> 
>-        final boolean isAllowContact = ((CheckBox) findViewById(R.id.allow_contact)).isChecked();
>-        final boolean isIncludeUrl   = ((CheckBox) findViewById(R.id.include_url)).isChecked();
>-        final boolean isSendCrashRpt = ((CheckBox) findViewById(R.id.send_report)).isChecked();
>-        final String contactEmail = ((EditText) findViewById(R.id.email)).getText().toString();
>+        final boolean allowContact = ((CheckBox) findViewById(R.id.allow_contact)).isChecked();
>+        final boolean includeUrl   = ((CheckBox) findViewById(R.id.include_url)).isChecked();
>+        final boolean sendReport   = ((CheckBox) findViewById(R.id.send_report)).isChecked();
>+        final String contactEmail  = ((EditText) findViewById(R.id.email)).getText().toString();
> 
>-        editor.putBoolean(GeckoApp.PREFS_ALLOW_CONTACT_CRASH, isAllowContact);
>-        editor.putBoolean(GeckoApp.PREFS_INCL_URL_CRASH, isIncludeUrl);
>-        editor.putBoolean(GeckoApp.PREFS_SEND_CRASH_RPT, isSendCrashRpt);
>-        editor.putString(GeckoApp.PREFS_CONTACT_EMAIL_CRASH, contactEmail);
>+        editor.putBoolean(PREFS_ALLOW_CONTACT, allowContact);
>+        editor.putBoolean(PREFS_INCLUDE_URL, includeUrl);
>+        editor.putBoolean(PREFS_SEND_REPORT, sendReport);
>+        editor.putString(PREFS_CONTACT_EMAIL, contactEmail);
> 
>         // A slight performance improvement via async apply() vs. blocking on commit().
>         if (Build.VERSION.SDK_INT < Build.VERSION_CODES.GINGERBREAD) {
>             editor.commit();
>         } else {
>             editor.apply();
>         }
>     }
>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
>--- a/mobile/android/base/GeckoApp.java
>+++ b/mobile/android/base/GeckoApp.java
>@@ -152,20 +152,16 @@ abstract public class GeckoApp
>     public static final String EXTRA_STATE_BUNDLE          = "stateBundle";
> 
>     public static final String PREFS_ALLOW_STATE_BUNDLE    = "allowStateBundle";
>     public static final String PREFS_CRASHED               = "crashed";
>     public static final String PREFS_NAME                  = "GeckoApp";
>     public static final String PREFS_OOM_EXCEPTION         = "OOMException";
>     public static final String PREFS_VERSION_CODE          = "versionCode";
>     public static final String PREFS_WAS_STOPPED           = "wasStopped";
>-    public static final String PREFS_SEND_CRASH_RPT        = "sendMozCrashRpt";
>-    public static final String PREFS_INCL_URL_CRASH        = "includeUrlCrash";
>-    public static final String PREFS_ALLOW_CONTACT_CRASH   = "allowMozContactCrash";
>-    public static final String PREFS_CONTACT_EMAIL_CRASH   = "contactEmailCrash";
> 
>     public static final String SAVED_STATE_IN_BACKGROUND   = "inBackground";
>     public static final String SAVED_STATE_PRIVATE_SESSION = "privateSession";
> 
>     static private final String LOCATION_URL = "https://location.services.mozilla.com/v1/submit";
> 
>     // Delay before running one-time "cleanup" tasks that may be needed
>     // after a version upgrade.
Comment on attachment 8338260 [details] [diff] [review]
V2 of patch that accounts for minor changes to variable names and scoping to CrashReporter.java

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

Looks good; r+ with changes described below.

::: mobile/android/base/CrashReporter.java
@@ +146,5 @@
>          final EditText commentsEditText = (EditText) findViewById(R.id.comment);
>          final EditText emailEditText = (EditText) findViewById(R.id.email);
>  
>          // Load CrashReporter preferences to avoid redundant user input.
> +        SharedPreferences prefs = getSharedPreferences(PREFS_NAME, 0);

Please use GeckoApp.PREFS_NAME here like you had before. The convention we've been using is to store all global prefs in a single file.

@@ +232,5 @@
>          }, "CrashReporter Thread").start();
>      }
>  
>      private void savePrefs() {
> +        SharedPreferences prefs = getSharedPreferences(PREFS_NAME, 0);

Same here.
Attachment #8338260 - Flags: review?(bnicholson) → review+
Posted patch save_crash_settings_v3.patch (obsolete) — Splinter Review
Attachment #8338260 - Attachment is obsolete: true
Attachment #8340719 - Flags: review+
Brian, I wasn't completely sure how to handle the r+.  I think I submitted the patch okay w/ r+ and marking the old one obsolete.

Please let me know if there's anything I missed or required; otherwise, I'll consider this bug complete on my part.

Cheers.
Thanks CJ. For addressing minor review comments, carrying forward the r+ like you did is fine. You could also request review again -- either is acceptable.

Just a few last steps to get this in landable form. Please do the following:
* Fold together your patches into a single patch. It's good to split up patches into multiple parts if each patch addresses a specific piece of the feature, but I don't think there's any clear division in this case.
* Append "r=bnicholson" to the commit message of your patch to indicate that it has passed review (for example, see other landed patches at http://hg.mozilla.org/integration/fx-team/).
* Add "checkin-needed" as a keyword for this bug to indicate that the patch is ready to be landed.
Attachment #8340719 - Attachment is obsolete: true
Attachment #8341432 - Flags: review+
Keywords: checkin-needed
Status: NEW → ASSIGNED
Brian, I could not denote the status as "resolved --> fixed". I'm not sure what build release this is part of.  

I'll leave it up to use to update accordingly.
Don't worry, we have a tool that will mark it as FIXED and set the target milestone appropriately once your change makes it to the appropriate repository (mozilla-central). Thanks for the patch!
https://hg.mozilla.org/mozilla-central/rev/cd60f45d5165
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.