Bug 261934 (enableIDNpref)

regression: network.standard-url.encode.utf8 and network.enableIDN prefs are ignored

RESOLVED FIXED

Status

()

RESOLVED FIXED
15 years ago
8 years ago

People

(Reporter: unprotected, Assigned: smontagu)

Tracking

(4 keywords)

Trunk
x86
Windows 2000
fixed-aviary1.0.1, fixed1.7.6, helpwanted, regression
Points:
---
Bug Flags:
blocking1.7.6 +
blocking-aviary1.0.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3) Gecko/20040913 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3) Gecko/20040913 Firefox/0.10

Upon start Firefox 1.0 does not encode URLs to the UTF-8 despite
network.standard-url.encode.utf8 preference setting set to TRUE.
Switch network.standard-url.encode.utf8 setting to FALSE then to TRUE solves the
problem.
Previous version (0.9.3) doesn't demonstrate such a problem.


Reproducible: Always
Steps to Reproduce:
1. Start Firefox
2. Try to reach URL with non-ASCII characters in it - URL does not encoded in UTF-8
3. Switch network.standard-url.encode.utf8 setting to FALSE then to TRUE
4. Try to reach same URL - This time URL encoded to UTF-8
(Assignee)

Comment 1

15 years ago
*** Bug 271122 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 2

15 years ago
The same problem exists in trunk, but it's currently masked by bug 271196
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

15 years ago
Component: Preferences → Networking
Product: Firefox → Core
Version: unspecified → Trunk
(Assignee)

Updated

15 years ago
Assignee: firefox → darin
QA Contact: mconnor → benc
Keywords: regression
Summary: Firefox does not use network.standard-url.encode.utf8 preference → regression: network.standard-url.encode.utf8 pref is ignored

Comment 3

15 years ago
Can this problem be reproduced using a recent Mozilla trunk build?
(Assignee)

Comment 4

15 years ago
Yes, see comment 2 :-P

I was running into this constantly when testing bug 271196

Comment 5

15 years ago
Simon, ok.. thx.  i misunderstood your comment.  anyways, do you want to take
this bug?
Keywords: helpwanted
(Assignee)

Comment 6

15 years ago
It was puzzling me how this ever worked, since the flag gAlwaysEncodeInUTF8 is
not and never has been initialized from the pref. I now suspect that when it
worked the user happened to open the profile manager when starting Firefox
(causing the flag to be set correctly by the pref observer), and when it didn't
work the user happened not to open the profile manager.
Assignee: darin → smontagu
(Assignee)

Updated

15 years ago
Attachment #167422 - Flags: superreview?(darin)
Attachment #167422 - Flags: review?(darin)

Comment 8

15 years ago
Comment on attachment 167422 [details] [diff] [review]
Intialize all prefs in InitGlobalObjects()

It would seem that there is some overlap here with the code in
nsStandardURL::nsPrefObserver::Observe.

It might be better to factor the code that reads the prefs into a helper
function.  See for example nsHttpHandler::PrefsChanged
(Assignee)

Comment 9

15 years ago
Posted patch Using helper function (obsolete) — Splinter Review
Attachment #167422 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #167630 - Flags: superreview?(darin)
Attachment #167630 - Flags: review?(darin)
(Assignee)

Updated

15 years ago
Attachment #167422 - Flags: superreview?(darin)
Attachment #167422 - Flags: review?(darin)
(Assignee)

Comment 10

15 years ago
Oops, I attached the original diff instead of the new one.
Attachment #167630 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #167631 - Flags: superreview?(darin)
Attachment #167631 - Flags: review?(darin)
(Assignee)

Updated

15 years ago
Attachment #167630 - Flags: superreview?(darin)
Attachment #167630 - Flags: review?(darin)

Comment 11

15 years ago
Comment on attachment 167631 [details] [diff] [review]
Using helper function

>Index: ../../../../netwerk/base/src/nsStandardURL.cpp

>+            PrefsChanged(prefBranch, NS_ConvertUCS2toUTF8(data).get());

nit: use NS_ConvertUTF16... instead of the UCS2 version.


>+void

nit: write,

