997030 - Export Bookmarks to html wrong with utf8

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Chen haojun]

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.43 BIDUBrowser/6.x Safari/537.31

Steps to reproduce:

Export Bookmarks to html


Actual results:

my bookmark address is
http://bt.ktxp.com/search.php?keyword=%E5%A6%84%E6%83%B3%E5%AD%A6%E7%94%9F%E4%BC%9A

it change
<A HREF="http://bt.ktxp.com/search.php?keyword=%25E5%25A6%2584%25E6%2583%25B3%25E5%25AD%25A6%25E7%2594%259F%25E4%25BC%259A" LAST_MODIFIED="1396869229" ICON_URI="http://static.ktxp.com/favicon.ico" ICON="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8/9hAAAAyElEQVQ4jaWS0Q3DIAwF3yAMkkXYAltiEBZhAIQ9AINkEfoBhbRK0jRBMlIUOO5hAJLyqNp0c+wCKBk4taBk7gEAgLXCib8PICmgtEwj8SApDaz2GsCpbRtzgBM/IIcGlAwoh3lSjzBAG6tTA0oLWOtYzBpBaQHJCsrhAqAbkKxw4sFam8mvCO9FrHFsohxGscZzA0qmaSbTI7TvbaxtB3YAs22stWc3HVS6Rf2w2L0Dp3ZG+HqV7V8Eazzrgrn0Cg8B/4wBeFAvPGD3uX8X93AAAAAASUVORK5CYII=">关键字 &quot;妄想学生会&quot; 的搜索结果 - 动漫下载|BT|漫画|动画|游戏 - 极影动漫</A>

in the html



Expected results:

I hava to many 25

IT only happened in 31.0a1

29 doesn't have this problem

Comment 1

[Marco Bonardo [:mak]]

sounds like a regression in the code generating the html

Comment 2

[Chen haojun]

it also happen in 30(beta)

Comment 3

[Mike Hoye [:mhoye]]

With a decent regression range, I'm calling this a [diamond][good-next-bug].

Comment 4

[Marco Bonardo [:mak]]

(In reply to Mike Hoye [:mhoye] from comment #3)
> With a decent regression range, I'm calling this a [diamond][good-next-bug].

I don't think there's need of a more precise regrange, this has surely been caused by bug 968177, that is already being blocked here.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

We're in our fourth week of FF30 Beta - at this stage backouts to known good state are preferred if that's an option here, or a very low risk forward fix. Can you give an update on your progress here?

Comment 6

[Marco Bonardo [:mak]]

(In reply to Lukas Blakk [:lsblakk] from comment #5)
> We're in our fourth week of FF30 Beta - at this stage backouts to known good
> state are preferred if that's an option here, or a very low risk forward
> fix. Can you give an update on your progress here?

I'll work on this today, so I will have an update (hopefully a patch) before EOD, otherwise I will check if a backout would be safe (I'm a little scared of backing out, since a lot of code changed here)

Comment 7

[Chen haojun]

(In reply to Lukas Blakk [:lsblakk] from comment #5)
> We're in our fourth week of FF30 Beta - at this stage backouts to known good
> state are preferred if that's an option here, or a very low risk forward
> fix. Can you give an update on your progress here?


I use the newest firefox 30(BETA) and it's still happen

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1

This was my fault, I mis-changed the behavior using encodeURI, when the old code was just escaping some url. our URIs are already encoded so we ended up double encoding.
This patch restores the original behavior.

It's a safe patch, very trivial and with a dedicated test, should be good for late beta.

Comment 9

[Asaf Romano (gone)]

Comment on attachment 8425588 [details] [diff] [review]
patch v1

r=mano for this partial back-out. I don't fully understand why this code path need stripped-down escaping, but this is as good as it was.

Comment 10

[Marco Bonardo [:mak]]

Yes, as you suggested I verified also Cu.nsINetUtil.ESCAPE_URL_MINIMAL would do, and would be more proper, but it's late to make experiments, and experimenting sounds more expensive than the benefit for this low priority code. Let's just go back to legacy code that we are sure worked as expected for years.

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/fx-team/rev/b2c1d53130ff

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8425588 [details] [diff] [review]
patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 997030
User impact if declined: uris may be double encoded in bookmarks.html, causing nonworking uris on re-import
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk (partial backout of the change that caused this bug)
String or IDL/UUID changes made by this patch: none

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-beta/rev/05baa07365d9
https://hg.mozilla.org/releases/mozilla-aurora/rev/2031c294fd9c

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/05baa07365d9