317033 - Can't use SSL after import from Internet Explorer.

Status: RESOLVED FIXED

(Firefox :: Migration, defect, P3)

Description

[Gennady Feldman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051118 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051118 Firefox/1.5

This only happens if SSL2.0 is disabled in Internet Explorer. (SSL3.0 and TLS1.0 are enabled). After importing IE Inernet Options I can't access any of the HTTPS sites unless I go and check the SSL2.0 option in Advanced section of settings.

Reproducible: Always

Steps to Reproduce:
1. Disable SSL2.0 in Internet Explorer.
2. Start with a clean slate (no profiles for Firefox)
3. Don't do the import when launching first time (branch clean profile).
4. Now do File->Import. Select Internet Explorer. Now "Internet Options" should be selected (actually I had all of it selected).
5. Now visit https://bugzilla.mozilla.org

Actual Results:  
I get an alert saying that I can't access the site cause SSL is disabled.

Expected Results:  
You should be able to visit that site. And the site should load.

Comment 1

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

I can't imagine they've changed the way they store that setting in the registry, so I suppose this never worked?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp&rev=1.33#1838

Comment 2

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

I'm not sure I understand fully. Is the failure in importing the settings, or is that importing the settings breaks HTTPS in general?

Comment 3

[Gennady Feldman]

Importing those settings breaks HTTPS. Unless you manually enable SSL2.0 again in Options. Turning off SSL2.0 seems to break it again.

Comment 4

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

Do you have any of the other Secure protocols enabled? If none are enabled (or if none are enabled that the site supports), then it's expected that browsing sites requiring SSL will fail. What is the state of the three checkboxes before and after importing? Which sites are you using to test this? This site, for example, works with SSLv3 or TLS1.0, but not with SSLv2.

Comment 5

[Gennady Feldman]

SSL 2.0 -> unchecked
SSL 3.0 -> checked
TLS 1.0 -> checked

It definitely something that import routine sets that throws things off. And it just breaks for any of the HTTPS sites. I tried https://bugzilla.mozilla.org and got that error message. 

The weird thing this only breaks after that import from IE. Manually changing those settings (without doing import at all) works fine.

Comment 6

[Jay Patel [:jay]]

Nominating for next release(es) since this might be a regression from mconner's
latest migration code changes. Assigning to him for more investigation.

Comment 7

[Mike Connor [:mconnor]]

+ for testing and investigation.

Comment 8

[Tracy Walker [:tracy]]

This is still broken in Bon Echo nightly from 2006-06-06. If SSL 2.0 is disabled in IE then we import IE's internet options same results as originally reported.

Comment 9

[Gennady Feldman]

Ok. Just tested this again with: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1a3) Gecko/20060606 BonEcho/2.0a3

You need to follow these steps exactly (other cases seem not to break this):

1. You need to delete the Firefox profiles directory or rename them. This way you are starting from scratch. (No profiles).

2. On migration you need to select the option that will not import anything. (Do nothing)

3. Select File->Import and select Internet Explorer. And let it import ALL of the settings. (haven't actually tried changing those checkboxes)

4. When it tries to connect to mozilla it says it can't connect.

P.S. You need to have SSL 2.0 disabled in IE before import. (and yes I know we no longer have SSL 2.0 listed in Firefox options)

Comment 10

[Michael Wu [:mwu]]

Bug 343800 is the root cause of this bug.

See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp&rev=1.33#1852

SetBoolPref does not convert nonzero values to PR_TRUE when storing the new pref value. Thus, later on, code checking for that pref equaling PR_TRUE does not work. (however, it all gets fixed up once the pref is saved to the disk and loaded back on restart.)

Comment 11

[Michael Wu [:mwu]]

Fixed by patch in bug 343800. Please verify.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Actually, passing values other than PR_TRUE or PR_FALSE as a PRBool is a bug in the calling code.  So the code at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp&rev=1.33#1852 should also be fixed, even if prefs code has been fixed to work around this bug.

Comment 13

[Michael Wu [:mwu]]

(In reply to comment #12)
> Actually, passing values other than PR_TRUE or PR_FALSE as a PRBool is a bug in
> the calling code.
True, but "fixing" the caller would increase the size of the code for no real benefit. I'm also not comfortable with changing the definition of true and false from nonzero and zero to 1 and 0. If we really want to go this route, we should assert whenever SetBoolPref receives values that are not PR_TRUE or PR_FALSE. However, I would prefer to allow backwards compatibility with the C definition of true and false because there is no harm in doing so.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> for no real benefit

Except for actually following the rules for working with PRBool.

> I'm also not comfortable with changing the definition

The definition of PRBool in Mozilla code is that passing any values other than 0 and 1 through a PRBool is an error.  Callers that do that can expect to fail in weird ways.

> we should assert whenever SetBoolPref receives values that are not PR_TRUE or
> PR_FALSE.

Yes.  We should add such an assert.

> because there is no harm in doing so.

Other than reinforcing incorrect coding patterns that _will_ get you into trouble in general.

Comment 15

[Michael Wu [:mwu]]

(In reply to comment #14)
> > for no real benefit
> 
> Except for actually following the rules for working with PRBool.
> 
True. It is too bad that the compiler cannot enforce this.

> > I'm also not comfortable with changing the definition
> 
> The definition of PRBool in Mozilla code is that passing any values other than
> 0 and 1 through a PRBool is an error.  Callers that do that can expect to fail
> in weird ways.
> 
I think using PR_TRUE and PR_FALSE for comparison is a mistake, and they should only be used to make setting/passing true or false very clear. I would like to think that PRBool has the same purpose - to remind people that the variable is just a special integer that should only be checked for whether it is zero or nonzero. However, that appears not to be the case, and we want |if (aOption == PR_TRUE)| to work. I think forcing people to use |if (aOption)| instead would be a real benefit. Oh well. 

> > we should assert whenever SetBoolPref receives values that are not PR_TRUE or
> > PR_FALSE.
> 
> Yes.  We should add such an assert.
> 
I'll open a bug for this.

> > because there is no harm in doing so.
> 
> Other than reinforcing incorrect coding patterns that _will_ get you into
> trouble in general.
> 
Not outside of mozilla, where true is not defined to be just 1.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> True. It is too bad that the compiler cannot enforce this.

Yes.  :(

> I think using PR_TRUE and PR_FALSE for comparison is a mistake,

I agree on this.  But using values other than PR_TRUE and PR_FALSE breaks other uses of PRBool.  Most importantly, it breaks assigning a PRBool to a PRPackedBool (and any similar operation such as assigning to a bit in a bitfield, etc).  This is where things have _really_ bitten us in the past with callers who assign random values to PRBools.

> Not outside of mozilla

Not really relevant to what gets checked into Mozilla code.  :)

Comment 17

[Michael Wu [:mwu]]

Attachment: Better way of checking bits

Comment 18

[Mike Connor [:mconnor]]

mwu, is this patch ready for reviews?

Comment 19

[Michael Wu [:mwu]]

(In reply to comment #18)
> mwu, is this patch ready for reviews?
> 
This bug was effectively fixed by bug 343800. This bug was reopened by bz to fix the caller, but that fix will have no effect on the behavior of the code since bug 343800 already works around it. This bug doesn't need to block anything since its symptoms are already fixed.

Comment 20

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

mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp 	1.39

Comment 21

[Mike Connor [:mconnor]]

Michael, is this ready to go on branch yet?