Soh Kam Yung Reporter
	Description • 22 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.2b) Gecko/20021001 Phoenix/0.2
Build Identifier: Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.2b) Gecko/20021001 Phoenix/0.2

I like to be reminded by the pop-up security alert when I switch between a
normal (http URL) and secure (https URL) website.

In Phoenix, the preference to display this alert is not 'stickly'. The alert
will always be de-selected and I have to re-enable it to keep seeing the reminders.



Reproducible: Always

Steps to Reproduce:
1. go to https://login.yahoo.com/
2. a security warning window will pop-up, alerting me that I am going to a
secure web site
3. enable the "Alert me..." option
4. exit the website
5. re-enter the website
6. the security warning pops-up again
Actual Results:  
The "Alert me..." option is de-selected again

Expected Results:  
The "Alert me..." option should remain selected (as in Mozilla)

You may have to delete the user_pref("security.*") lines from the user.js file
first to get the alerts to appear.

	Asa Dotzler [:asa]
	Comment 1 • 22 years ago

blake, this is all you. we like the default but it shouldn't override user
selection. 

	Asa Dotzler [:asa]
	Comment 2 • 22 years ago

*** Bug 174835 has been marked as a duplicate of this bug. ***

	Asa Dotzler [:asa]
	Comment 3 • 22 years ago

*** Bug 178075 has been marked as a duplicate of this bug. ***

	William Stockall
	Comment 4 • 22 years ago

If one of the bugs marked as a duplicate of this one is marked for all platforms
and the reporter was using Windows, why is this bug showing as Linux specific? 
Just curious.

	Pierre Chanial
	Comment 5 • 22 years ago

as a reminder, the problem is here:
http://lxr.mozilla.org/seamonkey/source/security/manager/boot/src/nsSecurityWarningDialogs.cpp#155

	Jesse Ruderman
	Comment 6 • 22 years ago

*** Bug 199826 has been marked as a duplicate of this bug. ***

	aynilove [So, Jae-yoon]
	Comment 7 • 22 years ago

> as a reminder, the problem is here:

then, simply delete line #155 to #157 will solve this problem - Why Not?

	Ben Goodger (use ben at mozilla dot org for email)
	Comment 8 • 22 years ago

-> 0.7

	Mike Auty
	Comment 9 • 22 years ago

I'm sorry, I've been unable to create a diff file, however thanks to the
extremely useful link provided by pch, I can describe a patch which will still
give the intended default but will not override the user's choice.
Firstly (as pointed out by aynilove) remove lines #155 through to #157, then
replace line #135

if (NS_FAILED(rv)) prefValue = PR_TRUE;

with

#ifdef MOZ_PHOENIX
if (NS_FAILED(rv)) prefValue = PR_FALSE;
#else
if (NS_FAILED(rv)) prefValue = PR_TRUE;
#endif

(Again, I'm sorry I wasn't able to supply a diff.  I've read through a lot of
material on the site about helping and the requirements for patches to get
considered are difficult to achieve even though I know enough to offer a patch
for simple bugs like this.  Could anyone mail me help or ideas to simplify the
task?)

	Robert Parenton
	Comment 11 • 22 years ago

Comment on attachment 123100 [details] [diff] [review]
Patch based on Mike Auty's fix

Requesting review from Blake Ross

	Robert Parenton
	Comment 12 • 22 years ago

Comment on attachment 123100 [details] [diff] [review]
Patch based on Mike Auty's fix

Build Firebird with this patch included on both Windows and Linux.  On both
platforms it fixed the problem with no apparant side effects.  We should
probably try to get this in 0.6 if possible now that we have a working patch.

	Brian Ryner (not reading) Assignee
	Comment 13 • 22 years ago

Comment on attachment 123100 [details] [diff] [review]
Patch based on Mike Auty's fix

Actually... why don't we just set the default to false in firebird's all.js and
get rid of the #ifdef altogether?

	Brian Ryner (not reading) Assignee
	Comment 14 • 22 years ago

I think this should do the trick.

	Brian Ryner (not reading) Assignee
	Comment 15 • 22 years ago

checked in.

	Mike Auty
	Comment 16 • 22 years ago

