183871 - Crash on "Show hidden files and directories" [@ nsFileView::SetDirectory(nsIFile*) ]

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Max Alekseyev]

Linux build 2002120508

To reproduce:
1. Open Edit->Preferences...->Advanced->Cache->Choose Folder
2. Click on "Show hidden files and directories" checkbox
3. Mozilla crashes

Comment 1

[Max Alekseyev]

That happens when "Choose Cache Folder" dialog says:
"You do not have the permissions necessary to view this directory".

Comment 2

[Olivier Cahagne]

can you post Talkback ID or stack trace ?

Comment 3

[Max Alekseyev]

How to get them?

Comment 4

[Andrew Schultz]

are you using a .mozilla.org build?  this bug has been reported multiple times,
all by users of debian builds except for one running RPMS from Red Hat.  see bug
158036

Comment 5

[Olivier Cahagne]

Max, for the Talkback part, see "mozilla/bin/components/talkback/talkback" give
you actually submitted Feedback when the Netscape Feedback agent popped up after
Mozilla crashed.

Comment 6

[Max Alekseyev]

yeah. i'm using mozilla.org build
ftp://ftp.mozilla.org/pub/mozilla/nightly/latest/mozilla-i686-pc-linux-gnu.tar.gz

I don't see talkback subdirectory in mozilla/conponents.

Comment 7

[Max Alekseyev]

The bug is easily reproducible here. Just let me know which build/tool to use to
provide you necessary information.

Comment 8

[Andrew Schultz]

talkback is not included in the .tar.gz builds.  You need to get the "sea" or
"installer" build.

Comment 9

[Max Alekseyev]

Attachment: Talkback info

Comment 10

[Andrew Schultz]

we just need the talkback ID.  run mozilla/components/talkback/talkback and grab
the talkback ID for the report you sent in.

Comment 11

[Max Alekseyev]

I didn't get how to learn ID.
I click 'Send' in Talkback agent, it disappears, and nothing happen.

Comment 12

[Andrew Schultz]

> I click 'Send' in Talkback agent, it disappears, and nothing happen.

that's fine.  now go to the mozilla directory (i.e. /usr/local/mozilla) and then
go to components/talkback.  run the talkback executable in that directory.

in an attempt to know why you're seeing this when nobody else is... what Mozilla
theme are you using?  what gtk theme are you using?

Comment 13

[Max Alekseyev]

Ok. Talkback ID: TB14817508W

I see the bug with any theme including default Modern one.
I'm using Debian release 3.0 - dunno what gtk it comes with.

Comment 14

[Olivier Cahagne]

we need a new TB ID, this one has expired (30 days).

Comment 15

[Max Alekseyev]

Ok. This is a fresh one.
Linux build 2003010610

Comment 16

[[e-mail] (gone - use [e-mail] instead)]

Trigger Reason  	SIGSEGV: Segmentation Fault: (signal 11)

Comment 17

[Andrew Schultz]

bug 176886 strikes again.

Comment 18

[Max Alekseyev]

Olivier, do you want a new stack again?
From which build?

Comment 19

[Olivier Cahagne]

Max, we'd need a stack from GDB with a debug build you'd compile yourself. I
can't reproduce this on my Linux box and Talkback sometimes totally misses the
stack.

Comment 20

[Andrew Schultz]

you could also try running with a clean profile.  if there's something in your
profile that's triggering the crash, we'd need to know what it is to reproduce
the crash.

Comment 21

[Max Alekseyev]

Clean profile works fine. And I've found what causes the crash.

I have two primary OS installed: OS/2 and Linux. And I'm trying to share Mozilla
settings between them that is objected by the bug 137006. So I have to use some
tricks to pass over that bug (at least partially). Mozilla settings are located
on OS/2 drive O: which is mounted as /O: directory in Linux.
Well... The current crash problem arises when I use profile from OS/2 version
containing line like

user_pref("browser.cache.disk.parent_directory",
"O:\\Home\\Mozilla\\Profiles\\relf\\lq1ln66p.slt");

Hope, this will help.

Comment 22

[Andrew Schultz]

