MOZILLA_OFFICIAL shouldn't be required to enable crash reporter

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
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.
According to KaiRo, it's not a problem anymore that random people with crashreporter enabled send crash reports.
Attachment #791612 - Flags: review?(ted)
Assignee: nobody → mh+mozilla
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.
Attachment #791612 - Attachment is obsolete: true
Attachment #791612 - Flags: review?(ted)
Duplicate of this bug: 884267
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.
(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.
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 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+
(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.
(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.
With the change to nsAppRunner.cpp for MOZ_DEBUG builds
Attachment #795852 - Flags: review?(ted)
Attachment #791614 - Attachment is obsolete: true
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+
(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)
Attachment #795852 - Attachment is obsolete: true
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+
(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.
Depends on: 911154
https://hg.mozilla.org/mozilla-central/rev/8a6ed3ef085a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.