/* static */ void

that way it is clear that this is a static member function.


>+#define PREF_CHANGED(p) ((pref == nsnull) || !PL_strcmp(pref, p))

nit: since pref is null checked, you can just use strcmp instead of PL_strcmp.


>+                NS_ADDREF(gIDNService = serv.get());

nit: you might be able to use the new nsCOMPtr::swap method here to
avoid an AddRef/Release pair.


Thanks for making this patch!  r+sr=darin with those nits picked.
Attachment #167631 - Flags: superreview?(darin)
Attachment #167631 - Flags: superreview+
Attachment #167631 - Flags: review?(darin)
Attachment #167631 - Flags: review+
(Assignee)

Comment 12

15 years ago
(In reply to comment #11)
> >+                NS_ADDREF(gIDNService = serv.get());
> 
> nit: you might be able to use the new nsCOMPtr::swap method here to
> avoid an AddRef/Release pair.

I don't think so: any time we get here we will either be unsetting the pref and
Releasing the old gIDNService, or setting it and AddRefing a new one; never both
at the same time.
(Assignee)

Comment 13

15 years ago
Checked in with the other nits picked.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(In reply to comment #12)
>(In reply to comment #11)
>>>+                NS_ADDREF(gIDNService = serv.get());
>> 
>> nit: you might be able to use the new nsCOMPtr::swap method here to
>> avoid an AddRef/Release pair.
> 
>I don't think so: any time we get here we will either be unsetting the pref
>and Releasing the old gIDNService, or setting it and AddRefing a new one;
>never both at the same time.
Actually it was probably the addref/release on the nsCOMPtr that darin wanted to
avoid by doing something like this:
+    if (PREF_CHANGED(NS_NET_PREF_ENABLEIDN)) {
+        if (GOT_PREF(NS_NET_PREF_ENABLEIDN, val)) {
+            nsCOMPtr<nsIIDNService> serv;
+            if (val) // initialize IDN
+                serv = do_GetService(NS_IDNSERVICE_CONTRACTID);
+            serv.swap(gIDNService);
+            LOG(("IDN support %s\n", gIDNService ? "enabled" : "disabled"));
+        }
+    }
Nominating for branch in the hopes that this fixes bug 281365 (that has to be
verified, of course, before it's approved).
Flags: blocking1.7.6?
Flags: blocking-aviary1.0.1?

Comment 16

14 years ago
Comment on attachment 167631 [details] [diff] [review]
Using helper function

This patch works properly on the 1.7 branch.
Attachment #167631 - Flags: approval1.7.6?
Attachment #167631 - Flags: approval-aviary1.0.1?

Comment 17

14 years ago
*** Bug 281365 has been marked as a duplicate of this bug. ***
Comment on attachment 167631 [details] [diff] [review]
Using helper function

a=dveditz for 1.7 and aviary1.0.1 (*not* aviary1.0) branches
Attachment #167631 - Flags: approval1.7.6?
Attachment #167631 - Flags: approval1.7.6+
Attachment #167631 - Flags: approval-aviary1.0.1?
Attachment #167631 - Flags: approval-aviary1.0.1+

Updated

14 years ago
Keywords: fixed1.7.6

Comment 19

14 years ago
fixed1.7.6 & fixed-aviary1.0.1
Keywords: fixed-aviary1.0.1
Flags: blocking1.7.6?
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.1?
Flags: blocking-aviary1.0.1+
Blocks: 281365
Summary: regression: network.standard-url.encode.utf8 pref is ignored → regression: network.standard-url.encode.utf8 and network.enableIDN prefs are ignored
*** Bug 281730 has been marked as a duplicate of this bug. ***
Alias: enableIDNpref
*** Bug 281765 has been marked as a duplicate of this bug. ***
*** Bug 282756 has been marked as a duplicate of this bug. ***

Comment 23

8 years ago
Seems to network.standard-url.encode.utf8 broken again, i don't know when, but russian symbols when copy entire url convert to symbol code, for example %D0%91%D1%80%D0%B0%D1%83%D0%B7%D0%B5%D1%80
You need to log in before you can comment on or make changes to this bug.