See bug 942495.

On Nightly, the issue was not reproduced. Probably because the default encoding is windows-1252 which is has round-trip mapping from/to Unicode.

I confirmed I had to set [Options]-[Content]-[Advanced]-[Fallback Character Encoding] to Japanese to reproduce the issue.

The error recovery in the previous patch didn't work if the sequence contains a partial sequence (such as 0xE3 0x81). The new patch will give up and fallback if the sequence contains an invalid sequence as UTF-8.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=a18b0647468a

The "fix" for bug 427089 is totally broken. It prevented access to files in <ftp://ftp.picosystems.net/> (from bug 942495). I confirmed this didn't regress bug 427089.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=2c5dfa4e775d

Comment on attachment 8338498 [details] [diff] [review] Ensure non-ASCII filenames are accessible in FTP directory listings Review of attachment 8338498 [details] [diff] [review]: ----------------------------------------------------------------- Tested with ftp://ftp.picosystems.net/ Still 550 when switched to Shift_JIS, is that expected? ::: netwerk/streamconv/converters/nsIndexedToHTML.cpp @@ +865,3 @@ > // Adding trailing slash helps to recognize whether the URL points to a file > // or a directory (bug #214405). > + if ((type == nsIDirIndex::TYPE_DIRECTORY) &&(loc.Last() != '/')) { add space after && @@ +884,5 @@ > // Without it, the trailing '/' will be escaped, and links from within > // 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, escapeBuf); to make clear what |escapeBuf| is please rename it to locEscaped @@ +895,5 @@ > + // Try to convert non-ASCII bytes to Unicode using UTF-8 decoder. > + nsCOMPtr<nsIUnicodeDecoder> decoder = > + mozilla::dom::EncodingUtils::DecoderForEncoding("UTF-8"); > + decoder->SetInputErrorBehavior(nsIUnicodeDecoder::kOnError_Signal); > + int32_t len = escapeBuf.Length(); blank line over this line (try to split to blocks of logically belonging code) @@ +898,5 @@ > + decoder->SetInputErrorBehavior(nsIUnicodeDecoder::kOnError_Signal); > + int32_t len = escapeBuf.Length(); > + int32_t outlen = 0; > + rv = decoder->GetMaxLength(escapeBuf.get(), len, &outlen); > + if (NS_FAILED(rv)) return rv; return rv on its own line @@ +899,5 @@ > + int32_t len = escapeBuf.Length(); > + int32_t outlen = 0; > + rv = decoder->GetMaxLength(escapeBuf.get(), len, &outlen); > + if (NS_FAILED(rv)) return rv; > + nsAutoArrayPtr<PRUnichar> outbuf(new PRUnichar[outlen + 1]); ! check outlen is in <0,PR_MAX_INT32-1>, if not, return NS_ERROR_FAILURE. @@ +904,5 @@ > + rv = decoder->Convert(escapeBuf.get(), &len, outbuf, &outlen); > + // Use the result only if the sequence is valid as UTF-8. > + if (rv == NS_OK) { > + outbuf[outlen] = 0; > + utf16URI.Append(outbuf); do utf16URI.Append(outbuf, outlen);

(In reply to Honza Bambas (:mayhemer) from comment #8) > Tested with ftp://ftp.picosystems.net/ > Still 550 when switched to Shift_JIS, is that expected? Yes, that's expected. The FTP directory listing has a bug about switching the encoding from the menu which is out of scope of this bug. Please change the default fallback encoding from Options-Content and reload the page to test. Also the second patch is needed. I'll fix other points.

> ! check outlen is in <0,PR_MAX_INT32-1>, if not, return NS_ERROR_FAILURE. |+ 1| was needed to make room for the trailing null byte. It was changed to |utf16URI.Append(outbuf, outlen);|, so outlen will no longer overflow.

Comment on attachment 8339240 [details] [diff] [review] Revert bug 427089 Stealing review on Michal's request.

(In reply to Masatoshi Kimura [:emk] from comment #6) > Created attachment 8339240 [details] [diff] [review] > Revert bug 427089 > > The "fix" for bug 427089 is totally broken. It prevented access to files in > <ftp://ftp.picosystems.net/> (from bug 942495). > I confirmed this didn't regress bug 427089. How? And I assume your fix is what fixes bug 427089 again? What are other behavior changes when you back this patch out? How this patch influences your patch?

By the patch of bug 427089, <ftp://ftp.picosystems.net/pub/DATEL_%E7%B7%8F%E5%90%88%E3%82%AB%E3%82%BF%E3%83%AD%E3%82%B0_1986_87.pdf> is treated as the same URL as <ftp://ftp.picosystems.net/pub/DATEL_%91%8D%8D%87%83J%83%5E%83%8D%83O_1986_87.pdf> which is the cause of the 550 error. This (UTF-8 to system charset) conversion was needed before my patch part 1 because non-ASCII path parts in URL were decoded using the system charset, then encoded back to UTF-8. But my patch made the non-ASCII bytes transparent, so the double conversion is breaking the access to file names which happens to be a valid sequence as UTF-8.

> <ftp://ftp.picosystems.net/pub/DATEL_%E7%B7%8F%E5%90%88%E3%82%AB%E3%82%BF%E3%83%AD%E3%82%B0_1986_87.pdf> is treated as the same URL as <ftp://ftp.picosystems.net/pub/DATEL_%91%8D%8D%87%83J%83%5E%83%8D%83O_1986_87.pdf> To be precise, it depends on the fallback charset. This is when the fallback charset is Shift_JIS.

Comment on attachment 8339240 [details] [diff] [review] Revert bug 427089 Not locally tested. FTP backout changes conform the original patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6882f119dcb1 We will need a test infrastructure for the FTP directory listing...

Hm, the status was not changed because of the word "revert"?

Comment on attachment 8339240 [details] [diff] [review] Revert bug 427089 [Approval Request Comment] Bug caused by (feature/regressing bug #): 427089 User impact if declined: Some files on FTP directory listings will not be accessible. Testing completed (on m-c, etc.): landed on m-c, tested locally. Risk to taking this patch (and alternatives if risky): pretty low String or IDL/UUID changes made by this patch: none

Missed the 28 train.