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

RESOLVED FIXED in mozilla1.7alpha

Status

Core Graveyard
File Handling
--
critical
RESOLVED FIXED
15 years ago
a year ago

People

(Reporter: Max Alekseyev, Assigned: Biesinger)

Tracking

({crash})

Trunk
mozilla1.7alpha
x86
Linux
crash

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

15 years ago
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
(Reporter)

Comment 1

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

Comment 2

15 years ago
can you post Talkback ID or stack trace ?
Keywords: crash, stackwanted
(Reporter)

Comment 3

15 years ago
How to get them?

Comment 4

15 years ago
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

15 years ago
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.
(Reporter)

Comment 6

15 years ago
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.
(Reporter)

Comment 7

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

Comment 8

15 years ago
talkback is not included in the .tar.gz builds.  You need to get the "sea" or
"installer" build.
(Reporter)

Comment 9

15 years ago
Created attachment 108541 [details]
Talkback info

Comment 10

15 years ago
we just need the talkback ID.  run mozilla/components/talkback/talkback and grab
the talkback ID for the report you sent in.
(Reporter)

Comment 11

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

Comment 12

15 years ago
> 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?
(Reporter)

Comment 13

15 years ago
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.
(Reporter)

Updated

15 years ago
Attachment #108541 - Attachment is obsolete: true

Updated

15 years ago
Whiteboard: TB14817508W

Updated

15 years ago
Blocks: 185584

Updated

15 years ago
No longer blocks: 185584

Comment 14

15 years ago
we need a new TB ID, this one has expired (30 days).
Whiteboard: TB14817508W
(Reporter)

Comment 15

15 years ago
Ok. This is a fresh one.
Linux build 2003010610
Keywords: stackwanted
Whiteboard: TB15870027Y
Trigger Reason  	SIGSEGV: Segmentation Fault: (signal 11)

Updated

15 years ago
Keywords: stackwanted
Whiteboard: TB15870027Y

Comment 17

15 years ago
bug 176886 strikes again.
(Reporter)

Comment 18

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

Comment 19

15 years ago
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

15 years ago
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.
(Reporter)

Comment 21

15 years ago
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

15 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: stackwanted

Comment 23

15 years ago
Created attachment 111318 [details]
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}
to bryner, since he's most familiar with this code....
Assignee: law → bryner
(Reporter)

Comment 25

15 years ago
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

15 years ago
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

15 years ago
Created attachment 111325 [details] [diff] [review]
patch

just check exists() and isDirectory() separately.
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?

Updated

15 years ago
Attachment #111325 - Flags: review?(bryner)

Comment 29

15 years ago
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.
Attachment #111325 - Flags: review?(bryner)

Comment 30

15 years ago
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.
Attachment #111325 - Attachment is obsolete: true

Comment 31

15 years ago
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.
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

15 years ago
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
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...
Would fixing this be as easy as adding a check for file existance in
InitWithNativePath?
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).  ;)
how about this, can we make the unix localfile implementation throw an exception
if Init is called with a relative path?

Comment 38

15 years ago
~/ should still be supported.

Updated

14 years ago
Blocks: 219107

Updated

14 years ago
Summary: Crash on "Show hidden files and directories" → Crash on "Show hidden files and directories" [@ nsFileView::SetDirectory(nsIFile*) ]
Created attachment 137189 [details] [diff] [review]
require absolute paths or "~/" in InitWithPath on unix

well... unfortunately this patch breaks xpcshell, which tries to init nsIFiles
with "."
Created attachment 137198 [details] [diff] [review]
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...
Attachment #137189 - Attachment is obsolete: true
taking
Assignee: bryner → file-handling
QA Contact: chrispetersen → ian
Target Milestone: --- → mozilla1.7alpha
Attachment #137198 - Flags: review?(dougt)
really taking this time
Assignee: file-handling → cbiesinger
Status: NEW → ASSIGNED

Comment 43

14 years ago
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?
>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

14 years ago
Comment on attachment 137198 [details] [diff] [review]
patch

lets get this into 1.7a.
Attachment #137198 - Flags: review?(dougt) → review+
Attachment #137198 - Flags: superreview?(bz-vacation)
Comment on attachment 137198 [details] [diff] [review]
patch

sr=bzbarsky
Attachment #137198 - Flags: superreview?(bz-vacation) → superreview+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(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.
Crash Signature: [@ nsFileView::SetDirectory(nsIFile*) ]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.