Last Comment Bug 691690 - Can't copy the multibyte character correctly from the location bar
: Can't copy the multibyte character correctly from the location bar
Status: VERIFIED FIXED
[qa!]
: dev-doc-complete, intl, jp-critical, regression, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla10
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
https://encrypted.google.com/search?q...
Depends on:
Blocks: 668019
  Show dependency treegraph
 
Reported: 2011-10-04 01:21 PDT by Masatoshi Kimura [:emk]
Modified: 2011-12-19 10:12 PST (History)
15 users (show)
m_kato: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
Part 1: add UTF-8 flag to createFixupURI (2.71 KB, patch)
2011-10-06 19:24 PDT, Masatoshi Kimura [:emk]
bzbarsky: review+
Details | Diff | Splinter Review
Part 2: Use UTF-8 flag on copy (2.10 KB, patch)
2011-10-06 19:27 PDT, Masatoshi Kimura [:emk]
gavin.sharp: review+
Details | Diff | Splinter Review
Backout bug 668019 (8.35 KB, patch)
2011-10-07 08:55 PDT, Masatoshi Kimura [:emk]
gavin.sharp: review-
Details | Diff | Splinter Review
Part 1: Add UTF-8 flag to createFixupURI, v2 (2.96 KB, patch)
2011-10-07 22:23 PDT, Masatoshi Kimura [:emk]
bzbarsky: review+
Details | Diff | Splinter Review
565708: Part 1: Add UTF-8 flag to createFixupURI, v2 (2.94 KB, patch)
2011-10-18 20:26 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Part 1 for aurora and beta (2.85 KB, patch)
2011-10-19 04:35 PDT, Masatoshi Kimura [:emk]
asa: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2011-10-04 01:21:12 PDT
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 2011-10-04 08:13:24 PDT
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 [:bz] (TPAC) 2011-10-04 11:19:59 PDT
Requesting tracking for fx8.

A regression range would likely answer the question from comment 1....
Comment 4 Masatoshi Kimura [:emk] 2011-10-04 13:33:37 PDT
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 2011-10-04 14:20:40 PDT
I can't reproduce. Maybe this only occurs when the default encoding is Shift_JIS?
Comment 6 Masatoshi Kimura [:emk] 2011-10-04 14:25:12 PDT
(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] 2011-10-04 14:29:54 PDT
It should be reproducible with <https://encrypted.google.com/search?q=÷&ie=utf-8> on Latin-1 system locale.
Comment 8 Masatoshi Kimura [:emk] 2011-10-05 05:42:57 PDT
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] 2011-10-05 07:40:44 PDT
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] 2011-10-05 14:11:42 PDT
(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] 2011-10-06 19:24:40 PDT
Created attachment 565431 [details] [diff] [review]
Part 1: add UTF-8 flag to createFixupURI
Comment 12 Masatoshi Kimura [:emk] 2011-10-06 19:27:41 PDT
Created attachment 565432 [details] [diff] [review]
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 [:bz] (TPAC) 2011-10-06 19:59:20 PDT
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] 2011-10-07 08:52:19 PDT
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] 2011-10-07 08:55:39 PDT
Created attachment 565549 [details] [diff] [review]
Backout bug 668019

Requesting backout on branches to workaround a critical regression.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-10-07 13:21:36 PDT
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: gavin@gavinsharp.com] 2011-10-07 13:25:45 PDT
(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: gavin@gavinsharp.com] 2011-10-07 13:27:32 PDT
I think we can take the actual fix on beta/aurora, to avoid shipping the regression.
Comment 19 Masatoshi Kimura [:emk] 2011-10-07 22:23:41 PDT
Created attachment 565708 [details] [diff] [review]
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] 2011-10-07 22:25:09 PDT
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] 2011-10-18 05:28:39 PDT
Gavin, ping?
Comment 22 Masatoshi Kimura [:emk] 2011-10-18 05:36:07 PDT
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] 2011-10-18 20:26:14 PDT
Created attachment 567959 [details] [diff] [review]
565708: Part 1: Add UTF-8 flag to createFixupURI, v2

Un-bug690892-bitrotting.
Carrying forward r+.
Comment 24 Makoto Kato [:m_kato] (PTO 9/22-9/25) 2011-10-19 00:41:33 PDT
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] (PTO 9/22-9/25) 2011-10-19 00:42:56 PDT
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 27 Masatoshi Kimura [:emk] 2011-10-19 04:35:14 PDT
Created attachment 568008 [details] [diff] [review]
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] 2011-10-20 14:36:05 PDT
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] 2011-10-20 15:52:12 PDT
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 christian 2011-10-25 20:07:46 PDT
---------------------------------[ 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 christian 2011-10-25 22:09:35 PDT
---------------------------------[ 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 33 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-26 09:32:06 PDT
qa+ for verification in Firefox 8 and 9.
Comment 34 Masatoshi Kimura [:emk] 2011-10-27 18:32:07 PDT
FIXUP_FLAG_USE_UTF8 needs to have a document.
Comment 35 Paul Silaghi, QA [:pauly] 2011-11-22 02:37:58 PST
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] 2011-11-22 02:44:48 PST
Sorry, comment #1 is correct.
Comment 37 Paul Silaghi, QA [:pauly] 2011-11-22 04:59:35 PST
Based on comment 35 and comment 36 marking the bug as verified fixed.
Comment 38 Eric Shepherd [:sheppy] 2011-12-19 10:12:18 PST
Documented (along with the rest of nsIURIFixup):

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

And mentioned on Firefox 10 for developers.

Note You need to log in before you can comment on or make changes to this bug.