595169 - Implement crash reporter client for Android

Status: RESOLVED FIXED

Description

[Ted Mielczarek [:ted] [:ted.mielczarek]]

The crash reporter is implemented as a native application per-platform. There's a common file:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter.cpp

that handles all the startup work and calls into platform-specific files to do some operations and display the UI. The platform implementations are responsible for displaying and managing the entire UI, submitting the crash report (via HTTP POST) using native networking libraries, and restarting the main application.

For Android, we'll need a native Android app. If we can reuse crashreporter.cpp via NDK, and wrap it in a native app, that would be great, so we don't have to duplicate all that logic.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Created attachment 481966 [details] [diff] [review]
WIP

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

the command line to launch this would be: 

am start -a org.mozilla.gecko.reportCrashfennec -n org.mozilla.fennec/org.mozilla.fennec.CrashReporter -f 0x10000000 --es reportPath <path to report> --es pageURL <url>

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Created attachment 482016 [details] [diff] [review]
WIP v.2

A little more progress. Anything passed in as a string extra (--es <key> <value>) on the command line will be included as a part in the multipart post. "upload_file_minidump" is special cased as the minidump file's path and "comment" is special cased to be included only if the user checks off that they want to include the web pages url. Everything else (product_name, product_version, ptime, ctime etc.) will be passed strait through.

Comment 4

[Ted Mielczarek [:ted] [:ted.mielczarek]]

Can we make this work more consistently with the other platforms? On every other platform we just pass the path to the .dmp file on the cmdline, and the client finds the .extra file by swapping out the file extension and reads the extra params from there.

Also I'm curious how packaging will work. Does this get stored in the same .apk file?

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Yes, I'd assume this will be packaged with the apk.

Can you attach an example of the .extra file?

Comment 6

[Ted Mielczarek [:ted] [:ted.mielczarek]]

It's a pretty simple key=value file, one per line. The code that parses it is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter.cpp#136

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Created attachment 483086 [details] [diff] [review]
patch

I launched this with a .dmp and .extra file from firefox with ServerURL pointing to an instance of the crashreport.sjs from mochitests. It replied with:
CrashID=bp-69d960d5-fb98-4c70-80d7-68ecfbfbd42 

which I take to mean success.

Comment 8

[Ted Mielczarek [:ted] [:ted.mielczarek]]

Yeah, if you run that, you can verify the results by loading the same URL you posted to, but appending ?id=<the id you received>, and it should reply with a JSON blob containing all the params you sent.

This looks good at a glance. The only thing I'm not wild about is putting those l10n strings in the .dtd, since we already have essentially the same strings in crashreporter.ini / crashreporter-override.ini. Duplicating strings doesn't seem like a good idea.

Comment 9

[Axel Hecht [:Pike]]

I'm not concerned about having the crashreporter strings in a DTD, really. For those folks with translation memory software, that's not going to have an impact.

Having the same DTD file used in both the crashreporter and the installer sounds icky, but I don't know enough about the platform to make an educated comment.

Comment 10

[Ted Mielczarek [:ted] [:ted.mielczarek]]

Comment on attachment 483086 [details] [diff] [review]
patch

Looks like a good start, but as discussed on IRC:
* Needs to write the .txt file to Crash Reports/submitted
* Needs to move dumps from the profile dir to Crash Reports/pending, and delete on successful submit
* Could be useful to write Crash Reports/submit.log like we do on other platforms
* Could use some UI tweaks (let madhava have a crack at it, probably), but we can do that as a followup

Also I am saddened that Android doesn't provide a Java API for sending multipart/form-data. That seems really crappy.

I'm able to successfully submit crash reports with this patch, which is cool. We still need to sort out why the exec isn't working.

Comment 11

[Ted Mielczarek [:ted] [:ted.mielczarek]]

RE: UI tweaks, I think you should at least add a spinny thobber + disable the close/submit buttons while submitting. Right now there's no feedback that clicking had any effect, which is confusing. Also, the desktop crashreporter clients do the submit + restart in parallel, so that you don't have to wait for the submit to finish before the browser comes back up (which is probably an even better idea on mobile where submitting might take a while).

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Created attachment 483218 [details] [diff] [review]
patch v.2

Comment 13

[Ted Mielczarek [:ted] [:ted.mielczarek]]

Comment on attachment 483218 [details] [diff] [review]
patch v.2