thanks, Max.  I can reproduce it with that setting.

marking NEW.  it's an invalid pref setting, but Mozilla shouldn't crash.
stacktrace to follow.

Comment 23

[Andrew Schultz]

Attachment: stacktrace

(gdb) frame 7
#7  0x41ff39d3 in nsFileView::SetDirectory(nsIFile*) (this=0x8878488,
aDirectory=0x1) at nsFileView.cpp:244
244	  aDirectory->GetDirectoryEntries(getter_AddRefs(dirEntries));
(gdb) p aDirectory
$1 = (nsIFile *) 0x1
(gdb) p *aDirectory
Cannot access memory at address 0x1
(gdb) frame 8
#8  0x41ff37c2 in nsFileView::SetShowHiddenFiles(int) (this=0x8878488,
aShowHidden=1076661060)
    at nsFileView.cpp:158
158	    SetDirectory(mDirectoryPath);
(gdb) p mDirectoryPath
$2 = {mRawPtr = 0x0}

Comment 24

[Boris Zbarsky [:bzbarsky]]

to bryner, since he's most familiar with this code....

Comment 25

[Max Alekseyev]

BTW, i disagree that pref is invalid. In Linux it can be treated as a valid
directory whose name contains '\' and ':' symbols. Nothing wrong with that.

Comment 26

[Andrew Schultz]

this kind of this is supposed to be handled (see bug 105809).

http://lxr.mozilla.org/mozilla/source/xpfe/components/filepicker/res/content/filepicker.js#162
162   if (!directory || !(sfile.exists() && sfile.isDirectory())) {
163     // Start in the user's home directory
164     sfile.initWithPath(homeDir.path);
165   }

sfile.exists() returns false, but it still checks sfile.isDirectory(), which
throws an unhandled Javascript exception, so the directory never gets reset.

Comment 27

[Andrew Schultz]

Attachment: patch

just check exists() and isDirectory() separately.

Comment 28

[Boris Zbarsky [:bzbarsky]]

umm.... if sfile.exists() returns false (are you sure that it does?) then it
checking sfile.directory() there is a blatant bug in JS, which is defined to do
operator short-circuiting....

Are you sure that's what's going on?

Comment 29

[Andrew Schultz]

Comment on attachment 111325 [details] [diff] [review]
patch

it appears that the exception was from some debugging I had, but the patch did
fix things.  However, backing out the patch now also works.  I'm confused.

Comment 30

[Andrew Schultz]

Comment on attachment 111325 [details] [diff] [review]
patch

ok, to reproduce this bug, you have to actually visit a page so that it creates
the bogus cache directory (relative to the current directory).	If the
directory does not exist, then the filepicker behaves as advertised and falls
back to the home directory.

Comment 31

[Andrew Schultz]

ok, the real problem is that filepicker's populateAncestorList() accesses
directory.parent, which returns NS_ERROR_FILE_INVALID_PATH because the path does
not contain a '/':
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsLocalFileUnix.cpp#1176
GetParent seems to expect an absolute path, so a path without a '/' is
inherently invalid.

try/catching the access to directory.parent and the while loop seems to fix
things up a bit.  The directory is properly displayed in the filepicker and
turning on hidden files doesn't crash, but (of course) the "Look in" dropdown is
unpopulated and hitting "Up" doesn't do anything.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Ugh.  So yes, getComplexValue from prefs will call setPersistentDescriptor with
whatever garbage is in the pref.  And nsLocalFileUnix will cheerfully let itself
be initialized (through InitWithNativePath()) with a path that has nothing to do
with a Unix path... even though it later totally fails to deal (eg see assertion
in GetParent, the logic in AppendRelativeNativePath, etc, etc).

Adding some exception catching code to the filepicker is fine with me, but only
if .parent is allowed to throw.  Too bad we didn't bother documenting that in
nsIFile...

Should InitWithNativePath throw when the path is obviously bogus?

Comment 33

[Pete Collins (MDG)]

Initializing nsLocalFileUnix w/ a bogus value sounds like bad client logic. If
nsLocalFileUnix is crashing, that is a different story.

I tried to reproduce on debian, but no crash.

