FTP: Detect the character encoding only after the entire listing is received.

FTP: Detect the character encoding only after the entire listing is received.

Otherwise the parser could be confused extremely easily. Imagine a listing
which has only ASCII characters at the beginning, but after we detect it
as ASCII-encoded, some UTF-8 bytes appear. It can be made more complex though,
for example the beginning might look like UTF-8, but the entire listing
may use a different encoding incompatible with UTF-8 (that was the case here,
see bug).

This change also simplifies the directory listing parser. When we always
have the entire listing when parsing, some complex code can be removed.

BUG=76171
TEST=see bug

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79490

[Paweł Hajdan Jr., 2011-03-17 10:09:25]

What do you think about this change? Technically just to fix the bug a smaller
change is needed, but then it would make some more complex parts of this code
useless, so I've removed them.

[wtc, 2011-03-17 20:44:25]

phajdan.jr: (I didn't look at the CL.)  Does this mean you
want to buffer the entire directory listing?  Do you have a
max number of bytes you're willing to buffer?

What do you need to detect the character encoding?  For the
month names in last modification times?

[Paweł Hajdan Jr., 2011-03-18 08:56:28]

On 2011/03/17 20:44:25, wtc wrote:
> Does this mean you want to buffer the entire directory listing? 

Yes, that's correct.

> Do you have a max number of bytes you're willing to buffer?

No. I remember a similar discussion before. This code runs in the renderer
process, so if it OOMs, just the renderer will be killed. A possibly more evil
scenario would be a long listing that takes a lot of CPU time to process. I'm
not sure how serious is that, I think the listing processing code should run in
linear time (based on size of the listing in bytes).

Please note that even if the code in net/ "streams" the HTML data in multiple
chunks as the listing data is received and processed, that HTML has still to be
in renderer's memory in some form. I think it doesn't make much difference
whether it's a raw string, an HTML string, internal WebKit data structures, or a
mixture of all three when it comes to memory usage.

Anyway, I can implement some hardcoded limit, but I'd prefer to avoid doing
that.

> What do you need to detect the character encoding?  For the
> month names in last modification times?

This is tricky. Actually the month names are
http://codereview.chromium.org/6697021/ and I don't need (at least not yet) the
entire listing for that.

Please take a look at the linked bug:
http://code.google.com/p/chromium/issues/detail?id=76171. The issue is that the
listing is relatively long, and we initially detect UTF-8 encoding, but late in
the listing there are few bytes that cannot be decoded in UTF-8, but then it's
too late to change the decision about the encoding and parsing has to fail.

Another solution I considered was increasing encoding detection threshold (1024
bytes currently), but for any limit L the code would still be broken if there is
a "breaking" character at position > L.

I have tested this CL for most common listing sizes, and there is no visible
performance impact.

[Paweł Hajdan Jr., 2011-03-24 07:55:16]

ping

[eroman, 2011-03-24 22:27:56]

I've been out sick, or I would have responded earlier :)

I agree with your assessment that this is an OK thing to do, since we are
buffering in the renderer not the browser.

I am OK with waiting for the entire response before displaying any of it, but it
isn't optimal.

We have the same sort of problem with mime-sniffing. What we do for that (which
by no means is ideal either), is to have both "soft" and "hard" buffering
limits. Basically the code buffers (stalls the response) until it has received
enough bytes to be confident about its decision. So in some cases it may buffer
more than in other cases.

Of course though, it can be tricked by content which does weird stuff after the
maximum buffer amount.

I will review the code in a moment, just wanted to give my high-level feedback
first.


I am fine with waiting for the entire response before

[eroman, 2011-03-24 23:09:35]

lgtm

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser.cc (right):

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser.cc:30: // Use first encoding that can be
used to decode the text.
I wander if it would be more flexible to move this loop further out.

In other words, rather than using the first encoding the decodes the text, use
the first encoding whose decoded text is parseable:

for each possible encoding:
  decode to UTF16
  split into lines
  for each directory listing type:
    try to parse all lines
    if successful, return OK (else continue trying parsers/decoders)

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser_ls.cc (right):

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_ls.cc:178: if
(!base::StringToInt64(columns[2 + column_offset], &entry.size)) {
I only glossed over this section quickly. I assume it hasn't changed from
before, and is just a move right?

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser_unittest.cc (right):

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_unittest.cc:98: ASSERT_EQ(lines.size() / 8,
entries.size());
nit: these two asserts could be combined into one:

ASSERT_EQ(8 * entries.size(), lines.size());

(I am fine either way).

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_unittest.cc:115: FtpDirectoryListingEntry
entry = entries[i];
nit: could make this |const FtpDirectoryListingEntry&|

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser_vms.cc (right):

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_vms.cc:180: namespace net {
nit: i suggest moving this up, so the anonymous namespace is also enclosed
within net (avoids having to prefix the types with net::)

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser_vms_unittest.cc (right):

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_vms_unittest.cc:86: "README.TXT;1 2
APR-2000 10:40:39",
how come the test data has changed?

[Paweł Hajdan Jr., 2011-03-26 09:47:50]

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser.cc (right):

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser.cc:30: // Use first encoding that can be
used to decode the text.
On 2011/03/24 23:09:35, eroman wrote:
> I wander if it would be more flexible to move this loop further out.
> 
> In other words, rather than using the first encoding the decodes the text, use
> the first encoding whose decoded text is parseable:
> 
> for each possible encoding:
>   decode to UTF16
>   split into lines
>   for each directory listing type:
>     try to parse all lines
>     if successful, return OK (else continue trying parsers/decoders)

Yes, it sounds like a good idea. I'd prefer to do it in a separate patch though,
to avoid doing too much in one CL (this is already quite big).

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser_ls.cc (right):

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_ls.cc:178: if
(!base::StringToInt64(columns[2 + column_offset], &entry.size)) {
On 2011/03/24 23:09:35, eroman wrote:
> I only glossed over this section quickly. I assume it hasn't changed from
> before, and is just a move right?

Yes, "ls -l" is just a move. VMS had some non-trivial modifications.

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser_unittest.cc (right):

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_unittest.cc:98: ASSERT_EQ(lines.size() / 8,
entries.size());
On 2011/03/24 23:09:35, eroman wrote:
> nit: these two asserts could be combined into one:
> 
> ASSERT_EQ(8 * entries.size(), lines.size());
> 
> (I am fine either way).

Done.

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_unittest.cc:115: FtpDirectoryListingEntry
entry = entries[i];
On 2011/03/24 23:09:35, eroman wrote:
> nit: could make this |const FtpDirectoryListingEntry&|

Done.

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser_vms.cc (right):

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_vms.cc:180: namespace net {
On 2011/03/24 23:09:35, eroman wrote:
> nit: i suggest moving this up, so the anonymous namespace is also enclosed
> within net (avoids having to prefix the types with net::)

Done.

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser_vms_unittest.cc (right):

http://codereview.chromium.org/6670085/diff/5002/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_vms_unittest.cc:86: "README.TXT;1 2
APR-2000 10:40:39",
On 2011/03/24 23:09:35, eroman wrote:
> how come the test data has changed?

I refactored the tests slightly, and removed some cases that the previous parser
would reject but now will be accepted (for simplicity and compatibility).

After seeing this comment I added an additional test case to restore some
previously existing listing lines that we should still be rejecting.