263462 - NS_UnescapeURL doesn't honor esc_OnlyNonASCII

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jungshik Shin]

NS_UnescapeURL doesn't check 'esc-OnlyNonASCII' flag. When unescaping, setting
it on should get ASCII characters to be skipped according to the 'documentation'
in nsEscape.h:

  esc_OnlyNonASCII   = PR_BIT(12), /* causes _graphic_ ascii octets (0x20-0x7E) 
                                    * to be skipped when escaping. causes all
                                    * ascii octets to be skipped when unescaping *

I wrote that comment, but apparently I didn't implement it in nsEscape.cpp
leading to the discrepancy and bug 243504 (I specified 'esc_OnlyNonASCII' in
nsUTF8ConverterService.cpp expecting ASCII characters to be kept 'url-escaped'
when calling NS_UnescapeURL, but the flag has no effect and ASCII characters are
escaped). 

I'm gonna upload a patch in a moment.

Comment 1

[Jungshik Shin]

Attachment: patch

implement esc_OnlyNonASCII

Comment 2

[Jungshik Shin]

Comment on attachment 161462 [details] [diff] [review]
patch

asking for r/sr

Comment 3

[Scott MacGregor]

plussing for the aviary branch since this fix is required to fix 243504 which is
aviary1.0+

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 161462 [details] [diff] [review]
patch

sr=jst

Comment 5

[Darin Fisher]

Comment on attachment 161462 [details] [diff] [review]
patch

looks good, r=darin

Comment 6

[Scott MacGregor]

I checked this into the aviary 1.0 branch

Comment 7

[Jungshik Shin]

Comment on attachment 161462 [details] [diff] [review]
patch

asking for a1.7
it's already gotten into aviary 1.0 branch

Comment 8

[Mike Kaply [:mkaply]]

Comment on attachment 161462 [details] [diff] [review]
patch

a=mkaply for 1.7

Comment 9

[Jungshik Shin]

checked into 1.7 and trunk. thanks all.

Comment 10

[Scott MacGregor]

I think this change to our escaping code caused:

Bug #273109

Comment 11

[WADA:World Anti-bad-Duping Agency]

Re-open because patch for this bug is backed out on Thunderbird 1.0 release
build(temporary?) to resolve more serious regression, Bug 273109.
See Bug 273109 Comment #17.
This probably revived problem of Bug 243504(Bug 239192 also).
See Bug 243504 Comment #31. 

Comment 12

[Scott MacGregor]

clearing fixed 1.7.5 flag as the patch has been backed out of there as well

Comment 13

[Asa Dotzler [:asa]]

Doesn't look like this is going to make 1.8a6

Comment 14

[Jungshik Shin]

This was kept on trunk. This bug is only for 1.7/1.0 branches.

Comment 15

[Scott MacGregor]

I put this back on the thunderbird 1.0 branch. re-closing the bug.

Comment 16

[WADA:World Anti-bad-Duping Agency]

Reopening to comfirm status on 1.7 branch.

Mscott, pacth looks to be still backed-out on 1.7 branch. 
Is it intentional?

(Aviary)
> http://lxr.mozilla.org/aviarybranch/source/xpcom/io/nsEscape.cpp#437
> 437     PRBool ignoreNonAscii = (flags & esc_OnlyASCII);
> 438     PRBool ignoreAscii = (flags & esc_OnlyNonASCII);
> 439     PRBool writing = (flags & esc_AlwaysCopy);

(Mozilla 1.7 branch)
> http://lxr.mozilla.org/mozilla1.7/source/xpcom/io/nsEscape.cpp#437
> 437     PRBool ignoreNonAscii = (flags & esc_OnlyASCII);
> 438     PRBool writing = (flags & esc_AlwaysCopy);

Comment 17

[Jungshik Shin]

(In reply to comment #16)
> Reopening to comfirm status on 1.7 branch.
> 
> Mscott, pacth looks to be still backed-out on 1.7 branch. 
> Is it intentional?

It cannot be relaned until I get approval for landing the patch for bug 274264
and land it in 1.7 branch.

Comment 18

[WADA:World Anti-bad-Duping Agency]

Oh, delay by waiting for approval problem again...
If so, I think setting "blocking 1.7.6" flag is better.
Jshin, what do you think?

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Not a blocker.  blocking1.7.6-

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this fixed on the trunk?  If so, should it be marked as such?

Comment 21

[Roberto Salomon]

I made some updates to bug 243504 that I think contains some additional
information regarding the issue. Apparently it has been solved in the user
interface but not from the command line.

Comment 22

[Magnus Melin [:mkmelin]]

The patch is apparently checked in a long time ago.
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsEscape.cpp#384

Marking FIXED.