717538 - MOZILLA_OFFICIAL shouldn't be required to enable crash reporter

Status: RESOLVED FIXED

Description

[Mike Hommey [:glandium]]

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

[(not currently active) Ted Mielczarek]

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.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Enable crash reporter in application.ini with MOZ_CRASHREPORTER instead of MOZILLA_OFFICIAL

According to KaiRo, it's not a problem anymore that random people with crashreporter enabled send crash reports.

Comment 3

[Mike Hommey [:glandium]]

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.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Enable crash reporter in application.ini with MOZ_CRASHREPORTER instead of MOZILLA_OFFICIAL

Comment 6

[(not currently active) Ted Mielczarek]

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.

Comment 7

[Mike Hommey [:glandium]]

(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

[(not currently active) Ted Mielczarek]

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

[(not currently active) Ted Mielczarek]

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?

Comment 10

[Mike Hommey [:glandium]]

(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

[(not currently active) Ted Mielczarek]

(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.

Comment 12

[Mike Hommey [:glandium]]

Attachment: Enable crash reporter in application.ini with MOZ_CRASHREPORTER instead of MOZILLA_OFFICIAL

With the change to nsAppRunner.cpp for MOZ_DEBUG builds

Comment 13

[(not currently active) Ted Mielczarek]

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.

Comment 14

[Mike Hommey [:glandium]]

(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)

Comment 15

[Mike Hommey [:glandium]]

Attachment: Enable crash reporter in application.ini with MOZ_CRASHREPORTER instead of MOZILLA_OFFICIAL

Let's try again like this.

Comment 16

[(not currently active) Ted Mielczarek]

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?

Comment 17

[Mike Hommey [:glandium]]

(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.

Comment 18

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a6ed3ef085a

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a6ed3ef085a

Comment 20

[Octoploid]

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'