Bug 45113 – FTP EPLF does not handle directory

• Component: Page Loading
• Status: RESOLVED FIXED

[arno.]

Hi,
ftp eplf does not handle directory.
When trying to parse this example:
http://cr.yp.to/ftp/list/eplf.html

    +i8388621.48594,m825718503,r,s280, djb.html
     +i8388621.50690,m824255907,/, 514
     +i8388621.48598,m824253270,r,s612, 514.html

directory 514 is not displayed.

[arno.]

Created attachment 66381 [details]
php testcase

[arno.]

that's because of:
if (!result.type) /* access denied */ 
{
 ...
return FTPJunkEntry;
}

(in file FTPDirectoryParser.cpp, function  parseOneFTPLine)
actually, FTPDirectoryEntry is 0.  So, FTPDirectoryEntry is considered as junk.
I don't known why this test is performed only in SUPPORT_EPLF. Is it safe to remove it ?

[arno.]

Created attachment 66385 [details]
patch v0.1

first patch proposal: removes access denied test

[arno.]

Created attachment 66386 [details]
patch v0.2

second patch proposal: start FTPEntryType enum to 1

[Eric Seidel (no email)]

Comment on attachment 66385 [details]
patch v0.1

webkit-patch upload will correctly obsolete old patches and set r? on new patches for you.

Also 
+        No new tests. (OOPS!)

Should be replaced with a list of tests for this change, or an explanation of why testing is impossible.

[Eric Seidel (no email)]

Comment on attachment 66386 [details]
patch v0.2

You need to explain "why" this is correct in your ChangeLog.  As-is, this untested change makes no sense.

[Adrien Destugues]

Created attachment 239713 [details]
Patch

[Chris Dumez]

Comment on attachment 239713 [details]
Patch

Looks good, r=me.

[Chris Dumez]

Comment on attachment 239713 [details]
Patch

Wait, there seems to be a test case attached to this bug report, are we sure this cannot be layout tested?

[Adrien Destugues]

Right, I didn't notice it was there and I wasn't aware php could be used in tests. I will look into turning it into a proper layout test but I probably need to get the php-based tests running for the Haiku port first.

[Blaze Burg]

Comment on attachment 239713 [details]
Patch

Thanks for your contribution. We are clearing the review queue of old patches that no longer apply to WebKit trunk.

Please resubmit this bug with the test case turned into a layout test. If you need help, ping myself or cdumez on IRC.

[Pascal Abresch]

Hello,
I would like to work on this.
I have only found a higher level overview of how testcases are to be written for webkit, is there some specific documentation for cases like this?

[Adrien Destugues]

Hi,

The tests are found in the LayoutTests/ directory. Most of them are html files and are just loaded directly by the browser by accessing the files.

In this specific case we need a test that is loaded through a web server. When this patch was first written, this would be done using php, but now it's using python instead. You can find examples of tests like that in LayoutTests/http/tests/.

The php file attached to this ticket can be trivially converted to a python script following these examples.

Next to each test there is a file with the same name, with the extension replaced with "-expected.txt". This corresponds to the output of the DumpRenderTree executable when loading and executing that specific test. The testsuite frameworks run DumpRenderTree and compares the result with this file, to decide if the test is passed or failed.

The expected result (pass or fail) for each test is stored in "TestExpectations" files in the LayoutTests directory. The default is PASS so you don't need to do anything special if the test passes everywhere. There are several expectation files for different platforms and configurations, allowing to keep track of the current state of tests. This make it easy to see the impact of any given changes, both in terms of tests that are broken, and tests that are fixed by it.

[Pascal Abresch]

Created attachment 447520 [details]
Patch

[Pascal Abresch]

Created attachment 447523 [details]
Patch

[Pascal Abresch]

It looks like I've uploaded the wrong test expectation (from a previous iteration I've tested) which the Macos builder hit, sorry about that.

It does not really explain however why the gtk builder has hit an internal server error when trying to run this test.
https://ews-build.s3-us-west-2.amazonaws.com/GTK-WK2-Tests-EWS/r447520-3351/http/tests/misc/ftp-eplf-directory-pretty-diff.html

I don't really know what the GTK port does different to the mac port here, I would apreciate some guidance.

[Pascal Abresch]

The link above is from the previous attempt, this time it just times out on the GTK builder (while the mac builder(s) seem to run the test normally)

[Pascal Abresch]

Created attachment 447543 [details]
Patch

[Adrien Destugues]

Hello,

A bit of context for this: we don't use it for FTP in HaikuWebKit (yet) but we use it to provide directory listings for the file:// protocol accessing local directories. The EPLF format is used because it is well-specified and easy to generate in our file:// protocol handler.

