Bug 15415 – parse the end of line about the FTPDIR list error!

• Component: Page Loading
• Status: NEW

[wesleyZeng]

Split ftp list with '\n', but the variable foundNewLine(loader/FTPDirectoryDocument.cpp:389) is setted when the current character is '\r'. 
    See details about diffs:

Index: FTPDirectoryDocument.cpp
===================================================================
--- FTPDirectoryDocument.cpp    £¨ÐÞ¶©°æ 26103£©
+++ FTPDirectoryDocument.cpp    £¨¹¤×÷¿½±´£©
@@ -386,7 +386,6 @@

         if (c == '\r') {
             *m_dest++ = '\n';
-            foundNewLine = true;
             // possibly skip an LF in the case of an CRLF sequence
             m_skipLF = true;
         } else if (c == '\n') {
@@ -394,6 +393,7 @@
                 *m_dest++ = c;
             else
                 m_skipLF = false;
+            foundNewLine = true;
         } else {
             *m_dest++ = c;
             m_skipLF = false;

[Robert Blaut]

(In reply to comment #0)
> Split ftp list with '\n', but the variable
> foundNewLine(loader/FTPDirectoryDocument.cpp:389) is setted when the current
> character is '\r'. 
>     See details about diffs:

Can someone proceed with this report, please? 

wesleyZeng, did you see live example (URL) that the reported issue caused problems?

[wesleyZeng]

'\n' is the end of line(EOL) on Linux , "\r\n" is EOL on Win32, and '\r' is EOL on Mac.

So, if the current character is '\r' , m_skipLF is setted; 
if the current character is '\n', foundNewLine should be setted; 

e.g
ftp://ftp.mozilla.org/pub

[wesleyZeng]

Created attachment 19859 [details]
fix ftpdir

[Brady Eidson]

Comment on attachment 19859 [details]
fix ftpdir

This was a good find, but I think the fix is wrong for this reason:
We treat any of the sequences - /r, /r/n, and /n - as newlines.  foundNewLine should be set whenever we encounter *any* of these sequences.  I think moving the foundNewLine assignment isn't right, but adding the new one is (so we have it in both places)

[Brady Eidson]

On the note of bug fixes coming in to this code, we really need to get the layout tests augmented to regression test it  =/