>diff --git a/embedding/android/CrashReporter.java.in b/embedding/android/CrashReporter.java.in
>new file mode 100644
>--- /dev/null
>+++ b/embedding/android/CrashReporter.java.in
>+public class CrashReporter extends Activity {
>+  final String kMiniDumpPathKey = "upload_file_minidump";
>+  final String kPageURLKey = "comments";

Uh, the URL goes in the key named..."URL".

>+
>+
>+  boolean moveFile(File inFile, File outFile) {
>+    Log.i("GeckoCrashReporter", "moving " + inFile + " to " + outFile);
>+    if (inFile.renameTo(outFile))
>+      return true;
>+    try {
>+      outFile.createNewFile();
>+      Log.i("GeckoCrashReporter", "couldn't rename minidump file");
>+      // so copy it instead
>+      FileChannel inChannel = new FileInputStream(inFile).getChannel();
>+      FileChannel outChannel = new FileOutputStream(outFile).getChannel();
>+      long tansferred = inChannel.transferTo(0, inChannel.size(), outChannel);

spelling nit: "transferred".

>+  /** Called when the activity is first created. */
>+  @Override
>+  public void onCreate(Bundle savedInstanceState) {
>+    super.onCreate(savedInstanceState);
>+    setContentView(R.layout.crash_reporter);
>+    final Button restartButton = (Button) findViewById(R.id.restart);
>+    final Button closeButton = (Button) findViewById(R.id.close);
>+    String passedMinidumpPath = getIntent().getStringExtra("minidumpPath");
>+    File passedMinidumpFile = new File(passedMinidumpPath);
>+    File pendingDir =
>+      new File("/data/data/org.mozilla.fennec/mozilla/Crash Reports/pending");

@MOZ_APP_NAME@. Also, isn't there an API to get this directory instead of hardcoding it? Probably http://developer.android.com/reference/android/content/Context.html#getDir%28java.lang.String,%20int%29


>+  boolean readStringsFromFile(String filePath, Map stringMap)
>+  {
>+    try{
>+      BufferedReader reader = new BufferedReader(
>+        new FileReader(filePath));
>+      String line;
>+      while ((line = reader.readLine()) != null) {
>+        int equalsPos = -1;
>+        if ((equalsPos = line.indexOf('=')) != -1) {
>+          String key = line.substring(0, equalsPos);
>+          String val = line.substring(equalsPos + 1);
>+          stringMap.put(key, val);

You'll want to unescape too:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter.cpp#93

Otherwise newlines etc in the value will get transferred incorrectly.

>+  void sendReport(boolean restart, File minidumpFile,
>+                  Map<String, String> extras, File extrasFile) {
>+    Log.i("GeckoCrashReport", "sendReport: " + minidumpFile.getPath());
>+    final CheckBox sendReportCheckbox = (CheckBox) findViewById(R.id.send_report);
>+    final CheckBox includeURLCheckbox = (CheckBox) findViewById(R.id.include_url);
>+    if (!sendReportCheckbox.isChecked())
>+      closeAndDoRestart(restart);
>+    String spec = extras.get("ServerURL");
>+    if (spec == null)
>+      closeAndDoRestart(restart);
>+
>+    ProgressDialog progressDialog =
>+      ProgressDialog.show(CrashReporter.this, "",
>+                          getString(R.string.sending_crash_report),
>+                          true);
>+
>+    Log.i("GeckoCrashReport", "server url: " + spec);
>+    try {
>+      URL url = new URL(spec);
>+      HttpURLConnection conn = (HttpURLConnection)url.openConnection();
>+      conn.setRequestMethod("POST");
>+      String boundary = generateBoundary();
>+      conn.setDoOutput(true);
>+      conn.setRequestProperty("Content-Type", "multipart/form-data; boundary=" + boundary);
>+
>+      OutputStream os = conn.getOutputStream();
>+      Iterator<String> keys = extras.keySet().iterator();
>+      while (keys.hasNext()) {
>+        String key = keys.next();
>+        Log.i("GeckoCrashReport", "key: " + key);

This is bound to be really noisy in the log. Do we care about log spew?

>+      if (conn.getResponseCode() == conn.HTTP_OK) {
>+        File submittedDir = new File(
>+          "/data/data/org.mozilla.fennec/mozilla/Crash Reports/submitted");

Same comment about hardcoded directories.

>+        submittedDir.mkdirs();
>+        minidumpFile.delete();
>+        extrasFile.delete();
>+        String response = responseBuffer.toString();
>+        String crashid = response.replaceFirst("CrashID=", "");

You should parse the response using the same readStrings logic, since we may want to send additional data in the future. (Right now we only send CrashID, but that could change and I'd like the flexibility.)

>+    progressDialog.dismiss();
>+    closeAndDoRestart(restart);

You're still restarting only after the submit finishes, I think that's going to make for a bad experience on cell networks. Can you do them in parallel instead?

>diff --git a/embedding/android/android_strings.dtd b/embedding/android/android_strings.dtd
>--- a/embedding/android/android_strings.dtd
>+++ b/embedding/android/android_strings.dtd
>@@ -2,3 +2,10 @@
> <!ENTITY  incompatable_cpu_error "This device does not meet the minimum system requirements for &brandShortName;.">
> <!ENTITY  no_space_to_start_error "There is not enough space available for &brandShortName; to start.">
> <!ENTITY  error_loading_file "An error occurred when trying to load files required to run &brandShortName;">
>+<!ENTITY  crash_message "&brandShortName; has crashed. Your tabs should be listed on the Firefox Start page when you restart.">
>+<!ENTITY  crash_help_message "Please help us fix this problem!">
>+<!ENTITY  crash_send_report_message "Send Mozilla a crash report">
>+<!ENTITY  crash_include_url "include page address">

Nit: should probably capitalize "Include"

>+<!ENTITY  crash_close_label "Close">
>+<!ENTITY  crash_restart_label "Restart &brandShortName;">
>+<!ENTITY  sending_crash_report "Sending crash report">

Nit: add an elipsis at the end.

r- for a few changes, but it looks pretty good.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Created attachment 483512 [details] [diff] [review]
patch

fixed ted's nits

screenshots for madhava's ui review:
http://lassey.us/crash_reporter.png
http://lassey.us/crash_reporter_spinner.png

ted, for your getDir() suggestion, I think that might be the right thing to do, but we don't do it anywhere else in the code. I'll file a follow up to look at making that change across the board.

Comment 15

[Ted Mielczarek [:ted] [:ted.mielczarek]]

Comment on attachment 483512 [details] [diff] [review]
patch

Looks good, r=me

Comment 16

[Madhava Enros [:madhava]]

Comment on attachment 483512 [details] [diff] [review]
patch

This is fine with a couple of nits, layout-wise. See the diagram here:
http://www.flickr.com/photos/madhava_work/5084221923/

the things:
- some margins on the overall window
- larger body text for the first paragraphs
- some spacing between the sections
- center the buttons

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Created attachment 483631 [details] [diff] [review]
patch

addresses madhava's nits, carrying his ui-r+ and ted's r+

Comment 18

[Michael Wu [:mwu]]

Comment on attachment 483631 [details] [diff] [review]
patch

Some nits, some more important things.

1. The string constants at the top of CrashReporter should be static final.
2. Will the crash data ever end up in another partition? If not, avoid the file
copy/delete fallback.
3. Use the android:onClick attribute in the xml layout to specify functions
that are called on click. This will let you avoid the terribly ugly Java idiom
for writing callbacks.
4. Curly braces for top level classes should start on their own line.
5. Curly braces for methods should start on their own line.
6. Space after try (readStringsFromFile)
7. Put throws on its own line (sendPart, sendFile)
8. Space after the comma in arg list of sendReport.
9. Remove the extra empty lines before and after the return in generateBoundary
and in the while loop in sendReport.
10. Inline the ----- in generateBoundary.
11. Remove the extra space after BufferReader br in sendReport.
12. Ensure that we delete all our files no matter what happens in sendReport,
either by using finally or maybe just adding the delete code after the try
block.
13. Space after MOZ_ANDROID_DRAWABLES in the makefile.
14. There should be no direct reference to Firefox in the strings.
15. There should be some ifdef'ing in case we're building without crash
reporter.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

Created attachment 483693 [details] [diff] [review]
patch

(In reply to comment #18)
> 2. Will the crash data ever end up in another partition? If not, avoid the file
> copy/delete fallback.
the renameTo() fails, the fallback is needed

> 12. Ensure that we delete all our files no matter what happens in sendReport,
> either by using finally or maybe just adding the delete code after the try
> block.
my understanding from ted is we're supposed to leave them in the pending folder if something goes wrong

Comment 20

[Michael Wu [:mwu]]

Comment on attachment 483693 [details] [diff] [review]
patch

Ah, these onClick callbacks are much less rage inducing.

There's still an odd looking blank line in the while loop in sendReport.

I suspect we can ifdef the crashreporter activity (in AndroidManifest.xml.in) on MOZ_CRASHREPORTER too. r=me with that looked into. Thanks.

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Created attachment 483697 [details] [diff] [review]
patch for checkin

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/13dc6bfc5e7a

Comment 23

[ignoring orange]

I just tripped over the ellipsis in sending_crash_report. That's a unicode escape here, does mProgressDialog.setMessage decode that?

Otherwise, we'd have to use a numeric xml entity to encode the ellipsis, or the real unicode char.

Comment 24

[Axel Hecht [:Pike]]

Doh, that was me, fwiw.