--pete

Comment 34

[Boris Zbarsky [:bzbarsky]]

Um... the client (prefs) initializes an nsILocalFile.  It does not know what the
underlying impl is and it should not care, since it makes no sense to put the
validity checks in _all_ the clients when those would be much more cleanly
encapsulated in the nsILocalFile implementations.

More to the point, nsLocalFileWin, nsLocalFileMac, nsLocalFileOS2,
nsLocalFileOSX,  all perform sanity-checking on the path and will return
NS_ERROR_UNRECOGNIZED_PATH if the path is not recognized (makes sense, eh?). 
Yeah, every single client of nsILocalFile could include the
couple-of-hundred-lines of ifdeffed error checking... do you really think that
is the right approach?

No, nsLocalFileUnix does not crash.  It will simply throw exceptions that the
interface does not define as being thrown and behave in completely ridiculous
ways in other cases....  It seems to me, that it would make more sense to throw
an exception where callers are actually expecting one and prepared to deal...

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

Would fixing this be as easy as adding a check for file existance in
InitWithNativePath?

Comment 36

[Boris Zbarsky [:bzbarsky]]

You can created nsIFiles pointing to non-existing places and then call Create()
or CreateUnique() on them, you know...  Breaking that would be BAD (frozen
interface, yadda yadda).  ;)

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

how about this, can we make the unix localfile implementation throw an exception
if Init is called with a relative path?

Comment 38

[Doug Turner (:dougt)]

~/ should still be supported.

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: require absolute paths or "~/" in InitWithPath on unix

well... unfortunately this patch breaks xpcshell, which tries to init nsIFiles
with "."

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

well the real problem was that nsDirectoryService.cpp was relying on the fact
that nsLocalFileUnix::InitWithNativePath accepted relative paths. That
contradicted its interface documentation, but oh well...

Comment 41

[Christian :Biesinger (don't email me, ping me on IRC)]

taking

Comment 42

[Christian :Biesinger (don't email me, ping me on IRC)]

really taking this time

Comment 43

[Doug Turner (:dougt)]

Comment on attachment 137198 [details] [diff] [review]
patch

Is there a nsDirectoryService.cpp patch?

Why can't the value of PR_GetEnv(...) be a canonicalized absolute path and drop
this realpath business?  

I think that your change to xpcom/io/nsLocalFileUnix.cpp may break things. 
Yes, the interface says one thing but as you pointed out, the callers do
another.  What kind of testing have you done?

Comment 44

[Christian :Biesinger (don't email me, ping me on IRC)]

>Is there a nsDirectoryService.cpp patch?

yes, in the exact attachment you commented on

>Why can't the value of PR_GetEnv(...) be a canonicalized absolute path and drop
>this realpath business? 

let me note that GRE_GetCurrentProcessDirectory already uses the realpath solution.

>I think that your change to xpcom/io/nsLocalFileUnix.cpp may break things. 

Well, nsLocalFileUnix seems to not always deal with that either - GetLeafName
seems to require absolute paths. Also GetParent.

>What kind of testing have you done?

run xpcshell, run mozilla, do some browsing, run mailnews, attach patches (using
the filepicker).
do you have anything specific in mind that you suspect of breaking?

Comment 45

[Doug Turner (:dougt)]

Comment on attachment 137198 [details] [diff] [review]
patch

lets get this into 1.7a.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 137198 [details] [diff] [review]
patch

sr=bzbarsky

Comment 47

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, checked in
Checking in nsDirectoryService.cpp;
/cvsroot/mozilla/xpcom/io/nsDirectoryService.cpp,v  <--  nsDirectoryService.cpp
new revision: 1.67; previous revision: 1.66
done
Checking in nsLocalFileUnix.cpp;
/cvsroot/mozilla/xpcom/io/nsLocalFileUnix.cpp,v  <--  nsLocalFileUnix.cpp
new revision: 1.109; previous revision: 1.108
done

Comment 48

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #43)
> I think that your change to xpcom/io/nsLocalFileUnix.cpp may break things.

Just for the record, dougt was right and this change did cause bug 238290.