The new proposed patch doesn't seem to have the same functionality.  This will
turn off the security dialogs, and require the user to specifically turn them on
before they can be seen.  The previous patch shows the dialog once, and then
defaults to false while allowing the user to choose.  This patch should be
backed out if possible.

	Sven Grull
	Comment 17 • 22 years ago

I agree with comment #16. Security warnings always should be activated by
default so the user has to think about it and then decide whether he will turn
them off.

	Owen Marshall (Not reading bugmail)
	Comment 18 • 22 years ago

Reopening based upon comment 16 and comment 17. 

If this is what the patch is causing, this isn't good at all. Opting in for the 
security info will not be welcomed.

	Mike Auty
	Comment 19 • 22 years ago

The first patch mentioned in Comment #10 (which has review+ from bryner), should
work, the second patch was causing the problems.  That needs backing out.  Can
anyone super-review the first patch please?

	Owen Marshall (Not reading bugmail)
	Comment 20 • 22 years ago

We also need a patch for backing out the one that was checked in. pch, can you 
take a look? Or bryner?

--> bryner

	Brian Ryner (not reading) Assignee
	Comment 21 • 22 years ago

Sorry, I did misunderstand the intent here.

I think the problem is that we actually need 3 states for the pref:

1. default: the dialog is shown once, but the default is to not show it again
2. the user chooses to display the dialog always
3. the user chooses to never display the dialog

The patch that I marked review+ on attempts to express these 3 states by making
failure to get the default pref value correspond to state #1.  I don't think
that will work unless we also removed the default pref values from
security-prefs.js.

	Mike Auty
	Comment 22 • 22 years ago

I think you're right, it probably won't work without removing the defaults from
security-prefs.js.  Sorry I didn't catch that first time round, I'm a bit new to
the code as you can probably tell.

It feels a bit like we're mangling the code to get it to do what we want. 
Perhaps the preference should be an integer:

0 - No alert
1 - Alert
2 - Alert once, then default to off
Anything else - Alert

The reason for this ordering is to keep the 0=false, 1=true idea.  Is this the
only code that changing this would affect, or will there be more changes
required throughout the source?  I have no idea which is better coding practice,
hardcode the default, or turn the preference into a 3 state option?  Any
suggestions?

	Brian Ryner (not reading) Assignee
	Comment 23 • 22 years ago

I suspect changing it to an integer pref may cause problems with upgrades. 
Conrad, can you confirm that?

If that's problematic, we may have to make another set of "first time" prefs (ugh).

	Pierre Chanial
	Comment 24 • 22 years ago

or we could have only two integer prefs (first time + let's stick the alert) and
use their bits instead for the set of warnings.

	Sven Grull
	Comment 25 • 22 years ago

Re Comment #21:

>I don't think that will work unless we also removed the default pref
>values from security-prefs.js.

I created a new profile and visited a secure site, there was no popup. So I
checked about:config and all security.warn_* were set to false. 
I don't know which pref is higher rated, all.js or security-prefs.js. But after
my try it seems that all.js overrules the other prefs.

	aynilove [So, Jae-yoon]
	Comment 26 • 22 years ago

>Perhaps the preference should be an integer:
>
>0 - No alert
>1 - Alert
>2 - Alert once, then default to off
>Anything else - Alert
>
>The reason for this ordering is to keep the 0=false, 1=true idea.

In same, it can seperated in two pref.

 One pref. determine (Show dialog or Not)
 Other pref. determines (Checked or Unchecked)

Like IE does, Dialog is open only once, and  "alert me whenever" is NOT checked.
But user check it and click a command button, this decision will saved,
and next time dialog showed it should checked.

It's not a simple question than I supposed before.

BUT! Why we not use mozilla's defult dialog form that works well?

	Mike Auty
	Comment 27 • 22 years ago

We could separate it into two prefs, but that would be two prefs for each of the
five dialogs.  Requiring it to store 10 prefs, rather than 5.  Also, after the
first time the dialog shows up, the "show tick mark" pref is rather pointless. 
If they kept it once (show dialog), they're likely to want to keep it again
(checked), and if not then it doesn't matter what the checked or unchecked pref
is, the checkbox won't be seen again.

The mozilla's dialog default is to check the box (making the problem far
simpler) but most people only want the box once and would like it not to show
again (as you said IE behaves).

