Bug 45113 - FTP EPLF does not handle directory
Summary: FTP EPLF does not handle directory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://renevier.net/misc/eplf.php
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-09-02 09:55 PDT by arno.
Modified: 2022-02-28 06:52 PST (History)
13 users (show)

See Also:


Attachments
php testcase (233 bytes, application/x-httpd-php)
2010-09-02 09:56 PDT, arno.
no flags Details
patch v0.1 (1.71 KB, patch)
2010-09-02 10:07 PDT, arno.
no flags Details | Formatted Diff | Diff
patch v0.2 (1.41 KB, patch)
2010-09-02 10:10 PDT, arno.
eric: review-
Details | Formatted Diff | Diff
Patch (2.06 KB, patch)
2014-10-13 00:45 PDT, Adrien Destugues
bburg: review-
Details | Formatted Diff | Diff
Patch (6.95 KB, patch)
2021-12-18 08:00 PST, Pascal Abresch
no flags Details | Formatted Diff | Diff
Patch (6.95 KB, patch)
2021-12-18 10:44 PST, Pascal Abresch
no flags Details | Formatted Diff | Diff
Patch (7.00 KB, patch)
2021-12-18 23:47 PST, Pascal Abresch
no flags Details | Formatted Diff | Diff
Patch (7.64 KB, patch)
2022-02-20 01:48 PST, Pascal Abresch
no flags Details | Formatted Diff | Diff
Patch (7.64 KB, patch)
2022-02-21 07:31 PST, Pascal Abresch
no flags Details | Formatted Diff | Diff
Patch (10.87 KB, patch)
2022-02-26 12:11 PST, Pascal Abresch
no flags Details | Formatted Diff | Diff
Patch (10.86 KB, patch)
2022-02-26 13:41 PST, Pascal Abresch
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description arno. 2010-09-02 09:55:16 PDT
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.
Comment 1 arno. 2010-09-02 09:56:23 PDT
Created attachment 66381 [details]
php testcase
Comment 2 arno. 2010-09-02 10:03:17 PDT
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 ?
Comment 3 arno. 2010-09-02 10:07:44 PDT
Created attachment 66385 [details]
patch v0.1

first patch proposal: removes access denied test
Comment 4 arno. 2010-09-02 10:10:31 PDT
Created attachment 66386 [details]
patch v0.2

second patch proposal: start FTPEntryType enum to 1
Comment 5 Eric Seidel (no email) 2010-12-03 13:23:54 PST
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.
Comment 6 Eric Seidel (no email) 2010-12-03 13:24:46 PST
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.
Comment 7 Adrien Destugues 2014-10-13 00:45:53 PDT
Created attachment 239713 [details]
Patch
Comment 8 Chris Dumez 2014-10-16 14:11:59 PDT
Comment on attachment 239713 [details]
Patch

Looks good, r=me.
Comment 9 Chris Dumez 2014-10-16 14:13:32 PDT
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?
Comment 10 Adrien Destugues 2014-10-16 14:24:42 PDT
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.
Comment 11 BJ Burg 2015-11-12 12:13:01 PST
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.
Comment 12 Pascal Abresch 2021-12-13 10:52:13 PST
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?
Comment 13 Adrien Destugues 2021-12-13 11:27:13 PST
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.
Comment 14 Pascal Abresch 2021-12-18 08:00:01 PST
Created attachment 447520 [details]
Patch
Comment 15 Pascal Abresch 2021-12-18 10:44:57 PST
Created attachment 447523 [details]
Patch
Comment 16 Pascal Abresch 2021-12-18 13:00:45 PST
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.
Comment 17 Pascal Abresch 2021-12-18 13:23:11 PST
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)
Comment 18 Pascal Abresch 2021-12-18 23:47:27 PST
Created attachment 447543 [details]
Patch
Comment 19 Adrien Destugues 2021-12-19 01:16:44 PST
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?
Comment 20 Pascal Abresch 2021-12-19 04:01:35 PST
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;
               }
             }
Comment 21 Carlos Alberto Lopez Perez 2021-12-20 15:54:38 PST
(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
Comment 22 Pascal Abresch 2022-02-20 01:48:39 PST
Created attachment 452688 [details]
Patch
Comment 23 Pascal Abresch 2022-02-20 01:49:55 PST
I've updated the patch to skip the GTK test and include the MacOS test as the expected result.
Comment 24 Pascal Abresch 2022-02-21 07:31:25 PST
Created attachment 452729 [details]
Patch
Comment 25 Pascal Abresch 2022-02-21 20:35:55 PST
The layout test now passes for MacOS, it still fails for iOS duo to the differinh font sizes. How should I proceed with this?
Comment 26 Carlos Alberto Lopez Perez 2022-02-26 08:30:47 PST
(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
Comment 27 Pascal Abresch 2022-02-26 12:11:45 PST
Created attachment 453305 [details]
Patch
Comment 28 Darin Adler 2022-02-26 13:31:36 PST
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).
Comment 29 Pascal Abresch 2022-02-26 13:41:53 PST
Created attachment 453307 [details]
Patch
Comment 30 Pascal Abresch 2022-02-26 13:48:54 PST
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!
Comment 31 Darin Adler 2022-02-26 13:49:28 PST
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.
Comment 32 Darin Adler 2022-02-26 13:50:49 PST
Comment on attachment 453307 [details]
Patch

Waiting for EWS to show passing results, then someone should set commit-queue+ too.
Comment 33 Pascal Abresch 2022-02-28 06:00:59 PST
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.
Comment 34 EWS 2022-02-28 06:51:29 PST
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].
Comment 35 Radar WebKit Bug Importer 2022-02-28 06:52:19 PST
<rdar://problem/89555741>