989576 - Bug 948901 broke ftp://ftp.asu.ru/incoming/

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Masatoshi Kimura [:emk]]

Steps to reproduce:
1. Open <ftp://ftp.asu.ru/incoming/>.
2. Click "111 презентации".

Actual result:
550 Failed to change directory.

Expected result:
Navigate to the link.

The copied link was changed from <ftp://ftp.asu.ru/incoming/111%20%EF%F0%E5%E7%E5%ED%F2%E0%F6%E8%E8/> to <ftp://ftp.asu.ru/incoming/111%20%D0%BF%D1%80%D0%B5%D0%B7%D0%B5%D0%BD%D1%82%D0%B0%D1%86%D0%B8%D0%B8/>.

Comment 1

[Masatoshi Kimura [:emk]]

Before the patch, non-ASCII characters in the URL were percent-encoded:
"<a class="dir" href="111%20%EF%F0%E5%E7%E5%ED%F2%E0%F6%E8%E8/">111 ïðåçåíòàöèè</a>"
But with the patched build, they are not percent-encoded anymore:
"<a class="dir" href="111%20презентации/">111 презентации</a>"

We must percent-encode the URL unless the document is encoded in UTF-8.

Comment 2

[Masatoshi Kimura [:emk]]

(By the way, it was difficult to get the streamconverted source.)

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Restore removed code doing percent-encode for non-ASCII bytes

Asking whoever comes first for review.

Sorry I should have noticed this when I was requested feedback.

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Restore removed code doing percent-encode for non-ASCII bytes

Forgot to qref.

Comment 5

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b93c9a7e21a5

Comment 6

[[e-mail]]

Comment on attachment 8398878 [details] [diff] [review]
Restore removed code doing percent-encode for non-ASCII bytes

Sorry for introducing this regression. My bugmail is down this weekend otherwise I might have noticed this earlier.

>+    nsXPIDLCString encoding;
>+    rv = mParser->GetEncoding(getter_Copies(encoding));
>+    if (NS_FAILED(rv)) return rv;
Please do NOT call GetEncoding. The value returned is not meaningful. Only the HTML parser knows the true encoding (from the user override, detector, hostname or fallback as appropriate).

>         // that directory will be incorrect
>         escFlags = esc_Forced | esc_OnlyASCII | esc_AlwaysCopy | esc_FileBaseName | esc_Colon | esc_Directory;
>     }
>     NS_EscapeURL(loc.get(), loc.Length(), escFlags, locEscaped);
>     // esc_Directory does not escape the semicolons, so if a filename
>     // contains semicolons we need to manually escape them.
>     // This replacement should be removed in bug #473280
>     locEscaped.ReplaceSubstring(";", "%3b");
>+    if (!encoding.EqualsLiteral("UTF-8")) {
>+        // Escape all non-ASCII bytes to preserve the raw value.
>+        nsAutoCString outstr;
>+        NS_EscapeURL(locEscaped, esc_AlwaysCopy | esc_OnlyNonASCII, outstr);
>+        locEscaped = outstr;
>+    }
I am confused as to why you need to call NS_EscapeURL twice. Surely you should just tweak escFlags?

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: Followup, stop using mParser->GetEncoding()

(In reply to [e-mail] from comment #6)
> Please do NOT call GetEncoding. The value returned is not meaningful. Only
> the HTML parser knows the true encoding (from the user override, detector,
> hostname or fallback as appropriate).

OK, I decided to always percent-encode the URL because we can't get the page encoding reliably.

> I am confused as to why you need to call NS_EscapeURL twice. Surely you
> should just tweak escFlags?

Fair enough. Removed esc_OnlyASCII from escFlags.

Comment 8

[Masatoshi Kimura [:emk]]

Marking leave-open for the followup.

Comment 9

[Phil Ringnalda (:philor)]

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

Comment 10

[[e-mail]]

Patch looks good but site seems to be down again :-(

Comment 11

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1c988d5a43d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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