It may work out that the easiest course of action would be to remove the prefs
from security-prefs.js and all.js, and apply the first patch.  The only thing
we're waiting on is for someone to decide if this is a good decision or whether
it would be better to rewrite the thing to use some sort of preference
mechanism, and if so what the best way of doing that would be.

In the short term, could someone reverse the diff in the second patch, so that
it doesn't affect the nightlies.  Also does anyone know how to sort out
security-prefs.js so that it only affects firebird?  Thanks...

File reference:
http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/security-prefs.js
Lines 39 through 43 (the security.warn prefs)

	Conrad Carlen (not reading bugmail)
	Comment 28 • 22 years ago

> Also does anyone know how to sort out
security-prefs.js so that it only affects firebird?

Yes. The default pref files are interpreted in reverse-sort order. See
http://lxr.mozilla.org/seamonkey/source/modules/libpref/src/nsPrefService.cpp#684.
The pref which is read last overrides the previous value. So, for firebird, make
a file called "security-prefs-firebird.js" and it will do what the 1st patch did
but without an #ifdef. This is what Camino does in order to differ from
mozilla's all.js.

	Mike Auty
	Comment 29 • 22 years ago

I'm not sure that will achieve what we need.  The problem is to not specify a
default (ie have no value, either true or false) in *any* of the prefs files. 
Obviously it would be best only to have this for Firebird, so I was wondering
how to pack firebird with a different security-prefs.js and all.js than any
other package?  The other packages *should* cope without defaults, but it's
probably nicer to just remove the prefs for firebird.  Has anybody taken out the
changes made to all.js yet?

	Robert Parenton
	Comment 30 • 22 years ago

Comment on attachment 123100 [details] [diff] [review]
Patch based on Mike Auty's fix

Removed the obsolete flag on the first patch as it worked as desired.

	Robert Parenton
	Comment 31 • 22 years ago

Brian/Blake,

