bug 444103 - add ability to add string-formatted notes to crash report

2008-07-25 16:54:41 -04:00 · 2008-07-25 16:54:41 -04:00 · 38dc60766a
--- a/toolkit/crashreporter/nsExceptionHandler.cpp
+++ b/toolkit/crashreporter/nsExceptionHandler.cpp
@ -136,6 +136,7 @@ static const int kTimeSinceLastCrashParameterLen =
 // this holds additional data sent via the API
 static nsDataHashtable<nsCStringHashKey,nsCString>* crashReporterAPIData_Hash;
 static nsCString* crashReporterAPIData = nsnull;
+static nsCString* notesField = nsnull;

 static XP_CHAR*
 Concat(XP_CHAR* str, const XP_CHAR* toAppend, int* size)
@ -340,6 +341,9 @@ nsresult SetExceptionHandler(nsILocalFile* aXREDirectory,
  rv = crashReporterAPIData_Hash->Init();
  NS_ENSURE_SUCCESS(rv, rv);

+  notesField = new nsCString();
+  NS_ENSURE_TRUE(notesField, NS_ERROR_OUT_OF_MEMORY);
+
  // locate crashreporter executable
  nsCOMPtr<nsIFile> exePath;
  rv = aXREDirectory->Clone(getter_AddRefs(exePath));
@ -696,6 +700,11 @@ nsresult UnsetExceptionHandler()
    crashReporterAPIData = nsnull;
  }

+  if (notesField) {
+    delete notesField;
+    notesField = nsnull;
+  }
+
  if (crashReporterPath) {
    NS_Free(crashReporterPath);
    crashReporterPath = nsnull;
@ -746,7 +755,7 @@ static PLDHashOperator PR_CALLBACK EnumerateEntries(const nsACString& key,
  return PL_DHASH_NEXT;
 }

-nsresult AnnotateCrashReport(const nsACString &key, const nsACString &data)
+nsresult AnnotateCrashReport(const nsACString& key, const nsACString& data)
 {
  if (!gExceptionHandler)
    return NS_ERROR_NOT_INITIALIZED;
@ -778,8 +787,34 @@ nsresult AnnotateCrashReport(const nsACString &key, const nsACString &data)
  return NS_OK;
 }

+nsresult AppendAppNotesToCrashReport(const nsACString& data)
+{
+  if (!gExceptionHandler)
+    return NS_ERROR_NOT_INITIALIZED;
+
+  if (DoFindInReadable(data, NS_LITERAL_CSTRING("\0")))
+    return NS_ERROR_INVALID_ARG;
+
+  notesField->Append(data);
+  return AnnotateCrashReport(NS_LITERAL_CSTRING("Notes"), *notesField);
+}
+
+// Returns true if found, false if not found.
+bool GetAnnotation(const nsACString& key, nsACString& data)
+{
+  if (!gExceptionHandler)
+    return NS_ERROR_NOT_INITIALIZED;
+
+  nsCAutoString entry;
+  if (!crashReporterAPIData_Hash->Get(key, &entry))
+    return false;
+
+  data = entry;
+  return true;
+}
+
 nsresult
-SetRestartArgs(int argc, char **argv)
+SetRestartArgs(int argc, char** argv)
 {
  if (!gExceptionHandler)
    return NS_OK;
--- a/toolkit/crashreporter/nsExceptionHandler.h
+++ b/toolkit/crashreporter/nsExceptionHandler.h
@ -54,8 +54,9 @@ nsresult SetExceptionHandler(nsILocalFile* aXREDirectory,
                             const char* aServerURL);
 nsresult SetMinidumpPath(const nsAString& aPath);
 nsresult UnsetExceptionHandler();
-nsresult AnnotateCrashReport(const nsACString &key, const nsACString &data);
-nsresult SetRestartArgs(int argc, char **argv);
+nsresult AnnotateCrashReport(const nsACString& key, const nsACString& data);
+nsresult AppendAppNotesToCrashReport(const nsACString& data);
+nsresult SetRestartArgs(int argc, char** argv);
 nsresult SetupExtraData(nsILocalFile* aAppDataDirectory,
                        const nsACString& aBuildID);
 #ifdef XP_WIN32
--- a/toolkit/crashreporter/test/TestCrashReporterAPI.cpp
+++ b/toolkit/crashreporter/test/TestCrashReporterAPI.cpp
@ -49,27 +49,36 @@
 #include "nsILocalFile.h"

 #include "nsExceptionHandler.h"
-#include "nsICrashReporter.h"

-#define mu_assert(message, test) do { if (NS_FAILED(test)) \
-                                       return message; } while (0)
-#define mu_assert_failure(message, test) do { if (NS_SUCCEEDED(test)) \
-                                               return message; } while (0)
-#define mu_run_test(test) do { char *message = test(); tests_run++; \
-                                if (message) return message; } while (0)
+// Defined in nsExceptionHandler.cpp, but not normally exposed
+namespace CrashReporter {
+  bool GetAnnotation(const nsACString& key, nsACString& data);
+};
+
+#define ok(message, test) do {                   \
+                               if (!(test))      \
+                                 return message; \
+                             } while (0)
+#define equals(message, a, b) ok(message, a == b)
+#define ok_nsresult(message, rv) ok(message, NS_SUCCEEDED(rv))
+#define fail_nsresult(message, rv) ok(message, NS_FAILED(rv))
+#define run_test(test) do { char *message = test(); tests_run++; \
+                            if (message) return message; } while (0)
 int tests_run;

