Can't use SSL after import from Internet Explorer.

RESOLVED FIXED in Firefox 2 beta2

Status

()

Firefox
Migration
P3
trivial
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Gennady Feldman, Assigned: mwu)

Tracking

unspecified
Firefox 2 beta2
x86
Windows 2000
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.0.1 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
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.

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
I'm not sure I understand fully. Is the failure in importing the settings, or is that importing the settings breaks HTTPS in general?
(Reporter)

Comment 3

12 years ago
Importing those settings breaks HTTPS. Unless you manually enable SSL2.0 again in Options. Turning off SSL2.0 seems to break it again.
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.
(Reporter)

Comment 5

12 years ago
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

12 years ago
Nominating for next release(es) since this might be a regression from mconner's
latest migration code changes. Assigning to him for more investigation.
Assignee: nobody → mconners
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
Keywords: regression
Assignee: mconners → mconnor
Flags: blocking1.8.0.1? → blocking1.8.0.1-
+ for testing and investigation.
Status: NEW → ASSIGNED
Flags: blocking-firefox2+
Target Milestone: --- → Firefox 2 beta1

Updated

12 years ago
Priority: -- → P3

Comment 8

12 years ago
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.
(Reporter)

Comment 9

12 years ago
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)

Updated

12 years ago
Whiteboard: 181b1+

Updated

12 years ago
Assignee: mconnor → michael.wu
Status: ASSIGNED → NEW

Updated

12 years ago
Keywords: relnote
Target Milestone: Firefox 2 beta1 → Firefox 2 beta2

Updated

12 years ago
Whiteboard: 181b1+
(Assignee)

Updated

12 years ago
Depends on: 343800
(Assignee)

Comment 10

12 years ago
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.)
(Assignee)

Comment 11

12 years ago
Fixed by patch in bug 343800. Please verify.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

12 years ago
(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.
> 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.
(Assignee)

Comment 15

12 years ago
(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.
> 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.  :)
(Assignee)

Updated

12 years ago
Depends on: 344863
(Assignee)

Comment 17

12 years ago
Created attachment 229569 [details] [diff] [review]
Better way of checking bits
mwu, is this patch ready for reviews?
Whiteboard: [mustfix]
(Assignee)

Comment 19

12 years ago
(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.
(Assignee)

Updated

12 years ago
Attachment #229569 - Flags: superreview?(mconnor)
Attachment #229569 - Flags: review?(mconnor)
(Assignee)

Updated

12 years ago
Severity: normal → trivial
Keywords: regression, relnote
Whiteboard: [mustfix]

Updated

12 years ago
Attachment #229569 - Flags: superreview?(mconnor)
Attachment #229569 - Flags: review?(mconnor)
Attachment #229569 - Flags: review+

Updated

12 years ago
Whiteboard: [checkin needed]
mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp 	1.39
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Michael, is this ready to go on branch yet?
Whiteboard: [has patch][baking on trunk]

Updated

12 years ago
Flags: blocking-firefox2+
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][baking on trunk]
You need to log in before you can comment on or make changes to this bug.