Closed Bug 890367 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 25

People

(Reporter: Aleksej, Assigned: mjh563)

References

Details

(Whiteboard: [bugday-20131023])

Attachments

(1 file, 1 obsolete file)

1.02 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
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.
Summary: browser.newtab.url does not accept non-ASCII values (non-Latin / non-English / Russian / Cyrillic / would-be-IDN) → browser.newtab.url does not accept non-ASCII values (non-Latin / non-English / Russian / Cyrillic / would-be-IDN), they are mangled when used
OS: Linux → All
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.
Component: Internationalization → Tabbed Browser
Product: Core → Firefox
Hardware: x86_64 → All
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → mjh563
Status: NEW → ASSIGNED
Attachment #778934 - Flags: review?(ttaubert)
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.
Attachment #778934 - Flags: review?(ttaubert) → feedback+
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.
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()?
Flags: needinfo?(ttaubert)
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
Flags: needinfo?(ttaubert) → needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
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.
(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.
Attached patch updated patchSplinter Review
Updated patch with the changes from comment 3.
Attachment #778934 - Attachment is obsolete: true
Attachment #779492 - Flags: review?(ttaubert)
Comment on attachment 779492 [details] [diff] [review]
updated patch

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

Thank you!
Attachment #779492 - Flags: review?(ttaubert) → review+
Keywords: checkin-needed
See Also: → 840976
https://hg.mozilla.org/mozilla-central/rev/b9ee08d135dd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
(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.
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)
Status: RESOLVED → VERIFIED
Whiteboard: [bugday-20131023]
You need to log in before you can comment on or make changes to this bug.