+
+
 char *
 test_init_exception_handler()
 {
  nsCOMPtr<nsILocalFile> lf;
  // we don't plan on launching the crash reporter in this app anyway,
  // so it's ok to pass a bogus nsILocalFile
-  mu_assert("NS_NewNativeLocalFile", NS_NewNativeLocalFile(EmptyCString(),
-                                                           PR_TRUE,
-                                                           getter_AddRefs(lf)));
+  ok_nsresult("NS_NewNativeLocalFile", NS_NewNativeLocalFile(EmptyCString(),
+                                                             PR_TRUE,
+                                                             getter_AddRefs(lf)));

-  mu_assert("CrashReporter::SetExceptionHandler",
+  ok_nsresult("CrashReporter::SetExceptionHandler",
            CrashReporter::SetExceptionHandler(lf, nsnull));
  return 0;
 }
@ -81,19 +90,19 @@ test_set_minidump_path()
  nsCOMPtr<nsIProperties> directoryService = 
    do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv);

-  mu_assert("do_GetService", rv);
+  ok_nsresult("do_GetService", rv);

  nsCOMPtr<nsILocalFile> currentDirectory;
  rv = directoryService->Get(NS_XPCOM_CURRENT_PROCESS_DIR,
                             NS_GET_IID(nsILocalFile),
                             getter_AddRefs(currentDirectory));
-  mu_assert("directoryService->Get", rv);
+  ok_nsresult("directoryService->Get", rv);

  nsAutoString currentDirectoryPath;
  rv = currentDirectory->GetPath(currentDirectoryPath);
-  mu_assert("currentDirectory->GetPath", rv);
+  ok_nsresult("currentDirectory->GetPath", rv);
  
