Make FTP directory parser less strict

Make FTP directory parser less strict

FTP directory listing returns empty result if encoding detection or
conversion fails. This CL puts less strict constraint on conversion
so that the whole list can survive some filenames with broken chars
and be shown to users. Other browsers seem to work this way.

The broken chars can happen due to wrong encoding detection, which
cannot be avoided 100%, as reported in the bug.

BUG=676762
R=mmenke@chromium.org, phajdan.jr@chromium.org

Review-Url: https://codereview.chromium.org/2608213002 .
Cr-Commit-Position: refs/heads/master@{#441856}
Committed: https://chromium.googlesource.com/chromium/src/+/ed3ef90ecc3c01246a3d838fbeefdab72c94c228

[Jinsuk Kim, 2017-01-04 00:19:37]

jinsukkim@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2017-01-04 14:12:41]

Sounds good. Could you please add a regression test for this?

Landing binary files may be tricky using Rietveld, so let me know if you need
some help with that. Also let me know if you have any questions about the test
harness for FTP listings.

Otherwise, feel free to get review from any other src/net OWNERS in your
timezone for faster turnaround.

[Jinsuk Kim, 2017-01-04 22:48:24]

The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 22:49:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jinsuk Kim, 2017-01-05 00:16:38]

On 2017/01/04 14:12:41, Paweł Hajdan Jr. wrote:
> Sounds good. Could you please add a regression test for this?
>
Done.
 
> Landing binary files may be tricky using Rietveld, so let me know if you need
> some help with that. Also let me know if you have any questions about the test
> harness for FTP listings.
> 

Done this before. Will land it manually.

> Otherwise, feel free to get review from any other src/net OWNERS in your
> timezone for faster turnaround.

[Jinsuk Kim, 2017-01-05 00:17:07]

jinsukkim@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2017-01-05 00:20:54]

https://codereview.chromium.org/2608213002/diff/20001/net/ftp/ftp_directory_l...
File net/ftp/ftp_directory_listing_parser.cc (right):

https://codereview.chromium.org/2608213002/diff/20001/net/ftp/ftp_directory_l...
net/ftp/ftp_directory_listing_parser.cc:94: base::OnStringConversionError::SKIP,
Would SUBSTITUTE be better, so we display a character that's more obviously
wrong, rather than a file name that just mysteriously can't be downloaded, for
unclear reasons?

[Jinsuk Kim, 2017-01-05 00:25:39]

https://codereview.chromium.org/2608213002/diff/20001/net/ftp/ftp_directory_l...
File net/ftp/ftp_directory_listing_parser.cc (right):

https://codereview.chromium.org/2608213002/diff/20001/net/ftp/ftp_directory_l...
net/ftp/ftp_directory_listing_parser.cc:94: base::OnStringConversionError::SKIP,
On 2017/01/05 00:20:53, mmenke wrote:
> Would SUBSTITUTE be better, so we display a character that's more obviously
> wrong, rather than a file name that just mysteriously can't be downloaded, for
> unclear reasons?

Tried this but then the processing in |ParseListing| below fails. I stopped
there rather than debugging it further. Probably it doesn't like the replaced
char. 

In whichever case the file cannot be downloaded because it got different from
the original name that has broken chars.

[Jinsuk Kim, 2017-01-05 00:45:28]

https://codereview.chromium.org/2608213002/diff/20001/net/ftp/ftp_directory_l...
File net/ftp/ftp_directory_listing_parser.cc (right):

https://codereview.chromium.org/2608213002/diff/20001/net/ftp/ftp_directory_l...
net/ftp/ftp_directory_listing_parser.cc:94: base::OnStringConversionError::SKIP,
On 2017/01/05 00:25:39, Jinsuk Kim wrote:
> On 2017/01/05 00:20:53, mmenke wrote:
> > Would SUBSTITUTE be better, so we display a character that's more obviously
> > wrong, rather than a file name that just mysteriously can't be downloaded,
for
> > unclear reasons?
> 
> Tried this but then the processing in |ParseListing| below fails. I stopped
> there rather than debugging it further. Probably it doesn't like the replaced
> char. 
> 
> In whichever case the file cannot be downloaded because it got different from
> the original name that has broken chars.

Your feedback got me to look into it further... SUBSTITUTE option works if
another conversion used in |FillInRawName| also uses SUBSTITUTE. This looks
better as you pointed out, by making the broken character more obvious. Will
update accordingly. Thanks.

[Paweł Hajdan Jr., 2017-01-05 15:50:22]

LGTM

[mmenke, 2017-01-05 16:03:04]

LGTM as well.  Thanks for taking the time to investigate using substitution!

[Jinsuk Kim, 2017-01-06 02:57:10]

Description was changed from

==========
Make FTP directory parser less strict

FTP directory listing returns empty result if encoding detection or
conversion fails. This CL puts less strict constraint on conversion
so that the whole list can survive some filenames with broken chars
and be shown to users. Other browsers seem to work this way.

The broken chars can happen due to wrong encoding detection, which
cannot be avoided 100%, as reported in the bug.

BUG=676762
==========

to

==========
Make FTP directory parser less strict

FTP directory listing returns empty result if encoding detection or
conversion fails. This CL puts less strict constraint on conversion
so that the whole list can survive some filenames with broken chars
and be shown to users. Other browsers seem to work this way.

The broken chars can happen due to wrong encoding detection, which
cannot be avoided 100%, as reported in the bug.

BUG=676762
R=mmenke@chromium.org, phajdan.jr@chromium.org

Review-Url: https://codereview.chromium.org/2608213002 .
Cr-Commit-Position: refs/heads/master@{#441856}
Committed:
https://chromium.googlesource.com/chromium/src/+/ed3ef90ecc3c01246a3d838fbeef...
==========

[Jinsuk Kim, 2017-01-06 02:57:12]

Committed patchset #3 (id:40001) manually as
ed3ef90ecc3c01246a3d838fbeefdab72c94c228 (presubmit successful).

[loyso (OOO), 2017-01-06 03:57:29]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2614103002/ by loyso@chromium.org.

The reason for reverting is: Fails on all platforms.

https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%....

[loyso (OOO), 2017-01-06 03:58:30]

Public link:
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/bui...

[findit-for-me, 2017-01-06 04:00:43]

FYI: Findit identified this CL at revision 441856 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...