Closed Bug 385280 Opened 13 years ago Closed 12 years ago

should send proxy settings to the breakpad reporter

Categories

(Toolkit :: Crash Reporting, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dcamp, Assigned: alfred.peng)

References

Details

Attachments

(3 files, 2 obsolete files)

The linux reporter needs proxy settings for uploading minidumps.
Duplicate of this bug: 398688
Attached patch patch v1 (obsolete) — Splinter Review
ted, could we approach this proxy issue by adding something like the following line to application.ini: HttpProxy=http://your.proxy.com:18023/? And the patch will pass the proxy info to libcurl.
Attachment #316605 - Flags: review?(ted.mielczarek)
Is it more reasonable to get the proxy information from gconf?
Comment on attachment 316605 [details] [diff] [review]
patch v1

Getting the proxy info from gconf would be acceptable, it would match what we're doing on Mac/Windows. If you were going to pass the proxy info from the main application, I would prefer to do it in an environment variable. I'm not sure what you mean about putting the proxy in application.ini, that's not configured per-user.
Attachment #316605 - Flags: review?(ted.mielczarek) → review-
Duplicate of this bug: 430469
Attached patch patch v2Splinter Review
Get the proxy information from gconf.

1. This patch doesn't include the "Ignore Host List".
2. Still working on the lack of certificates on Solaris. Need to push some guy to deliver them. Provide a workaround in this patch. Not sure whether it's acceptable.
Attachment #316605 - Attachment is obsolete: true
Attachment #317974 - Flags: review?(ted.mielczarek)
Someone can tell me if this bug, when fixed, will cover Windows SO too ? or just Linux?

Thank you
The Windows crash reporter already uses the system proxy settings. (whatever you have configured for IE)
Comment on attachment 317974 [details] [diff] [review]
patch v2

r=me on the proxy bits, but r- on the SSL bits. You'll have to fix that on the OS side, disabling SSL auth is not a good solution.
Attachment #317974 - Flags: review?(ted.mielczarek) → review+
(Also the SSL bits are unrelated to this bug.)
Attached patch patch v2-2Splinter Review
Remove the r- part from the previous patch and ask for approval1.9. The patch enables sending crash report through http proxy from gconf for Linux/Solaris and should be safe.
Assignee: nobody → alfred.peng
Status: NEW → ASSIGNED
Attachment #318195 - Flags: approval1.9?
Comment on attachment 318195 [details] [diff] [review]
patch v2-2

a1.9+=damons
Attachment #318195 - Flags: approval1.9? → approval1.9+
Checking in toolkit/crashreporter/client/Makefile.in;
/cvsroot/mozilla/toolkit/crashreporter/client/Makefile.in,v  <--  Makefile.in
new revision: 1.18; previous revision: 1.17
done
Checking in toolkit/crashreporter/client/crashreporter_linux.cpp;
/cvsroot/mozilla/toolkit/crashreporter/client/crashreporter_linux.cpp,v  <--  crashreporter_linux.cpp
new revision: 1.20; previous revision: 1.19
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
This introduced a dependency (libgconf and libORBit) which we did not have before and is not listed http://developer.mozilla.org/en/docs/Linux_Build_Prerequisites or http://wiki.mozilla.org/Linux/Runtime_Requirements.  Or http://wiki.mozilla.org/Firefox3/Firefox_Requirements#Linux although that list has far more serious problems.
Andrew: we were already checking and using that in configure.in, afaict, this shouldn't have been anything new.
Ah, OK. toolkit/system/gnome also links against libgconf.  But configure.in automagically disables that if libgconf is not found, whereas crashreporter will happily try to compile (and fail).

Apparently people on gnome-less distros (slack, kubuntu, perhaps) have been compiling happily with defaultish options (of course, they wouldn't want crash reporter anyway, but...)
this isn't cool. it means that mozilla.org builds which should have been able to report crashes before will not be able to run the crash reporter on those systems.

this should be reworked as a dynamically loaded library.
timeless: doubtful, since those builds would be linked against gconf anyway, so they won't even start. I think we could fix things to not try to link against gconf if it's not present, but I think that's all we need to do.
please rework this as a dynamic load.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Follow up fix (obsolete) — Splinter Review
Ted, can we fix it this way?
Comment on attachment 318979 [details] [diff] [review]
Follow up fix

Almost. You'll need to:
1) add MOZ_ENABLE_GCONF to config/autoconf.mk.in
2) add an AC_DEFINE(MOZ_ENABLE_GCONF) in configure.in (inside an if block testing that it's set)

With those tweaks this should be ok.
Attachment #318979 - Flags: review-
Attached patch follow up fix v2Splinter Review
Attachment #318979 - Attachment is obsolete: true
Attachment #319107 - Flags: review?(ted.mielczarek)
Attachment #319107 - Flags: review?(ted.mielczarek) → review+
Attachment #319107 - Flags: approval1.9?
Comment on attachment 319107 [details] [diff] [review]
follow up fix v2

a1.9=beltzner
Attachment #319107 - Flags: approval1.9? → approval1.9+
Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1992; previous revision: 1.1991
done
Checking in config/autoconf.mk.in;
/cvsroot/mozilla/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 3.462; previous revision: 3.461
done
Checking in toolkit/crashreporter/client/Makefile.in;
/cvsroot/mozilla/toolkit/crashreporter/client/Makefile.in,v  <--  Makefile.in
new revision: 1.19; previous revision: 1.18
done
Checking in toolkit/crashreporter/client/crashreporter_linux.cpp;
/cvsroot/mozilla/toolkit/crashreporter/client/crashreporter_linux.cpp,v  <--  crashreporter_linux.cpp
new revision: 1.21; previous revision: 1.20
done
Is it fixed?
=> FIXED. Sorry for the delay.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
(In reply to comment #18)
> timeless: doubtful, since those builds would be linked against gconf anyway,
> so they won't even start.

They do start as the Gnome dependency is checked at runtime.

> I think we could fix things to not try to link against
> gconf if it's not present, but I think that's all we need to do.

We can and should do runtime detection (bug 434997).
There will be plenty of people running without a 32-bit libgconf installed.
Depends on: 434997
Sorry, that's my fault, I should have verified my assertions.
BTW, I'm not sure that Breakpad always works on a 64-bit system running a 32-bit binary anyway.
Depends on: 518244
For anyone looking for current information on why crash reports don't work through a proxy, see bug 1333125.
You need to log in before you can comment on or make changes to this bug.