745154 - Disable automatic safe mode after startup crashes in debug builds or using an environment variable

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

The dialog is very annoying, and rarely useful.

Comment 1

[Justin Dolske [:Dolske]]

Which dialog, exactly?

Comment 2

[Matthew N. [:MattN]]

Does the text start with "We sincerely apologize for the inconvenience."?

Comment 3

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 716171 [details] [diff] [review]
patch

I would prefer combining this with the lines that initially set mIsSafeModeNecessary rather than duplicating the set later on (just seems less confusing that way).

(Why is this code in two places, I wonder? Can we fix that too?)

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 716171 [details] [diff] [review]
patch

Otherwise, defaulting to false ifdef DEBUG and adding an environment variable seems reasonable to me.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 716171 [details] [diff] [review]
patch

I would like to know how people are running into this to know if there is a bug in the detection.  If this is E10S-related then we already have bug 749133.  Are you starting and killing (e.g. Force Quitting) Firefox consecutively without letting it run for 30s after startup?

Since we already have the pref toolkit.startup.max_resumed_crashes that can be set to -1 to disable this feature, do we really need to an environment variable?  Can't we just change the default value of the pref inside #ifdef DEBUG at https://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js?rev=a71e34325926#1177

That would mean people would lose their user-set value of -1 if they switch to a debug build and back to non-debug but that is an edgecase that probably isn't important.

Comment 7

[Andrew McCreight [:mccr8]]

For me this happens because I'm debugging something (a test case or a patch) that causes Firefox to instantly crash.  I didn't realize there was a pref.

Comment 8

[Benoit Girard (:BenWa)]

This and I often just hit CTRL+C when I missed something (breakpoint, rebuild etc...). Setting the preference in a profile is a solution but its a bit of a pain when working with multiple profiles.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 716171 [details] [diff] [review]
patch

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #4)
> (Why is this code in two places, I wonder? Can we fix that too?)

The comment explains it:
>diff --git a/toolkit/components/startup/nsAppStartup.cpp b/toolkit/components/startup/nsAppStartup.cpp
>   // recalculate since recent crashes count may have changed above
>   mIsSafeModeNecessary = (recentCrashes > maxResumedCrashes && maxResumedCrashes != -1);

BTW the mIsSafeModeNecessary override for debug builds could probably be done in nsAppStartup::GetAutomaticSafeModeNecessary if we don't go with the pref solution.

Comment 10

[Matthew N. [:MattN]]

(In reply to Benoit Girard (:BenWa) from comment #8)
> Setting the preference in a profile is a solution but its a
> bit of a pain when working with multiple profiles.

Setting the pref to -1 by default in debug builds should be fine though, right?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I'd prefer environment variable. Then I could set it to my script I run before doing any
moz development and I wouldn't need to care about the profile.
Similar to XPCOM_MEM_LEAK_LOG and MOZ_NO_REMOTE

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to Matthew N. [:MattN] from comment #10)
> Setting the pref to -1 by default in debug builds should be fine though,
> right?

That has the build-switching downside you mention. Adding an #ifdef DEBUG to the code seems pretty simple, I don't really see a downside. And the environment variable is useful for "debugging" opt builds, and often more convenient than a pref. I don't see much of a need to be conservative here, given the simplicity of the required changes.

Comment 13

[Benoit Girard (:BenWa)]

review ping as Comment 10 has been addressed by 11&12

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Can you attach a new patch that addresses comment 4?

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 716171 [details] [diff] [review]
patch

Also see the bottom of comment 9.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

I ran into this today and it's really annoying.

Comment 17

[(no longer active)]

Comment on attachment 738841 [details] [diff] [review]
patch v2

Review of attachment 738841 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, please!

(Oh, and it would be nice if you added a comment here explaining the DEBUG #ifdef.)

Comment 18

[Matthew N. [:MattN]]

LGTM

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v3

Unfortunately this broke the test that was checking that this feature was working. I still think it's really important to avoid this safe mode stuff by default in debug builds, so I added a preference to decide whether debug builds should completely skip safe mode checks. It's only set during the test.

Comment 20

[(no longer active)]

Comment on attachment 739701 [details] [diff] [review]
patch v3

Review of attachment 739701 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below.

::: toolkit/components/startup/nsAppStartup.cpp
@@ +833,5 @@
>    NS_ENSURE_ARG_POINTER(_retval);
>  
> +  bool alwaysSafe;
> +  nsresult rv = Preferences::GetBool(kPrefAlwaysUseSafeMode, &alwaysSafe);
> +  NS_ENSURE_SUCCESS(rv, rv);

This doesn't look right, since if the pref doesn't exist, you return without setting _retval.  Please init alwaysSafe to false, and stop checking the return value of GetBool.

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d3fb4987df7

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1d3fb4987df7