Closed
Bug 717538
Opened 13 years ago
Closed 11 years ago
MOZILLA_OFFICIAL shouldn't be required to enable crash reporter
Categories
(Toolkit :: Crash Reporting, defect)
Toolkit
Crash Reporting
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file, 3 obsolete files)
Currently, you need to export MOZILLA_OFFICIAL=1 to actually enable the crash reporter. Considering the other things MOZILLA_OFFICIAL changes (most notably, it sets android:debuggable to false), I think it is wrong to use this variable.
My proposition is to make MOZ_CRASHREPORTER enable it in application.ini, and not setting MOZILLA_OFFICIAL either remove the submit url or make it a dummy one.
Comment 1•13 years ago
|
||
I used MOZILLA_OFFICIAL originally because I wanted two things:
1) Enabled at compile time for all developers.
2) Enabled at runtime only for nightly builds, not arbitrary developer builds.
Your change would reverse the latter, which I don't think is a good idea.
Assignee | ||
Comment 2•11 years ago
|
||
According to KaiRo, it's not a problem anymore that random people with crashreporter enabled send crash reports.
Attachment #791612 -
Flags: review?(ted)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → mh+mozilla
Assignee | ||
Comment 3•11 years ago
|
||
Note that makes http://hg.mozilla.org/mozilla-central/file/8ad1e4c838c8/toolkit/xre/nsAppRunner.cpp#l2973 moot. I could see a case for allowing to disable the crash reporter, though.
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #791614 -
Flags: review?(ted)
Assignee | ||
Updated•11 years ago
|
Attachment #791612 -
Attachment is obsolete: true
Attachment #791612 -
Flags: review?(ted)
Comment 6•11 years ago
|
||
The only thing that sucks about this is that it makes builds less debuggable. Breakpad interferes with debugging on OS X, and will probably cause unexpected behavior elsewhere.
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> The only thing that sucks about this is that it makes builds less
> debuggable. Breakpad interferes with debugging on OS X, and will probably
> cause unexpected behavior elsewhere.
I'm curious. How does it interfere with debugging on osx? As for elsewhere, at least on Linux, that's not the case afaik.
Comment 8•11 years ago
|
||
I believe because gdb relies on replacing the task's exception ports, which is what Breakpad does, and so you can't actually catch exceptions or something like that (it's been a while since I tried).
Comment 9•11 years ago
|
||
Comment on attachment 791614 [details] [diff] [review]
Enable crash reporter in application.ini with MOZ_CRASHREPORTER instead of MOZILLA_OFFICIAL
Review of attachment 791614 [details] [diff] [review]:
-----------------------------------------------------------------
I think this is generally fine, but I think maybe we should make this only for --disable-debug builds, so that Breakpad stays out of the way for people who explicitly want to debug their build.
::: toolkit/xre/nsAppRunner.cpp
@@ +2969,5 @@
> if (NS_FAILED(rv))
> return 1;
>
> #ifdef MOZ_CRASHREPORTER
> + {
What's up with this extra open brace?
Attachment #791614 -
Flags: review?(ted) → review+
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> Comment on attachment 791614 [details] [diff] [review]
> Enable crash reporter in application.ini with MOZ_CRASHREPORTER instead of
> MOZILLA_OFFICIAL
>
> Review of attachment 791614 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I think this is generally fine, but I think maybe we should make this only
> for --disable-debug builds, so that Breakpad stays out of the way for people
> who explicitly want to debug their build.
On which side? nsAppRunner or application.ini?
> ::: toolkit/xre/nsAppRunner.cpp
> @@ +2969,5 @@
> > if (NS_FAILED(rv))
> > return 1;
> >
> > #ifdef MOZ_CRASHREPORTER
> > + {
>
> What's up with this extra open brace?
Huh, no idea.
Comment 11•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #10)
> > I think this is generally fine, but I think maybe we should make this only
> > for --disable-debug builds, so that Breakpad stays out of the way for people
> > who explicitly want to debug their build.
>
> On which side? nsAppRunner or application.ini?
Whatever is simplest, as long as we get the desired behavior.
Assignee | ||
Comment 12•11 years ago
|
||
With the change to nsAppRunner.cpp for MOZ_DEBUG builds
Attachment #795852 -
Flags: review?(ted)
Assignee | ||
Updated•11 years ago
|
Attachment #791614 -
Attachment is obsolete: true
Comment 13•11 years ago
|
||
Comment on attachment 795852 [details] [diff] [review]
Enable crash reporter in application.ini with MOZ_CRASHREPORTER instead of MOZILLA_OFFICIAL
Review of attachment 795852 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/xre/nsAppRunner.cpp
@@ +2977,5 @@
> + if (!EnvHasValue("MOZ_CRASHREPORTER"))
> +#else
> + // In non-debug builds, enable the crash reporter by default, and allow
> + // to disable it with the MOZ_DISABLE_CRASHREPORTER env variable.
> + if (EnvHasValue("MOZ_DISABLE_CRASHREPORTER"))
Apparently we support such a thing already, but it's MOZ_CRASHREPORTER_DISABLE:
https://developer.mozilla.org/en-US/docs/Environment_variables_affecting_crash_reporting
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#805
I'm not sure you actually need this code block since that's handled in SetExceptionHandler.
Attachment #795852 -
Flags: review?(ted) → review+
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> I'm not sure you actually need this code block since that's handled in
> SetExceptionHandler.
Oh, good point. The MOZ_DEBUG case still needs to be added, though. (By the way, now that you made me look around, I found a separate bug in nsAppRunner.cpp)
Assignee | ||
Comment 15•11 years ago
|
||
Let's try again like this.
Attachment #796444 -
Flags: review?(ted)
Assignee | ||
Updated•11 years ago
|
Attachment #795852 -
Attachment is obsolete: true
Comment 16•11 years ago
|
||
Comment on attachment 796444 [details] [diff] [review]
Enable crash reporter in application.ini with MOZ_CRASHREPORTER instead of MOZILLA_OFFICIAL
Review of attachment 796444 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +806,5 @@
> + // In debug builds, disable the crash reporter by default, and allow to
> + // enable it with the MOZ_CRASHREPORTER environment variable.
> + const char *envvar = PR_GetEnv("MOZ_CRASHREPORTER");
> + if ((!envvar || !*envvar) && !force)
> + return NS_OK;
The code in nsAppRunner.cpp that handles MOZ_CRASHREPORTER is kind of obsolete as of this patch, no?
Attachment #796444 -
Flags: review?(ted) → review+
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #16)
> The code in nsAppRunner.cpp that handles MOZ_CRASHREPORTER is kind of
> obsolete as of this patch, no?
Kind of, it can still be useful when an non-compiled application.ini sets Enabled=0. If we consider that code obsolete, we might as well remove Enabled from application.ini and application.h.
Assignee | ||
Comment 18•11 years ago
|
||
Comment 19•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 20•11 years ago
|
||
This breaks the build for me (--disable-crashreporter):
...
/var/tmp/moz-build-dir/_virtualenv/bin/python /home/markus/mozilla-central/build/appini_header.py ../dist/bin/application.ini > application.ini.h
Traceback (most recent call last):
File "/home/markus/mozilla-central/build/appini_header.py", line 51, in <module>
main(sys.argv[1])
File "/home/markus/mozilla-central/build/appini_header.py", line 47, in main
};''' % appdata
KeyError: 'Crash Reporter:serverurl'
You need to log in
before you can comment on or make changes to this bug.
Description
•