Closed Bug 380540 Opened 13 years ago Closed 13 years ago

crash reporter client for Linux

Categories

(Toolkit :: Crash Reporting, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: ted, Assigned: dcamp)

References

Details

Attachments

(4 files, 4 obsolete files)

We should get a crash reporter client working on Linux.
Depends on: 380541
fwiw, callion said that libcurl is probably installed by default on FC since OOo depends on it.
Attached patch WIP (obsolete) — Splinter Review
Here's a first stab at a linux client.  Need to figure out the libcurl stuff better before we commit it, what's in this patch is a bit of a hack.
Assignee: nobody → dcamp
Status: NEW → ASSIGNED
If we just build http_upload.cc into the static lib in airbag/src/common/linux/Makefile.in, it shouldn't be a problem, should it?  The linker will throw out those functions in the main app since they're not used.  Should be easy enough to test, since the libcurl lib isn't specified for the main app.
Attachment #268729 - Flags: review?(ted.mielczarek)
Comment on attachment 268729 [details] [diff] [review]
WIP

>diff -r e879df4187e6 toolkit/airbag/client/Makefile.in
> LIBS += \
>-  $(DEPTH)/toolkit/airbag/airbag/src/common/linux/$(LIB_PREFIX)breakpad_linux_common_s.$(LIB_SUFFIX) \
>-  $(NULL)
>+	$(DEPTH)/toolkit/airbag/airbag/src/common/linux/$(LIB_PREFIX)breakpad_linux_common_s.$(LIB_SUFFIX) \
>+	$(NULL)

Actually I think I specifically made this indent this way, since this is what I now prefer.  If you feel like changing it, change the other LIBS sections to match the smaller indent style instead.

> include $(topsrcdir)/config/rules.mk

>+
>+ifeq ($(OS_ARCH),Linux)
>+export:: $(srcdir)/../airbag/src/common/linux/http_upload.cc
>+	$(INSTALL) $^ .
>+endif

This sucks but it's not your fault. :)

>diff -r e879df4187e6 toolkit/airbag/client/crashreporter.cpp

