261934 - (enableIDNpref) regression: network.standard-url.encode.utf8 and network.enableIDN prefs are ignored

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[unprotected]

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

Comment 1

[Simon Montagu :smontagu]

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

Comment 2

[Simon Montagu :smontagu]

The same problem exists in trunk, but it's currently masked by bug 271196

Comment 3

[Darin Fisher]

Can this problem be reproduced using a recent Mozilla trunk build?

Comment 4

[Simon Montagu :smontagu]

Yes, see comment 2 :-P

I was running into this constantly when testing bug 271196

Comment 5

[Darin Fisher]

Simon, ok.. thx.  i misunderstood your comment.  anyways, do you want to take
this bug?

Comment 6

[Simon Montagu :smontagu]

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.

Comment 7

[Simon Montagu :smontagu]

Attachment: Intialize all prefs in InitGlobalObjects()

Comment 8

[Darin Fisher]

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

Comment 9

[Simon Montagu :smontagu]

Attachment: Using helper function

Comment 10

[Simon Montagu :smontagu]

Attachment: Using helper function

Oops, I attached the original diff instead of the new one.

Comment 11

[Darin Fisher]

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.

Comment 12

[Simon Montagu :smontagu]

(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.

Comment 13

[Simon Montagu :smontagu]

Checked in with the other nits picked.

Comment 14

[[e-mail]]

(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"));
+        }
+    }

Comment 15

[Daniel Veditz [:dveditz]]

Nominating for branch in the hopes that this fixes bug 281365 (that has to be
verified, of course, before it's approved).

Comment 16

[Darin Fisher]

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

This patch works properly on the 1.7 branch.

Comment 17

[Darin Fisher]

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

Comment 18

[Daniel Veditz [:dveditz]]

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

a=dveditz for 1.7 and aviary1.0.1 (*not* aviary1.0) branches

Comment 19

[Darin Fisher]

fixed1.7.6 & fixed-aviary1.0.1

Comment 20

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

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

Comment 21

[Daniel Veditz [:dveditz]]

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

Comment 22

[Phil Ringnalda (:philor)]

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

Comment 23

[Alex]

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