Closed
Bug 745154
Opened 13 years ago
Closed 12 years ago
Disable automatic safe mode after startup crashes in debug builds or using an environment variable
Categories
(Toolkit :: Startup and Profile System, defect)
Toolkit
Startup and Profile System
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: smaug, Assigned: billm)
References
Details
(Whiteboard: MOZ_DISABLE_AUTO_SAFE_MODE)
Attachments
(2 files, 1 obsolete file)
855 bytes,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
2.95 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
The dialog is very annoying, and rarely useful.
Comment 1•13 years ago
|
||
Which dialog, exactly?
Comment 2•13 years ago
|
||
Does the text start with "We sincerely apologize for the inconvenience."?
Comment 3•12 years ago
|
||
Comment 4•12 years ago
|
||
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?)
Attachment #716171 -
Flags: review?(gavin.sharp) → review?(mnoorenberghe+bmo)
Comment 5•12 years ago
|
||
Comment on attachment 716171 [details] [diff] [review]
patch
Otherwise, defaulting to false ifdef DEBUG and adding an environment variable seems reasonable to me.
Attachment #716171 -
Flags: feedback+
Comment 6•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Comment on attachment 716171 [details] [diff] [review]
patch
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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•12 years ago
|
||
(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?
Reporter | ||
Comment 11•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
review ping as Comment 10 has been addressed by 11&12
Comment 14•12 years ago
|
||
Can you attach a new patch that addresses comment 4?
Updated•12 years ago
|
Flags: needinfo?(bgirard)
Comment 15•12 years ago
|
||
Attachment #716171 -
Flags: review?(mnoorenberghe+bmo)
Assignee | ||
Comment 16•12 years ago
|
||
I ran into this today and it's really annoying.
Attachment #738841 -
Flags: review?(mnoorenberghe+bmo)
Updated•12 years ago
|
Comment 17•12 years ago
|
||
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.)
Attachment #738841 -
Flags: review?(mnoorenberghe+bmo) → review+
Comment 18•12 years ago
|
||
LGTM
Component: General → Startup and Profile System
OS: Linux → All
Product: Firefox → Toolkit
Hardware: x86_64 → All
Summary: Disable the silly "Nightly safe mode" dialog on debug builds by default → Disable automatic safe mode after startup crashes in debug builds or using an environment variable
Whiteboard: MOZ_DISABLE_AUTO_SAFE_MODE
Updated•12 years ago
|
Attachment #716171 -
Attachment is obsolete: true
Assignee | ||
Comment 19•12 years ago
|
||
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.
Attachment #739701 -
Flags: review?(ehsan)
Comment 20•12 years ago
|
||
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.
Attachment #739701 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 21•12 years ago
|
||
Comment 22•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•