103726 - FTP directory listings not reflecting file changes on reload

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Charlie Whitman Jr.]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.4) Gecko/20010913
BuildID:    2001091303

With the FTP directory listings showing in HTML mode, hitting the reload button
will not reflect changes in the directory structure. I have tried both the Win32
version and the Linux version, and they both exhibit this behavior. Even closing
and re-opening the browser will not necessarily make changes show up;
apparently, the directory must first expire from the cache, which doesn't take
long for FTP files.

Reproducible: Always
Steps to Reproduce:
1. With Mozilla, view an FTP directory which you have the ability (by whatever
means) to add to or subtract from.
2. Add a file to (or take one away from) the directory that you have displayed
in Mozilla.
3. Hit reload on the browser. The changes will not be reflected.

Actual Results:  New files will not be diplayed. Deleted files will still be
present.

Expected Results:  It should have re-queried the FTP server, showing a new
directory listing.

This did not happen in the 0.9.2 version of Mozilla, but only since I upgraded
to 0.9.4 (I skipped over 0.9.3). In fact, 0.9.2 reloaded the directory from the
server every time you changed levels. Perhaps this was too much, but it would be
preferable to not reloading the directory even when specifically ordered to do so.

Comment 1

[Bradley Baetz (:bbaetz)]

The cache is 15 minutes, IIRC.

Unlike http, theres no easy way to do a conditional request. Shift-reloading
should force a reload.

Perhaps a normal reload should always refetch, too. darin?

Comment 2

[Darin Fisher]

yes, i think a normal reload should hit the server.  that would be consistent
with LOAD_NORMAL|VALIDATE_ALWAYS.  since ftp can't validate, it must refetch.

Comment 3

[Bradley Baetz (:bbaetz)]

OK, then.

->0.9.6, and confirming

Comment 4

[Bradley Baetz (:bbaetz)]

I'm going to attach a patch, which sets us to not read from the cache for
VALIDATE_ALWAYS | LOAD_BYPASS_CACHE

darin: Is that all I need to test? I'm not sure how ftp should handle some of
the const in nsIRequest.idl.

Comment 5

[Bradley Baetz (:bbaetz)]

Attachment: patch

Comment 6

[Darin Fisher]

for ftp, i think the following makes sense:

LOAD_FROM_CACHE                       : always use cache entry, even if expired
LOAD_BYPASS_CACHE                     : overwrite cache entry
LOAD_NORMAL|VALIDATE_ALWAYS           : overwrite cache entry
LOAD_NORMAL                           : honor expiration time
LOAD_NORMAL|VALIDATE_ONCE_PER_SESSION : overwrite cache entry if first access 
                                        this session, otherwise use cache entry 
                                        even if expired.
LOAD_NORMAL|VALIDATE_NEVER            : always use cache entry, even if expired

Comment 7

[Bradley Baetz (:bbaetz)]

"overwrite cache entry if first access this session, otherwise use cache entry
even if expired." How do I do this?

The http code has the comment for this flag:

            // If the cached response does not include expiration infor-
            // mation, then we must validate the response, despite whether
            // or not this is the first access this session.  This behavior
            // is consistent with existing browsers and is generally expected
            // by web authors.

which doesn't help ftp.

Comment 8

[Darin Fisher]

currently, ftp makes up an expiration time, i thought.  so, if you like my
interpretation of the load flags, then you could call
nsICacheSession::SetDoomEntriesIfExpired(PR_FALSE), and compare the expiration
time to the current time yourself.  this flag is by default set to true.

Comment 9

[Bradley Baetz (:bbaetz)]

Out of time, -> 0.9.7

Comment 10

[Bradley Baetz (:bbaetz)]

Missed 0.9.7, ->0.9.9

Comment 11

[Syd Logan]

only nsbeta1+ bugs can have milestones, resetting to ---

Comment 12

[Bradley Baetz (:bbaetz)]

Attachment: real patch

Oops, forgot to diff+attach this earlier

Comment 13

[Darin Fisher]

Comment on attachment 69334 [details] [diff] [review]
real patch


>+PRUint32 nsFtpState::mSessionStartTime = NowInSeconds();

do we need to worry about any problems related to initialization
of static variables?  not that i can think of a case where it
would be a problem, but it seems to be a pattern that we avoid.

one good alternative is to put a getter for this on FTP protocol
handler class.

sr=darin

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 69334 [details] [diff] [review]
real patch

what darin said.

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 69334 [details] [diff] [review]
real patch

No, POD types are OK to init at a file level

Restoring sr flag cause of uncaught midair.

Comment 16

[Darin Fisher]

if you know for certain that none of our platforms will have trouble with this
(eg. you've seen existing code in the tree that follows this pattern), then i'm
ok with it.

Comment 17

[Bradley Baetz (:bbaetz)]

Yeah, and I checked on #mozilla too.

Checked in.

Comment 18

[benc]

VERIFIED: 1.0 RC1, Win98
.

Comment 19

[benc]

fixing state