Pascal now converted the attached test to Python and added it to the LayoutTests, but the expectations.txt would need to be different on different platforms. As this isn't an html test, we have no control over the exact rendering of the directory listing (different fonts are used on each platform, changing the size of elements) and we also can't use javascript to do a more specific test.

We don't know how to proceed with this, any opinions?

[Pascal Abresch]

Looking at the MacOS test results it seems there is a second problem, that of the timestamp display. The Haiku port has a patch to change the display of timestamps in the ftp code to use the OS timezone for display instead of gmttime.

How does the TestSuite deal with such situations? Is there a predictable timezone set for tests or would for example the python test have to change the send listing based on the current timezone?

And as a second question should the code to change the timestamp display be submitted in this same changeset?

The diff, for reference:
--- a/Source/WebCore/loader/FTPDirectoryParser.cpp
+++ b/Source/WebCore/loader/FTPDirectoryParser.cpp
@@ -156,7 +156,7 @@ FTPEntryType parseOneFTPLine(const char* line, ListState& state, ListResult& res
                 time_t t = static_cast<time_t>(seconds);
 
                 // FIXME: This code has the year 2038 bug
-                gmtime_r(&t, &result.modifiedTime);
+                localtime_r(&t, &result.modifiedTime);
                 result.modifiedTime.tm_year += 1900;
               }
             }

[Carlos Alberto Lopez Perez]

(In reply to Pascal Abresch from comment #16)
> It looks like I've uploaded the wrong test expectation (from a previous
> iteration I've tested) which the Macos builder hit, sorry about that.
> 
> It does not really explain however why the gtk builder has hit an internal
> server error when trying to run this test.
> https://ews-build.s3-us-west-2.amazonaws.com/GTK-WK2-Tests-EWS/r447520-3351/
> http/tests/misc/ftp-eplf-directory-pretty-diff.html
> 
> I don't really know what the GTK port does different to the mac port here, I
> would apreciate some guidance.

GTK doesn't enable FTPDIR at build time. So the test is timing out there.

I suggest skipping this test at LayoutTests/platform/gtk/TestExpectations

[Pascal Abresch]

Created attachment 452688 [details]
Patch

[Pascal Abresch]

I've updated the patch to skip the GTK test and include the MacOS test as the expected result.

[Pascal Abresch]

Created attachment 452729 [details]
Patch

[Pascal Abresch]

The layout test now passes for MacOS, it still fails for iOS duo to the differinh font sizes. How should I proceed with this?

[Carlos Alberto Lopez Perez]

(In reply to Pascal Abresch from comment #25)
> The layout test now passes for MacOS, it still fails for iOS duo to the
> differinh font sizes. How should I proceed with this?

You should add a file in this patch with the iOS test results [1] in the path LayoutTests/platform/ios/http/tests/misc/ftp-eplf-directory-expected.txt


[1] https://ews-build.s3-us-west-2.amazonaws.com/iOS-15-Simulator-WK2-Tests-EWS/452729-8760-rerun/http/tests/misc/ftp-eplf-directory-actual.txt

[Pascal Abresch]

Created attachment 453305 [details]
Patch

[Darin Adler]

Comment on attachment 453305 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453305&action=review

Thanks so much for this contribution. Looks good.

> Source/WebCore/ChangeLog:17
> +        Test: http/tests/misc/ftp-eplf-directory-expected.txt

Ideally this should list the test, ftp-eplf-directory.py, not the expected result file.

Really unfortunate there is no way to make an FTP test use text-style results instead of render tree dumps. We’ll have to rebase these expected results any time there are rendering changes (unrelated to FTP).

[Pascal Abresch]

Created attachment 453307 [details]
Patch

[Pascal Abresch]

I've updated the patch, I agree that it is unfortunate to use the precise rendering output here since the test only wants to roughly test whether the directory is display, and not if it is displayed in the exact same way.

Thanks for all the help!

[Darin Adler]

Long ago this was Mozilla source code; I wonder if Firefox has a similar bug. I couldn’t find it myself by searching. Maybe they changed this long ago.

[Darin Adler]

Comment on attachment 453307 [details]
Patch

Waiting for EWS to show passing results, then someone should set commit-queue+ too.

[Pascal Abresch]

Looks like ews passed now, it had a (probably unrelated) failure in wk2 ios before, but it worked after a restart.

It should be ready to commit now.

[EWS]

Committed r290595 (247871@main): <https://commits.webkit.org/247871@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453307 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89555741>