-  mu_assert("CrashReporter::SetMinidumpPath",
+  ok_nsresult("CrashReporter::SetMinidumpPath",
            CrashReporter::SetMinidumpPath(currentDirectoryPath));

  return 0;
@ -102,17 +111,62 @@ test_set_minidump_path()
 char *
 test_annotate_crash_report_basic()
 {
-  mu_assert("CrashReporter::AnnotateCrashReport: basic",
+  ok_nsresult("CrashReporter::AnnotateCrashReport: basic 1",
     CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("test"),
                                        NS_LITERAL_CSTRING("some data")));

+
+  nsCAutoString result;
+  ok("CrashReporter::GetAnnotation", CrashReporter::GetAnnotation(NS_LITERAL_CSTRING("test"),
+                                                                  result));
+  nsCString msg = result + NS_LITERAL_CSTRING(" == ") +
+    NS_LITERAL_CSTRING("some data");
+  equals((char*)PromiseFlatCString(msg).get(), result,
+         NS_LITERAL_CSTRING("some data"));
+
+  // now replace it with something else
+  ok_nsresult("CrashReporter::AnnotateCrashReport: basic 2",
+     CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("test"),
+                                        NS_LITERAL_CSTRING("some other data")));
+
+
+  ok("CrashReporter::GetAnnotation", CrashReporter::GetAnnotation(NS_LITERAL_CSTRING("test"),
+                                                                  result));
+  msg = result + NS_LITERAL_CSTRING(" == ") +
+    NS_LITERAL_CSTRING("some other data");
+  equals((char*)PromiseFlatCString(msg).get(), result,
+         NS_LITERAL_CSTRING("some other data"));
+  return 0;
+}
+
+char *
+test_appendnotes_crash_report()
+{
+  // Append two notes
+  ok_nsresult("CrashReporter::AppendAppNotesToCrashReport: 1",
+              CrashReporter::AppendAppNotesToCrashReport(NS_LITERAL_CSTRING("some data")));
+
+  
+  ok_nsresult("CrashReporter::AppendAppNotesToCrashReport: 2",
+              CrashReporter::AppendAppNotesToCrashReport(NS_LITERAL_CSTRING("some other data")));
+
+  // ensure that the result is correct
+  nsCAutoString result;
+  ok("CrashReporter::GetAnnotation",
+     CrashReporter::GetAnnotation(NS_LITERAL_CSTRING("Notes"),
+                                  result));
+
+  nsCString msg = result + NS_LITERAL_CSTRING(" == ") +
+    NS_LITERAL_CSTRING("some datasome other data");
+  equals((char*)PromiseFlatCString(msg).get(), result,
+         NS_LITERAL_CSTRING("some datasome other data"));
  return 0;
 }

 char *
 test_annotate_crash_report_invalid_equals()
 {
-  mu_assert_failure("CrashReporter::AnnotateCrashReport: invalid = in key",
+  fail_nsresult("CrashReporter::AnnotateCrashReport: invalid = in key",
     CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("test=something"),
                                        NS_LITERAL_CSTRING("some data")));
  return 0;
@ -121,7 +175,7 @@ test_annotate_crash_report_invalid_equals()
 char *
 test_annotate_crash_report_invalid_cr()
 {
-  mu_assert_failure("CrashReporter::AnnotateCrashReport: invalid \n in key",
+  fail_nsresult("CrashReporter::AnnotateCrashReport: invalid \n in key",
     CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("test\nsomething"),
                                        NS_LITERAL_CSTRING("some data")));
  return 0;
@ -130,19 +184,20 @@ test_annotate_crash_report_invalid_cr()
 char *
 test_unset_exception_handler()
 {
-  mu_assert("CrashReporter::UnsetExceptionHandler",
+  ok_nsresult("CrashReporter::UnsetExceptionHandler",
            CrashReporter::UnsetExceptionHandler());
  return 0;
 }

 static char* all_tests()
 {
-  mu_run_test(test_init_exception_handler);
-  mu_run_test(test_set_minidump_path);
-  mu_run_test(test_annotate_crash_report_basic);
-  mu_run_test(test_annotate_crash_report_invalid_equals);
-  mu_run_test(test_annotate_crash_report_invalid_cr);
-  mu_run_test(test_unset_exception_handler);
+  run_test(test_init_exception_handler);
+  run_test(test_set_minidump_path);
+  run_test(test_annotate_crash_report_basic);
+  run_test(test_annotate_crash_report_invalid_equals);
+  run_test(test_annotate_crash_report_invalid_cr);
+  run_test(test_appendnotes_crash_report);
+  run_test(test_unset_exception_handler);
  return 0;
 }

@ -155,10 +210,10 @@ main (int argc, char **argv)

  char* result = all_tests();
  if (result != 0) {
-    printf("FAIL: %s\n", result);
+    printf("TEST-UNEXPECTED-FAIL | %s | %s\n", __FILE__, result);
  }
  else {
-    printf("ALL TESTS PASSED\n");
+    printf("TEST-PASS | %s | all tests passed\n", __FILE__);
  }
  printf("Tests run: %d\n", tests_run);
 
--- a/toolkit/xre/nsAppRunner.cpp
+++ b/toolkit/xre/nsAppRunner.cpp
@ -836,6 +836,12 @@ nsXULAppInfo::AnnotateCrashReport(const nsACString& key,
  return CrashReporter::AnnotateCrashReport(key, data);
 }

+NS_IMETHODIMP
+nsXULAppInfo::AppendAppNotesToCrashReport(const nsACString& data)
+{
+  return CrashReporter::AppendAppNotesToCrashReport(data);
+}
+
 NS_IMETHODIMP
 nsXULAppInfo::WriteMinidumpForException(void* aExceptionInfo)
 {
--- a/xpcom/system/nsICrashReporter.idl
+++ b/xpcom/system/nsICrashReporter.idl
@ -59,6 +59,19 @@ interface nsICrashReporter : nsISupports
   *                             '\n'.  Invalid character for data is '\0'.
   */
  void annotateCrashReport(in ACString key, in ACString data);
+
+  /**
+   * Append some data to the "Notes" field, to be submitted with a crash report.
+   * Unlike annotateCrashReport, this method will append to existing data.
+   *
+   * @param data
+   *        Data to be added.
+   *
+   * @throw NS_ERROR_NOT_INITIALIZED if crash reporting not initialized
+   * @throw NS_ERROR_INVALID_ARG if or data contains invalid characters.
+   *                             The only invalid character is '\0'.
+   */
+  void appendAppNotesToCrashReport(in ACString data);
  
  /**
    * Write a minidump immediately, with the user-supplied exception