56779 - Filesystem Datasource doesn't show full directory contents

Status: VERIFIED FIXED

(Core Graveyard :: RDF, defect, P2)

Description

[Brian Ryner (not reading)]

If the filesystem datasource encounters a dangling symlink while getting a
directory's contents, it stops processing the directory altogether, and files
appearing after that are just skipped.

This is causing several users problems with the filepicker, because there may be
dangling symlinks in system directories which they have no control over.

I have a patch which silently discards the bad file and moves on, instead of
terminating.

Comment 1

[Brian Ryner (not reading)]

Nominating for rtm based on the aforementioned symptoms in the filepicker.  The
fix (which I'm attaching next) is extremely safe (my only potential concern
would be a memleak, but I don't think there is one).

Comment 2

[Brian Ryner (not reading)]

Attachment: patch to silently skip the dangling symlinks

Comment 3

[Robert John Churchill]

The REAL problem here is that nsFile et.al. should NOT be trying to resolve 
symlinks.  The bogus symlink should still be shown (and NOT resolved) instead of 
being skipped.  The RDF file system datasource actually indicates that the item 
should not be resolve but nsFile does anyway... that's the real bug, and what 
should be fixed.  Giving this bug over to dougt...

Comment 4

[R.K.Aa.]

*** Bug 56760 has been marked as a duplicate of this bug. ***

Comment 5

[Robert John Churchill]

bryner: If you really need a quick fix for 6.0 RTM, a slightly better fix would 
be to just completely ignore any errors from the IsDirectory() method call... 
and perhaps add a comment as to the reason why errors are being ignored.  (Its a 
better fix as it would get the broken symlink to appear as well.)

Comment 6

[Brian Ryner (not reading)]

back to rjc so we can try to get a fix for 6.0 in, in the datasource.

Comment 7

[Brian Ryner (not reading)]

actually, over to me, i have a patch...

Comment 8

[Brian Ryner (not reading)]

Attachment: patch #2 (show the dangling symlink)

Comment 9

[Brian Ryner (not reading)]

ok, per conversation with rjc, this latest patch goes ahead and shows the
dangling symlink (it doesn't do anything with the error code from IsDirectory).

Comment 10

[Robert John Churchill]

Patch #2 looks good to me, r=rjc

Comment 11

[Peter Trudelle]

Problems affecting only a few users won't make it into N6 at this point.  What
are the circumstances under which this occurs?  Expected frequency?

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This is not an uncommon occurance, and bryner is right: it sometimes happens
where users have no control over the symlink in question.

I'd be surprised if you found a well-used Unix system that didn't have a few
dangling symlinks -- especially in users' home directories, which is where
they're most likely to be for attaching files, etc.

Comment 13

[R.K.Aa.]

Every *nix user is bound to come across tbis bug. The use of symlinks is
extensive on those OS'es. See bug 56760 for a real live sample; users can't play
realplayer as a helper app from it's default location. (And they sure can't use
it as a plugin, in most cases)

What's worse: How can i trust Mozilla not to overwrite files, when it doesn't
even realize files preexist? No alert would appear. Unknowing users risk
destroying files they otherwise would have made backups of. Now THAT is
admittedly a scenario not likely to happen too often, but it will happen. Doing
anything with files under *nix would be like working blindfolded, if this isn't
fixed. Fairly insecure.

Comment 14

[Peter Trudelle]

rtm need info, not a top crash, but sounds like a top failure of the app, with a
trivial bullet-proofing fix.

Comment 15

[Jason Eager]

Adding dataloss keyword to cover the situation where a user accidently
overwrites one of his/her own files because s/he didn't know it existed.

Comment 16

[Jason Eager]

We have an r=, we need an sr=

Comment 17

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I'm working on a fix that feels better: make nsLocalFileUnix honour the
.followLinks setting, like the other platforms.

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: Patch to make nsLocalFileUnix honour .followLinks

Comment 19

[Christopher Blizzard (:blizzard)]

r=blizzard

Comment 20

[Brendan Eich [:brendan]]

a=[e-mail].

/be

Comment 21

[Jason Eager]

is a r=,a= enough for a rtm+?

Comment 22

[Brendan Eich [:brendan]]

I use a= as I did when waterson and I were the only "super-reviewers", and only
for non-netscape.com contributors.  It's the same as sr= in my book, although
for some reason, despite years of r= usage and months of a= usage, people think
a= means something else.

This change should get checked into the trunk now that it has passed review. 

/be

Comment 23

[Phil Peterson]

Marking [rtm need info]. Does shaver's patch have the same effect as the
previous ones? Should rjc or bryner review? It's not clear to me that everyone
is in agreement; please mark [rtm+] again when The Right Thing is clear.

Comment 24

[Robert John Churchill]

shaver's patch helps out Unix regarding symlink resolution;  Windows and Mac 
still need the same luv'in...

Comment 25

[Brian Ryner (not reading)]

shaver's patch will have the same net effect in this particular case, but it
does a much better job of fixing the problem at the source.

I'll add my r= to the list here, the patch looks fine to me.

Marking [rtm+] for reconsideration.

Comment 26

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Oh!  I had thought, based on the Platform setting and the continual use of
``symlink'' that this was an nsLocalFileUnix-only problem.

I don't know enough about Windows and Mac code here to really say how to fix
them.  Maybe the right thing, in light of that, is to take the
datasource-specific fix.

Comment 27

[Phil Peterson]

Ok, so if we're going back to bryner's second patch, that was reviewed by rjc
but we still need a super reviewer, and I'd like evidence that it actually works
on Windows, Mac and Linux.

Marking [need info] again. Sorry if that's a pain, but you wouldn't believe the
problems I've caused by asking questions but leaving the bug [rtm+]

Comment 28

[Peter "jag" Annema]

The right thing to do for the trunk is to fix .followLinks on all platforms.
Using the quick hack for the branch is fine with me, but don't plaster this over
on the trunk.

Comment 29

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

sr=shaver on bryner's patch #2: it's a good, minimal fix to the datasource.

Comment 30

[Brian Ryner (not reading)]

back to rtm+ since we have an sr=

Comment 31

[Jason Eager]

Changing status whiteboard to reflect status.

PDT, please approve this change. If this fix doesn't make it in, I will
be unable to use NS6 as my daily browser, because displaying all
the filenames in a directory is fundimental, there should never be a case
when an application does not show all of the files in a directory.

Furthermore, This bug causes a whole bunch of other bugs, like
the "realplayer plugin problem".

Comment 32

[selmer (gone)]

rtm++ for "patch #2"

Comment 33

[Brian Ryner (not reading)]

checked in on branch and trunk.  we should file a new bug for making followLinks
work.

Comment 34

[Tom Everingham]

verified on branch
Linux rh6 1024 build

needs verified on trunk

Comment 35

[lchiang]

This may have spawned bug http://bugzilla.mozilla.org/show_bug.cgi?id=57905.

Comment 36

[Tom Everingham]

verified trunk:
Linux rh6 2000123106

Comment 37

[Keyser Sose]

*** Bug 63719 has been marked as a duplicate of this bug. ***