Can you review the first patch (Patch based on Mike Auty's fix) and check it in
if if looks ok.  I previously built FB with it (first patch only) on Windows and
Linux and it achieved the desired results on both platforms when creating a new
profile.

Also, it appears that the second patch (get rid of the #ifdef) has not been
backed out.  For the first patch to work, it is necessary to back out the second
patch.

I can build this on Windows and Linux if you want me to test it again before
rolling back the second patch and checking in the first.  Also, I would be glad
to test on both Windows and Linux after it is checked in.

	Robert Parenton
	Comment 32 • 22 years ago

This patch rolls back the second patch and applies the first.  The result is
that the dialogs will appear by default, and will be checked by default.  This
has been tested on Linux and will be tested on Windows.  I will report back if
the effects are different on Windows.

>It may work out that the easiest course of action would be to remove the prefs

>from security-prefs.js and all.js, and apply the first patch.

I just tried this and built FB.  The result was that the dialogs never
appeared.  It looks like it treats not having a pref set the same as having one
set as false.

	Robert Parenton
	Comment 33 • 22 years ago

After rereading this entire bug and getting a better understanding about the
desired effect of removing the security.warn prefs from all.js and
security-prefs.js, I rebuilt FB from scratch again, using the third patch, and
also removing the relevant lines from security-prefs.js.  Once again, I got the
same result as in Comment #32, that is no warning dialogs were displayed at all.
 I tested this on Linux and am building it on Windows right now.  I'm thinking
that some other code elsewhere changed that is changing the behavior of Mike
Auty's proposed fix.

	Robert Parenton
	Comment 34 • 22 years ago

Comment on attachment 127135 [details] [diff] [review]
Patch rolling back the second patch and applying the first (Dialogs will appear by default and be checked by default)

I have almost completed a patch that makes the default unchecked, but will not
override the user's preference (too tired to finish it tonight).  The only
caveat is that the security.warn prefs must be removed from all.js and
security-prefs.js.  This should not cause a problem for the Mozilla Suite, but
I don't know how it would effect Camino.

I was thinking that we could add a flag for the makefile (or where ever it
needs to be) that would use a different security-prefs.js for FB.  Something
similar to the way Windows and Unix get different versions of radio.css:

http://lxr.mozilla.org/mozilla/source/toolkit/skin/win/radio.css
http://lxr.mozilla.org/mozilla/source/toolkit/skin/unix/radio.css

If this isn't possible, I think we should try to get approval to remove the
security.warn prefs from security-prefs.js (Brian?, Asa?, Conrad?).

	Robert Parenton
	Comment 35 • 22 years ago

This patch should fix the problem.  It uses the method Mike Auty suggested for
determining the state of the checkbox and whether or not to show the warning
dialogs.

To do this, I created netwerk/base/public/firebird/security-prefs.js, which is
identical to netwerk/base/public/security-prefs.js, except the security.warn
prefs have been removed.  I then modified netwerk/base/public/Makefile.in to
use netwerk/base/public/firebird/security-prefs.js when MOZ_PHOENIX is defined.
 If MOZ_PHOENIX is not defined, it uses netwerk/base/public/security-prefs.js
as usual.  This should cause the removal of security.warn prefs in
security-prefs.js for Firebird only.

I am attaching netwerk/base/public/firebird/security-prefs.js since CVS gave an
error when I included it in the diff.  The netwerk/base/public/firebird
directory will need to be created and that file will need to be added to it in
addition to checking in the diff.

	Robert Parenton
	Comment 36 • 22 years ago

Comment on attachment 127386 [details] [diff] [review]
Patch that rolls back the addition of prefs to all.js and completely fixes the bug

Requesting patch review from Brian Ryner.

	Robert Parenton
	Comment 37 • 22 years ago

This file is the security-prefs.js that should be added to
netwerk/base/public/firebird (after that directory is created).  This needs to
be done in addition to checking in the above patch.

	Robert Parenton
	Comment 38 • 22 years ago

Comment on attachment 127387 [details]
netwerk/base/public/firebird/security-prefs.js that goes with the above patch (needs to be copied to netwerk/base/public/firebird)

Requesting review of addition required to Firebird by the patch for this bug
from Brian Ryner.

	Robert Parenton
	Comment 39 • 22 years ago

Just finished building and testing on both Linux and Windows.  The
patch+security-prefs.js combination fixed the bug on both platforms.

If anybody else could test with this patch combination, it would be greatly
appreciated.

	Robert Parenton
	Comment 40 • 22 years ago

Comment on attachment 127386 [details] [diff] [review]
Patch that rolls back the addition of prefs to all.js and completely fixes the bug

Requesting review from Blake Ross.

	Robert Parenton
	Comment 41 • 22 years ago

Comment on attachment 127387 [details]
netwerk/base/public/firebird/security-prefs.js that goes with the above patch (needs to be copied to netwerk/base/public/firebird)

Requesting review from Blake Ross.

	Pierre Chanial
	Comment 42 • 22 years ago

NO #IFDEF PLEASE !!!

	Robert Parenton
	Comment 43 • 22 years ago

Pch, I don't quite understand which #ifdef you are referring to.  Are you
talking about the one in nsSecurityWarningDialogs.cpp or the ifdef in Makefile.in?

Since you said #ifdef, I'm assuming that you mean the ones in
nsSecurityWarningDialogs.cpp.  If it is undesirable to have the #ifdef is that
file, it seems like the only way to make this work would be to fork the
nsSecurityWarningDialogs.cpp and add an ifdef to the appropriate Makefile.in. 
Is this correct and a more desirable solution?

	Mike Connor [:mconnor]
	Comment 44 • 21 years ago

taking QA contact, sorry about the bugspam

	Mike Connor [:mconnor]
	Comment 45 • 21 years ago

While I like, in theory, the idea of user opt-in on first warning, I don't think
that we can proceed with that without having UI to enable it.

The question of how to implement this properly without #ifdefs isn't trivial, so
I don't think we're going to make these changes before 0.7, and leaving things
as-is would be detrimental to the release.  In the short term, can we reverse
the defaults to true on the warning prefs, and let users opt out?

bryner, your thoughts on this?

	Robert Parenton
	Comment 46 • 21 years ago

-> Mike

I have a working version of the patch using a fork of
nsSecurityWarningDialogs.cpp and security-prefs.js.  However, the only fork I
could get working was when I forked the entire directory in which
nsSecurityWarningDialogs.cpp resides, which doesn't seem to be a very good solution.

If anybody can help me with modifying the Makefile.in files and doing the
xpinstall modifications (if they're necessary), I think I could finish it up in
time for 0.7.

	Robert Parenton
	Comment 47 • 21 years ago

I have a working build of FB built from the current trunk with my updated fix
(only fork the nsSecurityWarningDialogs.cpp instead of its entire directory)
included.

It is a standard optimized Linux build, except it requires a PentiumII or higher
to run (built with --march=pentium2).

If anybody can help test it out while I get the diff for the patch made, I would
appreciate it.  YOU MUST USE A NEW PROFILE.

It is posted at www.louisianaada.org/MozillaFirebird-i686-pc-linux-gnu.tar.gz

	Robert Parenton
	Comment 48 • 21 years ago

Comment on attachment 127386 [details] [diff] [review]
Patch that rolls back the addition of prefs to all.js and completely fixes the bug

Removing obsolete patch review request.

	Robert Parenton
	Comment 49 • 21 years ago

Comment on attachment 127387 [details]
netwerk/base/public/firebird/security-prefs.js that goes with the above patch (needs to be copied to netwerk/base/public/firebird)

Removing obsolete patch review request.

	Robert Parenton
	Comment 50 • 21 years ago

Correct link for the link in Comment #47 :

http://www.louisianaada.org/MozillaFirebird-i686-pc-linux-gnu.tar.gz

	Robert Parenton
	Comment 51 • 21 years ago

This is the patch for the changes I made to make the security warning dialogs
work in the build I posted in Comment #47.

-> Mike, Bryner

Can you take a look at this patch?

	Robert Parenton
	Comment 52 • 21 years ago

Comment on attachment 129948 [details] [diff] [review]
Patch that forks nsSecurityWarningDialogs.cpp and security-prefs.js (includes new files so it may need modifications before being checked in)

Requesting patch review from Brian Ryner

	Michael Gall
	Comment 53 • 21 years ago

It probably belongs in a new bug, but I can't see a problem with forking those
preference files. There is a hell of a lot of unneeded preferences in FB from
Mozilla suite.

	Az
	Comment 54 • 21 years ago

michael: it would be helpful if ceated a new bug in which you listed all of
these unneeded preferences.

	Robert Parenton
	Comment 55 • 21 years ago

If a fork similar to this is the way the devs want to go, I can create a patch
with the desired directory structure.  If another method is preferred, I can
probably create a patch with whatever method is desired.  I just need some input
from the devs.

	Brian Ryner (not reading) Assignee
	Comment 56 • 21 years ago

Comment on attachment 129948 [details] [diff] [review]
Patch that forks nsSecurityWarningDialogs.cpp and security-prefs.js (includes new files so it may need modifications before being checked in)

I think I have something better/safer.

	Kai Engert (:KaiE:)
	Comment 58 • 21 years ago

Comment on attachment 137229 [details] [diff] [review]
proposed fix

Err. I think the user choice simply does not get stored because of the current
code.

Ignore the patch, look at current code: SetBoolPref is only being called if the
new preference is false. If the user toggles the checkbox to checked, the
preference value is true, and the conditional call to SetBoolPref is not
reached.

I think, if you changed the default, all you need is either invert the
condition check, or remove the condition altogether and always call
SetBoolPref.

	Kai Engert (:KaiE:)
	Comment 59 • 21 years ago

Since the default in Seamonkey is still "true" 
  (which in my personal opinion is the right thing 
   and I would like to remain that way),

and the default in Firebird is "false" 
  (which I don't like, but I can live with),

and the code is shared between both

=> I think the conclusion is to remove the condition altogether.

	Kai Engert (:KaiE:)
	Comment 61 • 21 years ago

By always storing the user decision, ignoring the default value, we also make
sure the user's past preference will remain active, whatever default other
versions of the software might think as being appropriate.

	Kai Engert (:KaiE:)
	Comment 62 • 21 years ago

Err, are we sure this bug is still valid?

I might be doing something wrong, but what I have just tried and seen is
shocking me. Let me say that I'm not a Firebird user, but still stick with
Seamonkey.

I removed my Thunderbird profile directory.
I started Thunderbird.
I went to a secure page (e.g. https://login.yahoo.com)
I did not get a security dialog (for entering a secure site)
I clicked an insecure link in the upper right corner
I again did not get a security warning (for leaving a secure site)

Since we don't even get a warning, I wonder why we bother about checkboxes.

I'm shocked.
I vote for changing the defaults in the preferences to true.

Please tell me I'm doing something wrong.

	Brian Ryner (not reading) Assignee
	Comment 63 • 21 years ago

> I removed my Thunderbird profile directory.
> I started Thunderbird.
> I went to a secure page (e.g. https://login.yahoo.com)

Do you mean Firebird here, not Thunderbird?

What you pointed out is exactly what I'm trying to fix.  The previous fix for
this bug actually disabled the dialogs altogether.  That's not the behavior we
_want_ to have for Firebird.  What we want is for the dialog to show up the
first time, but the default value of the checkbox is to not show the dialog
again.  If the user checks the checkbox, the dialog should show every time, and
the "show every time" checkbox should be initially checked when the dialog shows.

The patch you posted looks to me like it will always turn off the dialogs after
showing them once, even if the user wanted them to keep showing up.  That's not
what either SeaMonkey or Firebird wants.  You could instead change it to set
prefValue instead of PR_FALSE and it would remember the choice as it should, but
that doesn't let us get the behavior I described above for Firebird -- the key
difference is to have a way for the initial value of the checkbox to differ from
the current value of the pref.

	Brendan Eich [:brendan]
	Comment 64 • 21 years ago

Comment on attachment 137229 [details] [diff] [review]
proposed fix

>@@ -260,6 +286,8 @@ nsSecurityWarningDialogs::ConfirmDialog(
> 
>   if (!prefValue && prefName != nsnull) {
>     mPref->SetBoolPref(prefName, PR_FALSE);
>+  } else if (prefValue && showOnce) {
>+    mPref->SetBoolPref(showOncePref.get(), PR_TRUE);
>   }
> 
>   return rv;

Nit: how about this instead?

  if (!prefValue) {
    if (prefName != nsnull) {
      mPref->SetBoolPref(prefName, PR_FALSE);
    }
  } else {
    if (showOnce) {
      mPref->SetBoolPref(showOncePref.get(), PR_TRUE);
    }
  }

(Hope I'm following the over-braced-one-line-then/else style correctly.)

	Ben Goodger (use ben at mozilla dot org for email)
	Comment 65 • 21 years ago

sr=ben@mozilla.org

	Bill Gianopoulos [:WG9s]
	Comment 66 • 21 years ago

I don't really understand any of the proposed patch code.  It seems to add a
bunch of show this only once preferences, none of which are required.  What we
need is a seperate preference for if we should show each type of security
warning.  All of these need to default to true.  Then we need to have the pop-up
warning for each of these include a checkbox which should NOT be checked by
default.  The box should be labeled "Do not show this warning again".  If you
check this box before dismissing the window, then the appropriate show this
specific security warning prefence should be turned off.

Additionally, in the advanced preferences, there needs to be a button added in
the security section to turn all the awarnings back on.

	Brian Ryner (not reading) Assignee
	Comment 67 • 21 years ago

The show_once preferences are required because SeaMonkey and Firebird want to
have different behavior for whether the "show this warning again" checkbox is
checked the first time the dialog appears.

	Brian Ryner (not reading) Assignee
	Comment 68 • 21 years ago

Comment on attachment 137229 [details] [diff] [review]
proposed fix

marking ben's sr= on the patch.

	Brian Ryner (not reading) Assignee
	Comment 69 • 21 years ago

checked in.

	Simon Paquet [:sipaq]
	Comment 70 • 21 years ago

verified fixed 2004-01-10

	aynilove [So, Jae-yoon]
	Comment 71 • 21 years ago

2004-01-20 build in windows, it works exactly that reported to this article.
Should it reopened?

	Mike Connor [:mconnor]
	Comment 72 • 18 years ago

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change