Closed Bug 691690 Opened 8 years ago Closed 8 years ago

Can't copy the multibyte character correctly from the location bar

Categories

(Core :: Internationalization, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla10
Tracking Status
firefox8 + fixed
firefox9 --- fixed

People

(Reporter: emk, Assigned: emk)

References

()

Details

(6 keywords, Whiteboard: [qa!])

Attachments

(3 files, 3 obsolete files)

Steps to reproduce:
1. Open the URL.
2. Copy the URL from the location bar.

Actual result:
https://encrypted.google.com/search?q=%E3%81%82&ie=utf-8

Expected result:
https://encrypted.google.com/search?q=%82%A0&ie=utf-8

It works on Firefox 7 release, broken since Firefox 8.0b1.
Expected Result:
https://encrypted.google.com/search?q=%E3%81%82&ie=utf-8
Actual Result:
https://encrypted.google.com/search?q=%82%A0&ie=utf-8

Result when pasting URL to other application:
https://encrypted.google.com/search?q=%82%A0&ie=utf-8

This problem seems to be problem when copying URL to clipboard.

Is this bug related to bug 668019 ?
Requesting tracking for fx8.

A regression range would likely answer the question from comment 1....
Blocks: 668019
I propose add an option to CreateFixupURI so that it encodes URI with UTF-8 instead of platform charset (and backout bug 668019 from aurora and beta).
I can't reproduce. Maybe this only occurs when the default encoding is Shift_JIS?
(In reply to Simon Montagu from comment #5)
> I can't reproduce. Maybe this only occurs when the default encoding is
> Shift_JIS?
Yes. It occurs only when the platform charset is not UTF-8 and the character has a mapping on the platform charset. ISO-8859-1 doesn't have a map of "あ", so it will fallback to UTF-8.
It should be reproducible with <https://encrypted.google.com/search?q=÷&ie=utf-8> on Latin-1 system locale.
Steps to reproduce:
1. Open <https://encrypted.google.com/search?q=%C3%B7&ie=utf-8>.
2. Copy the URL from the location bar.

Actual result:
https://encrypted.google.com/search?q=%81%80&ie=utf-8 (on Shift_JIS locale)
https://encrypted.google.com/search?q=%F7&ie=utf-8 (on Latin-1 locale)

Expected result:
https://encrypted.google.com/search?q=%C3%B7&ie=utf-8
This was broken before bug 668019 in the sense that not copying but just hitting enter would mess up the encoding.
(In reply to Dão Gottwald [:dao] from comment #9)
> This was broken before bug 668019 in the sense that not copying but just
> hitting enter would mess up the encoding.
I know. Things getting even worse since Firefox 8 because Japanese users can no longer even copy the URL correctly.
See Also: → 461304
Assignee: smontagu → VYV03354
Status: NEW → ASSIGNED
Attachment #565431 - Flags: review?(bzbarsky)
We should not use platform charset here because the URI is always decoded in UTF-8 regardless of the platform charset on the location bar.
Attachment #565432 - Flags: review?(gavin.sharp)
Comment on attachment 565431 [details] [diff] [review]
Part 1: add UTF-8 flag to createFixupURI

I don't think you need to rev the IID for the flag addition; it's completely ABI and API compatible.

r=me without the iid rev.
Attachment #565431 - Flags: review?(bzbarsky) → review+
Comment on attachment 565432 [details] [diff] [review]
Part 2: Use UTF-8 flag on copy

This patch causes misterious test timeout...

> TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_urlbarCopying.js | Expecting copy of: http://example.com/?%C3%B7%C3%B7
> TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_urlbarCopying.js | Console message: Direct3D 9 DeviceManager Initialized Succesfully.
> Driver: igdumdx32.dll
> Description: Mobile Intel(R) 965 Express Chipset Family
> Version: 8.14.10.1930
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_urlbarCopying.js | Timed out while polling clipboard for pasted data.
> Stack trace:
>     JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: wait :: line 634
>     JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: <TOP_LEVEL> :: line 659
>     native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

But it seems to be unrelated to this fix. The timeout is caused the test change alone.
Attachment #565432 - Flags: review?(gavin.sharp)
Attached patch Backout bug 668019 (obsolete) — Splinter Review
Requesting backout on branches to workaround a critical regression.
Attachment #565549 - Flags: approval-mozilla-beta?
Attachment #565549 - Flags: approval-mozilla-aurora?
Comment on attachment 565549 [details] [diff] [review]
Backout bug 668019

We're not going to back out that patch - the bug it fixes affects more users than this bug does.
Attachment #565549 - Flags: review-
Attachment #565549 - Flags: approval-mozilla-beta?
Attachment #565549 - Flags: approval-mozilla-aurora?
(In reply to Masatoshi Kimura [:emk] from comment #14)
> But it seems to be unrelated to this fix. The timeout is caused the test
> change alone.

That's how failures in this test manifest themselves (the clipboard polling fails to find the right value on the clipboard, so it times out).

https://people.mozilla.com/~gavin/clipboardDebug.diff may be useful in debugging the issue.
I think we can take the actual fix on beta/aurora, to avoid shipping the regression.
Ah, I overlooked "http://" case because the comment #0 URI was "https://".
Also canceled IID bumping. it's still ABI and API compatible.
Requesting review again because it contains an actual code change from the previous patch.
Attachment #565431 - Attachment is obsolete: true
Attachment #565708 - Flags: review?(bzbarsky)
Comment on attachment 565432 [details] [diff] [review]
Part 2: Use UTF-8 flag on copy

This part didn't contain the bug.
Attachment #565432 - Flags: review?(gavin.sharp)
Attachment #565549 - Attachment is obsolete: true
Attachment #565708 - Flags: review?(bzbarsky) → review+
Gavin, ping?
Attachment #565432 - Flags: review?(gavin.sharp) → review+
Un-bug690892-bitrotting.
Carrying forward r+.
Attachment #565708 - Attachment is obsolete: true
Attachment #567959 - Flags: review+
Keywords: checkin-needed
Flags: in-testsuite+
Comment on attachment 567959 [details] [diff] [review]
565708: Part 1: Add UTF-8 flag to createFixupURI, v2

requesting for aurora and beta since this is regression.
Attachment #567959 - Flags: approval-mozilla-beta?
Attachment #567959 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c9ec8dc0fd61
https://hg.mozilla.org/mozilla-central/rev/6d6fdd47ee1b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
The trunk patch don't apply to branches cleanly because of PRBool->bool switch.
Part 2 is not affected.
Attachment #568008 - Flags: approval-mozilla-beta?
Attachment #568008 - Flags: approval-mozilla-aurora?
Attachment #567959 - Flags: approval-mozilla-beta?
Attachment #567959 - Flags: approval-mozilla-aurora?
Comment on attachment 568008 [details] [diff] [review]
Part 1 for aurora and beta

approving for aurora but to take this into beta, we'd like to get a risk analysis. What could go wrong. What should we be looking for in feedback if something does go wrong with this fix?
Attachment #568008 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This patch changes a copy operation on the location bar. If something goes wrong, people will complain about wrong copied strings just like this bug. But I believe it's unlikely to occur.
Luckily, this patch avoids an IID bump. So it will not affect extension compatibility.
---------------------------------[ Triage Comment ]---------------------------------

Approving for 8beta as this was a regression in Firefox 7, the fix looks localized, and there is no add-on compatibility concern.

Please land ASAP.
Attachment #568008 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
---------------------------------[ Triage Comment ]---------------------------------

We'll track this for Firefox 8 as it is a regression and it would be good to track if the fix makes it in or not.
Keywords: checkin-needed
Whiteboard: [inbound] → land attachment #568008 and #565432 to aurora and beta
qa+ for verification in Firefox 8 and 9.
Whiteboard: [qa+]
FIXUP_FLAG_USE_UTF8 needs to have a document.
Keywords: dev-doc-needed
What's the expected result? Comment 0 and comment 1 seems to be contradictory.
I've verified this on:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0.1) Gecko/20100101 Firefox/8.0.1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111121 Firefox/10.0a2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111121 Firefox/11.0a1

and when pasting the URL to another application, the actual result is:
https://encrypted.google.com/search?q=%E3%81%82&ie=utf-8
Sorry, comment #1 is correct.
Based on comment 35 and comment 36 marking the bug as verified fixed.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Documented (along with the rest of nsIURIFixup):

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIURIFixup

And mentioned on Firefox 10 for developers.
You need to log in before you can comment on or make changes to this bug.