Closed
Bug 717538
Opened 11 years ago
Closed 10 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•11 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•10 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•10 years ago
|
Assignee: nobody → mh+mozilla
Assignee | ||
Comment 3•10 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•10 years ago
|
||
Attachment #791614 -
Flags: review?(ted)
Assignee | ||
Updated•10 years ago
|
Attachment #791612 -
Attachment is obsolete: true
Attachment #791612 -
Flags: review?(ted)
Comment 6•10 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•10 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•10 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•10 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•10 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•10 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•10 years ago
|
||
With the change to nsAppRunner.cpp for MOZ_DEBUG builds
Attachment #795852 -
Flags: review?(ted)
Assignee | ||
Updated•10 years ago
|
Attachment #791614 -
Attachment is obsolete: true
Comment 13•10 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•10 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•10 years ago
|
||
Let's try again like this.
Attachment #796444 -
Flags: review?(ted)
Assignee | ||
Updated•10 years ago
|
Attachment #795852 -
Attachment is obsolete: true
Comment 16•10 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•10 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•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a6ed3ef085a
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8a6ed3ef085a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 20•10 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
•