Closed Bug 989576 Opened 6 years ago Closed 6 years ago

Categories

(Core :: Networking: FTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: emk, Assigned: emk)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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/>.
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.
(By the way, it was difficult to get the streamconverted source.)
Asking whoever comes first for review.

Sorry I should have noticed this when I was requested feedback.
Attachment #8398876 - Flags: review?(neil)
Attachment #8398876 - Flags: review?(michal.novotny)
Attachment #8398876 - Flags: review?(honzab.moz)
Forgot to qref.
Attachment #8398876 - Attachment is obsolete: true
Attachment #8398876 - Flags: review?(neil)
Attachment #8398876 - Flags: review?(michal.novotny)
Attachment #8398876 - Flags: review?(honzab.moz)
Attachment #8398878 - Flags: review?(neil)
Attachment #8398878 - Flags: review?(michal.novotny)
Attachment #8398878 - Flags: review?(honzab.moz)
Keywords: regression
Attachment #8398878 - Flags: review?(michal.novotny) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b93c9a7e21a5
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Flags: in-testsuite?
Attachment #8398878 - Flags: review?(neil)
Attachment #8398878 - Flags: review?(honzab.moz)
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?
(In reply to neil@parkwaycc.co.uk 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.
Attachment #8398928 - Flags: review?(neil)
Marking leave-open for the followup.
Keywords: leave-open
Blocks: 26767
Patch looks good but site seems to be down again :-(
Attachment #8398928 - Flags: review?(neil) → review+
https://hg.mozilla.org/mozilla-central/rev/b1c988d5a43d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.