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

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Masatoshi Kimura [:emk]]

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.

Comment 1

[Masahiro YAMADA]

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 ?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Requesting tracking for fx8.

A regression range would likely answer the question from comment 1....

Comment 3

[Masatoshi Kimura [:emk]]

Regression window (mozilla-aurora)
Works: http://hg.mozilla.org/releases/mozilla-aurora/rev/009c64b64cf3
Broken: http://hg.mozilla.org/releases/mozilla-aurora/rev/c440edd84f84
Pushlog: https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=009c64b64cf3&tochange=c440edd84f84
It's most likely to be bug 668019.

Comment 4

[Masatoshi Kimura [:emk]]

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

Comment 5

[Simon Montagu :smontagu]

I can't reproduce. Maybe this only occurs when the default encoding is Shift_JIS?

Comment 6

[Masatoshi Kimura [:emk]]

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

Comment 7

[Masatoshi Kimura [:emk]]

It should be reproducible with <https://encrypted.google.com/search?q=÷&ie=utf-8> on Latin-1 system locale.

Comment 8

[Masatoshi Kimura [:emk]]

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

Comment 9

[Dão Gottwald [::dao]]

This was broken before bug 668019 in the sense that not copying but just hitting enter would mess up the encoding.

Comment 10

[Masatoshi Kimura [:emk]]

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

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Part 1: add UTF-8 flag to createFixupURI

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Use UTF-8 flag on copy

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Masatoshi Kimura [:emk]]

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.

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: Backout bug 668019

Requesting backout on branches to workaround a critical regression.

Comment 16

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

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.

Comment 17

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

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

Comment 18

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

I think we can take the actual fix on beta/aurora, to avoid shipping the regression.

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Add UTF-8 flag to createFixupURI, v2

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.

Comment 20

[Masatoshi Kimura [:emk]]

Comment on attachment 565432 [details] [diff] [review]
Part 2: Use UTF-8 flag on copy

This part didn't contain the bug.

Comment 21

[Masatoshi Kimura [:emk]]

Gavin, ping?

Comment 22

[Masatoshi Kimura [:emk]]

FYI, comment #14 problem has been resolved by Part 1 v2.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=611d186aafad

Comment 23

[Masatoshi Kimura [:emk]]

Attachment: 565708: Part 1: Add UTF-8 flag to createFixupURI, v2

Un-bug690892-bitrotting.
Carrying forward r+.

Comment 24

[Makoto Kato [:m_kato]]

landed to m-i
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ec8dc0fd61 (part 1)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d6fdd47ee1b (part 2)

Comment 25

[Makoto Kato [:m_kato]]

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.

Comment 26

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/c9ec8dc0fd61
https://hg.mozilla.org/mozilla-central/rev/6d6fdd47ee1b

Comment 27

[Masatoshi Kimura [:emk]]

Attachment: Part 1 for aurora and beta

The trunk patch don't apply to branches cleanly because of PRBool->bool switch.
Part 2 is not affected.

Comment 28

[Asa Dotzler [:asa]]

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?

Comment 29

[Masatoshi Kimura [:emk]]

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.

Comment 30

[LegNeato]

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

Comment 31

[LegNeato]

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

Comment 32

[Dão Gottwald [::dao]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/dae0bfc9a760
http://hg.mozilla.org/releases/mozilla-aurora/rev/99db0336dcd1
http://hg.mozilla.org/releases/mozilla-beta/rev/ff142e5fea36
http://hg.mozilla.org/releases/mozilla-beta/rev/9e0f94a06cac

Comment 33

[u279076]

qa+ for verification in Firefox 8 and 9.

Comment 34

[Masatoshi Kimura [:emk]]

FIXUP_FLAG_USE_UTF8 needs to have a document.

Comment 35

[Paul Silaghi, QA [:pauly]]

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

Comment 36

[Masatoshi Kimura [:emk]]

Sorry, comment #1 is correct.

Comment 37

[Paul Silaghi, QA [:pauly]]

Based on comment 35 and comment 36 marking the bug as verified fixed.

Comment 38

[Eric Shepherd [:sheppy]]

Documented (along with the rest of nsIURIFixup):

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

And mentioned on Firefox 10 for developers.