Implement crash reporter client for Android

RESOLVED FIXED in mozilla2.0b7

Status

()

Toolkit
Crash Reporting
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: ted, Assigned: blassey)

Tracking

Trunk
mozilla2.0b7
ARM
Android
Points:
---

Firefox Tracking Flags

(fennec2.0b2+)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Blocks: 595171
(Assignee)

Updated

7 years ago
Assignee: nobody → blassey.bugs
Created attachment 481966 [details] [diff] [review]
WIP
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>
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.
Attachment #481966 - Attachment is obsolete: true
(Reporter)

Comment 4

7 years ago
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?
Yes, I'd assume this will be packaged with the apk.

Can you attach an example of the .extra file?
(Reporter)

Comment 6

7 years ago
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
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.
Attachment #482016 - Attachment is obsolete: true
Attachment #483086 - Flags: review?(ted.mielczarek)
Attachment #483086 - Flags: review?(mwu)
(Reporter)

Comment 8

7 years ago
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

7 years ago
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.
(Reporter)

Comment 10

7 years ago
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.
Attachment #483086 - Flags: review?(ted.mielczarek) → review-
(Reporter)

Comment 11

7 years ago
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).
Created attachment 483218 [details] [diff] [review]
patch v.2
Attachment #483086 - Attachment is obsolete: true
Attachment #483218 - Flags: review?(ted.mielczarek)
Attachment #483218 - Flags: review?(mwu)
Attachment #483086 - Flags: review?(mwu)

Updated

7 years ago
Attachment #483218 - Flags: ui-review?(madhava)
(Reporter)

Comment 13

7 years ago
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.
Attachment #483218 - Flags: review?(ted.mielczarek) → review-
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.
Attachment #483218 - Attachment is obsolete: true
Attachment #483512 - Flags: ui-review?(madhava)
Attachment #483512 - Flags: review?(ted.mielczarek)
Attachment #483512 - Flags: review?(mwu)
Attachment #483218 - Flags: ui-review?(madhava)
Attachment #483218 - Flags: review?(mwu)
(Reporter)

Comment 15

7 years ago
Comment on attachment 483512 [details] [diff] [review]
patch

Looks good, r=me
Attachment #483512 - Flags: review?(ted.mielczarek) → review+
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
Attachment #483512 - Flags: ui-review?(madhava) → ui-review+
Created attachment 483631 [details] [diff] [review]
patch

addresses madhava's nits, carrying his ui-r+ and ted's r+
Attachment #483512 - Attachment is obsolete: true
Attachment #483631 - Flags: ui-review+
Attachment #483631 - Flags: review?(mwu)
Attachment #483512 - Flags: review?(mwu)

Comment 18

7 years ago
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.
Attachment #483631 - Flags: review?(mwu)
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
Attachment #483631 - Attachment is obsolete: true
Attachment #483693 - Flags: ui-review+
Attachment #483693 - Flags: review?(mwu)

Comment 20

7 years ago
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.
Attachment #483693 - Flags: review?(mwu) → review+
(Assignee)

Updated

7 years ago
tracking-fennec: --- → 2.0b2+
Created attachment 483697 [details] [diff] [review]
patch for checkin
Attachment #483693 - Attachment is obsolete: true
Attachment #483697 - Flags: ui-review+
Attachment #483697 - Flags: review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
pushed http://hg.mozilla.org/mozilla-central/rev/13dc6bfc5e7a
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Keywords: checkin-needed
Target Milestone: --- → mozilla2.0b8

Updated

7 years ago
Target Milestone: mozilla2.0b8 → mozilla2.0b7

Comment 23

7 years ago
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

7 years ago
Doh, that was me, fwiw.
You need to log in before you can comment on or make changes to this bug.