Closed
Bug 595169
Opened 15 years ago
Closed 15 years ago
Implement crash reporter client for Android
Categories
(Toolkit :: Crash Reporting, defect)
Tracking
()
RESOLVED
FIXED
mozilla2.0b7
Tracking | Status | |
---|---|---|
fennec | 2.0b2+ | --- |
People
(Reporter: ted, Assigned: blassey)
References
Details
Attachments
(1 file, 7 obsolete files)
21.02 KB,
patch
|
blassey
:
review+
blassey
:
ui-review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → blassey.bugs
Assignee | ||
Comment 1•15 years ago
|
||
Assignee | ||
Comment 2•15 years ago
|
||
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>
Assignee | ||
Comment 3•15 years ago
|
||
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•15 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?
Assignee | ||
Comment 5•15 years ago
|
||
Yes, I'd assume this will be packaged with the apk.
Can you attach an example of the .extra file?
Reporter | ||
Comment 6•15 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
Assignee | ||
Comment 7•15 years ago
|
||
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•15 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•15 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•15 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•15 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).
Assignee | ||
Comment 12•15 years ago
|
||
Attachment #483086 -
Attachment is obsolete: true
Attachment #483218 -
Flags: review?(ted.mielczarek)
Attachment #483218 -
Flags: review?(mwu)
Attachment #483086 -
Flags: review?(mwu)
Updated•15 years ago
|
Attachment #483218 -
Flags: ui-review?(madhava)
Reporter | ||
Comment 13•15 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-
Assignee | ||
Comment 14•15 years ago
|
||
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•15 years ago
|
||
Comment on attachment 483512 [details] [diff] [review]
patch
Looks good, r=me
Attachment #483512 -
Flags: review?(ted.mielczarek) → review+
Comment 16•15 years ago
|
||
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+
Assignee | ||
Comment 17•15 years ago
|
||
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•15 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)
Assignee | ||
Comment 19•15 years ago
|
||
(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•15 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•15 years ago
|
tracking-fennec: --- → 2.0b2+
Assignee | ||
Comment 21•15 years ago
|
||
Attachment #483693 -
Attachment is obsolete: true
Attachment #483697 -
Flags: ui-review+
Attachment #483697 -
Flags: review+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 22•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Keywords: checkin-needed
Target Milestone: --- → mozilla2.0b8
Updated•15 years ago
|
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Comment 23•14 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•14 years ago
|
||
Doh, that was me, fwiw.
You need to log in
before you can comment on or make changes to this bug.
Description
•