890367 - browser.newtab.url does not accept non-ASCII values (non-Latin / non-English / Russian / Cyrillic / would-be-IDN), they are mangled when used

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[[:Aleksej]]

2013-07-04-03-13-23-mozilla-central-firefox-25.0a1.en-US.linux-x86_64

1. In about:config, set the browser.newtab.url pref to something non-ASCII, like "http://пример.испытание" (one of the test IDNs, see https://en.wikipedia.org/wiki/List_of_Internet_top-level_domains#Test_TLDs ).
2. Open a new tab.

Actual results:
The URL is mangled: http://www.пÑимеÑ.иÑпÑÑание/

Expected results:
The URL is loaded as if you've entered it into the address bar.

This works properly with browser.startup.homepage.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problem is presumably somewhere reachable from the utilityOverlay.js caller here:
http://mxr.mozilla.org/mozilla-central/search?string=browser.newtab.url
but the nsDocShell.cpp caller might also need to be kept in sync.

Comment 2

[mjh563]

Attachment: patch

The browser.newtab.url preference is read using nsIPrefBranch::getCharPref, which isn't correct for prefs that might contain non-ASCII characters. 

Should use nsIPrefBranch::getComplexValue instead.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 778934 [details] [diff] [review]
patch

Review of attachment 778934 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/utilityOverlay.js
@@ +18,5 @@
>            !PrivateBrowsingUtils.permanentPrivateBrowsing)
>          return "about:privatebrowsing";
>      }
> +    return Services.prefs.getComplexValue
> +           (PREF, Components.interfaces.nsISupportsString).data || "about:blank";

Thank you for taking care of this! Two things:

Please use Ci.nsISupportsString, that's just a little shorted than using "Components.interfaces".
Please save the value to a variable so that we can get rid of the line wrapping and it should be easier to read.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Like dbaron mentioned, we'll need to update nsDocShell as well:

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10586

and replace GetDefaultCString() with GetComplex().

You can do this in a second patch (a la part 2) if you like but we'll need to land them together.

Comment 5

[mjh563]

Thanks for the quick response.

I did consider that line in nsDocShell, but I don't really know why it would need changing. If I'm not mistaken, it currently returns the pref as a UTF-8 string, which will be correct even when the value contains non-ASCII characters.

(Anyway, it gets the default value of the pref and not the user-set value.)

I'll make the other changes you requested, but please could you explain why GetComplex() should be used instead of GetDefaultCString()?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Sorry, I of course meant to write GetDefaultComplex(). I don't know much about our internal strings in native code... let's ask someone that does.

Benjamin, do we need to switch to GetDefaultComplex() in nsDocShell here to compare a loaded URL with a pref's default value?

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10586

Comment 7

[Benjamin Smedberg]

No, native code can use getCharPref. The problem is in the xpconnect reflection of nsIPrefBranch.getCharPref. It's declared to return a "string", which is ASCII-only.

That would probably be very hard to change to use AUTF8String instead, although I'd love for somebody to try it and see how much work it is.

GetComplexValue is a pretty horrible API, if all we need is to get a unicode value. If we can't just fix .getCharPref, we should probably just make .getStringPref which returns AUTF8String.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Thanks for shedding some light on this, Benjamin.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> GetComplexValue is a pretty horrible API, if all we need is to get a unicode
> value. If we can't just fix .getCharPref, we should probably just make
> .getStringPref which returns AUTF8String.

We should file a follow-up bug to investigate fixing .getCharPref or about introducing .getStringPref.

mjh563, I think your patch is good to go then with the proposed changes from comment #3 applied.

Comment 9

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

(In reply to Tim Taubert [:ttaubert] from comment #8)
> We should file a follow-up bug to investigate fixing .getCharPref or about
> introducing .getStringPref.

Yes please! Please CC me, and it's probably worth posting to newsgroups about this too.

Comment 10

[mjh563]

Attachment: updated patch

Updated patch with the changes from comment 3.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 779492 [details] [diff] [review]
updated patch

Review of attachment 779492 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9ee08d135dd

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b9ee08d135dd

Comment 14

[mjh563]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> We should file a follow-up bug to investigate fixing .getCharPref or about
> introducing .getStringPref.

I've filed bug 898181 about changing getCharPref.

There's also bug 353812 about adding a new function to get non-ASCII string prefs.

Comment 15

[[:Aleksej]]

Verified fixed with 2013-10-22-03-02-02-mozilla-central-firefox-27.0a1.en-US.linux-x86_64 (compared to 2013-07-21-03-02-03-mozilla-central-firefox-25.0a1.ru.linux-x86_64)