Closed Bug 261934 (enableIDNpref) Opened 17 years ago Closed 17 years ago

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

Categories

(Core :: Networking, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: unprotected, Assigned: smontagu)

References

Details

(4 keywords)

Attachments

(1 file, 2 obsolete files)

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
*** Bug 271122 has been marked as a duplicate of this bug. ***
The same problem exists in trunk, but it's currently masked by bug 271196
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Preferences → Networking
Product: Firefox → Core
Version: unspecified → Trunk
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
Can this problem be reproduced using a recent Mozilla trunk build?
Yes, see comment 2 :-P

I was running into this constantly when testing bug 271196
Simon, ok.. thx.  i misunderstood your comment.  anyways, do you want to take
this bug?
Keywords: helpwanted
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
Attachment #167422 - Flags: superreview?(darin)
Attachment #167422 - Flags: review?(darin)
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
Attached patch Using helper function (obsolete) — Splinter Review
Attachment #167422 - Attachment is obsolete: true
Attachment #167630 - Flags: superreview?(darin)
Attachment #167630 - Flags: review?(darin)
Attachment #167422 - Flags: superreview?(darin)
Attachment #167422 - Flags: review?(darin)
Oops, I attached the original diff instead of the new one.
Attachment #167630 - Attachment is obsolete: true
Attachment #167631 - Flags: superreview?(darin)
Attachment #167631 - Flags: review?(darin)
Attachment #167630 - Flags: superreview?(darin)
Attachment #167630 - Flags: review?(darin)
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+
(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.
Checked in with the other nits picked.
Status: NEW → RESOLVED
Closed: 17 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 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?
*** 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+
Keywords: fixed1.7.6
fixed1.7.6 & 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. ***
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.