>+bool CrashReporterReadStrings(istream& in, StringTable& strings, bool unescape)
>+bool CrashReporterReadStringsFromFile(const string& path,
>+bool CrashReporterWriteStrings(ostream& out,
>+bool CrashReporterWriteStringsToFile(const string& path,

I'd prefer namespace CrashReporter {} around this instead.  It's only 2 more characters in the function name then, and you can even do using namespace CrashReporter.
 
>diff -r e879df4187e6 toolkit/airbag/client/crashreporter_linux.cpp
>+  if (CrashReporterReadStringsFromFile(gSettingsPath + "/" + kIniFile,
>+                                       settings, true)) {
>+    gtk_entry_set_text(GTK_ENTRY(gEmailEntry), settings["Email"].c_str());

Any reason you don't do the same test here you do below it?  (if settings.find(foo) != settings.end())

>+static gboolean SendReportIdle(gpointer userData)
>+{
>+  string response;
>+  bool success = google_breakpad::HTTPUpload::SendRequest
>+    (gSendURL,
>+     gQueryParameters,
>+     gDumpFile,
>+     "upload_file_minidump",
>+     "", "",
>+     &response);

We should file a followup on passing a proxy through from the app if one is set.

> void UIShowCrashUI(const string& dumpfile,

I'm just going to pretend I didn't read this function, because it totally sucks to have to do all of that.  But again, not your fault.

Looks good aside from those few little things!  r=me
Attachment #268729 - Flags: review?(ted.mielczarek) → review+
Attached patch updated patchSplinter Review
Attachment #268729 - Attachment is obsolete: true
(In reply to comment #4)
> >diff -r e879df4187e6 toolkit/airbag/client/crashreporter.cpp
> 
> >+bool CrashReporterReadStrings(istream& in, StringTable& strings, bool unescape)
> >+bool CrashReporterReadStringsFromFile(const string& path,
> >+bool CrashReporterWriteStrings(ostream& out,
> >+bool CrashReporterWriteStringsToFile(const string& path,
> 
> I'd prefer namespace CrashReporter {} around this instead.  It's only 2 more
> characters in the function name then, and you can even do using namespace
> CrashReporter.

I moved most of crashreporter.cpp into that namespace (everything but main()).

> >diff -r e879df4187e6 toolkit/airbag/client/crashreporter_linux.cpp
> >+  if (CrashReporterReadStringsFromFile(gSettingsPath + "/" + kIniFile,
> >+                                       settings, true)) {
> >+    gtk_entry_set_text(GTK_ENTRY(gEmailEntry), settings["Email"].c_str());
> 
> Any reason you don't do the same test here you do below it?  (if
> settings.find(foo) != settings.end())

The default string is empty, so setting it has no effect.  But I put in a check there for consistency. 
 
> >+static gboolean SendReportIdle(gpointer userData)
> >+{
> >+  string response;
> >+  bool success = google_breakpad::HTTPUpload::SendRequest
> >+    (gSendURL,
> >+     gQueryParameters,
> >+     gDumpFile,
> >+     "upload_file_minidump",
> >+     "", "",
> >+     &response);
> 
> We should file a followup on passing a proxy through from the app if one is
> set.

filed bug 385280
Looks good.  The current tinderbox might not have libcurl (I'm not sure), but the new refplatform definitely does, so as long as we're not building this by default, we're ok right now.  I'll check to see if the current tinderbox has libcurl, and if so, maybe we can enable sooner.
What headers/libs should we be looking for?
pkg-config --modversion libcurl

Should be sufficient to verify that it will work.
committed the updated patch, here's one to enable by default
Attachment #269250 - Flags: review?(ted.mielczarek)
Comment on attachment 269250 [details] [diff] [review]
enable breakpad by default on linux

We may want to disable building this on 64-bit linux, but I don't actually know how to test for that in configure.  I'm sure someone will complain about it if it breaks.
Attachment #269250 - Flags: review?(ted.mielczarek) → review+
This version falls back to curl-config if libcurl.pc isn't found.
Attachment #269250 - Attachment is obsolete: true
Attachment #269277 - Flags: review?(ted.mielczarek)
Comment on attachment 269277 [details] [diff] [review]
fall back to curl-config

Awesome, let's try this again.
Attachment #269277 - Flags: review?(ted.mielczarek) → review+
adds some #includes on linux and mac
Attachment #269294 - Flags: review?(ted.mielczarek)
Attachment #269294 - Flags: review?(ted.mielczarek) → review+
Attached patch update the manifest files (obsolete) — Splinter Review
This adds the crash reporter to linux's manifest file and updates the comment on windows
Depends on: 385531
Depends on: 385532
Attachment #269440 - Attachment is obsolete: true
(attached the wrong file)
Attachment #269455 - Attachment is obsolete: true
Depends on: 385544
(In reply to comment #11)
> (From update of attachment 269250 [details] [diff] [review])
> We may want to disable building this on 64-bit linux, but I don't actually know
> how to test for that in configure.  I'm sure someone will complain about it if
> it breaks.
> 

The correct way to test in configure would be to key off of HAVE_64BIT_OS.  This way if you are doing a build on a 64-bit OS, but have set CFLAGS=-m32 in order to do a 32-bit build it will figure this out and not disable airbag.
Comment on attachment 269456 [details] [diff] [review]
update mail manifest too

God I hate how this works.  :-(
Attachment #269456 - Flags: review+
Depends on: 385561
Attachment 269250 [details] [diff] broke --disable-compile-environment again, sadly. I guess the extra error message should be if test_libs or whatever that is called. Bug 383463 shows how I fixed this just recently.

This turned the trunk builds on http://l10n.mozilla.org/buildbot/ red.
(In reply to comment #20)
> Attachment 269250 [details] [diff] broke --disable-compile-environment again, sadly. I guess the
> extra error message should be if test_libs or whatever that is called. Bug
> 383463 shows how I fixed this just recently.
> 
> This turned the trunk builds on http://l10n.mozilla.org/buildbot/ red.

Axel, can you file a new bug on this?  This bug is getting a bit overused.  Let's track any remaining issues in dependencies.

Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Filed bug 385745 on --disable-compile-environment.
You need to log in before you can comment on or make changes to this bug.