Closed Bug 134796 Opened 22 years ago Closed 15 years ago

"Save All" to root of drive (c:\) saves to wrong directory (to Working Folder/Current Directory)

Categories

(MailNews Core :: Attachments, defect)

x86
Windows 98
defect
Not set
major

Tracking

(Not tracked)

VERIFIED WORKSFORME

People

(Reporter: youying, Unassigned)

References

Details

Attachments

(5 obsolete files)

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.9+) Gecko/20020331
BuildID:    2002033109

After saving all attachments in MozillaMail via "Save All" feature,
the files are not saved to the specific directory.

Reproducible: Always
Steps to Reproduce:
1.Just save attachments via "Save All" in C:\
2.You will find those files are in Mozilla directory.


Actual Results:  Files should be saved in the specific directory.
Reporter, are you still seeing this in Mozilla 1.0?
So far...It's ok in 1.0 final.
Resolving worksforme as per comment.

Reporter, if this problem occurs again, please reopen this bug. Thank you!
Status: UNCONFIRMED → RESOLVED
Closed: 22 years ago
Resolution: --- → WORKSFORME
Hello,

Would you please tell me how to re-open ?
I notice that some bugs I filed had not been checked and reproduced.
I wonder if the QA contact was gone...:-Q
Now it occurs again in Mozill 1.1 Alpha (2002061104), not nighty.
This occurs in Mozilla 1.1 Alpha.
So I reopen this bug.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
Severity: normal → major
Keywords: mozilla1.1
Keywords: mail6
where ARE they saved ?
its normal that mozilla creates a directory with the site name and only leaves
the index.html in the folder you selected ...
Reporter: Can you take a look at Comment 7?
>>------- Additional Comment #7 From Michael Gabriel  2002-09-27 18:02 -------
>>where ARE they saved ?
>>its normal that mozilla creates a directory with the site name and only leaves
>>the index.html in the folder you selected ...

It was saved in x:\program files\mozilla.org\mozilla\ 

QA Contact: trix → yulian
QA Contact: yulian → stephend
confirmed with build 2003032104 WinXP
Status: UNCONFIRMED → NEW
Ever confirmed: true
Confirmed with 
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030615
*** Bug 173949 has been marked as a duplicate of this bug. ***
Summary: "Save All" did not save files to my specific directory → "Save All" to root of drive (c:\) saves to wrong directory
*** Bug 147495 has been marked as a duplicate of this bug. ***
*** Bug 194569 has been marked as a duplicate of this bug. ***
Still there in 1.5 final. It saves the files into the current directory on the
selected drive. This happens to be something like Comment 9 said, if no other
program changed the dir after Mozilla.
Same bug also in Thunderbird 0.4
You can save on root in all other drives except the drive where [Mozilla's] 
program directory is situated
still there in Mozilla 1.7a 20040208 winxp
saves to GRE base folder (C:\Program Files\Fichiers
communs\mozilla.org\GRE\1.7a_2004020808)

incredible !
(happilly, the download manager is able to locate it !)
It works in Thunderbird 0.4.

Based on duplicates, this bug appears to be a Windows-only bug that affects both
9x and NT.
Keywords: mozilla1.1
*** Bug 235231 has been marked as a duplicate of this bug. ***
reproduced on winxp 1.7b build 2004031508
Confirmed with 2004040608-trunk/Win-2K(SP4).

As aceman said in Comment #15, problem is probably described as follows ;
When specified drive is same as one for "Working Directry" in shortcut(=current
directry, usually Mozilla's program directry is set) and when root directry is
specified on save, file save is done to current directry.

This is same situation as following scenario.
(On starting of Mozilla)
 X:
 CD \Program_Directry_of_Mozilla
 mozilla.exe
(On saving file to X: from C:\TEMP)
 COPY C:\TEMP\FILE.EXT filename.ext => copied to current directry
or
 COPY C:\TEMP\FILE.EXT X:filename.ext => copied to current directry

This should be ; 
 COPY C:\TEMP\FILE.EXT X:\filename.ext
or
 X:
 COPY C:\TEMP\FILE.EXT \filename.ext
or
 X:
 CD \
 COPY C:\TEMP\FILE.EXT filename.ext
The problem is that nsLocalFile::InitWithNativePath("c:\")
<http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileWin.cpp#748> will
trim the trailing slash from the directory name to end up with "c:" because it
doesn't check to see if the final directory character is the 3rd character. This
bit needs to be wrapped like

if (pathLen > 3) { 
  // kill any ... 
}

However, once this is corrected (and any follow on bugs squashed), you then run
into the problem of nsLocalFile::GetParent()
<http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileWin.cpp#1757> also
trimming the trailing slash from "c:\" to get "c:". This bit also needs to be
wrapped like

if (parentPath.Length() > 3) { 
  // cannot ... 
}

However what is the desired result of calling GetParent() when there is no
parent? Return the same directory or give an error?

Attached patch proposed patch (obsolete) — Splinter Review
This is a patch which does what I just wrote in the comment. I've lightly
tested it on XP and it seems to do what it should (saves in c:\ or d:\,
displays c:\ or d:\ as the current directory next time Save all attachments is
shown). It also doesn't seem to break anything.
I found some problems with this patch. On vacation for a week and will update it
after that.
Attached file replacement test program for nsIFile (obsolete) —
This is a complete replacement for xpcom/tests/nsIFileTest.cpp which I needed
during testing of the new patch. There is no point in attaching a diff
(although I can) as this is completely rewritten. I have included all the old
tests as well as created a heap of new ones. The new test program doesn't need
any manual inspection of the results (it is completely automated).
Attached patch new patch (obsolete) — Splinter Review
This is the new proposed patch. It is for the latest current version of
nsLocalFileWin.cpp  Would someone please have a look through it and see what
you think of the proposed changes. Essentially I have added a helper function
which determines the type of path that the object is looking at because it
helps to simplify things.
Attachment #145725 - Attachment is obsolete: true
Note that the new nsIFileTest.cpp has only been tested on Windows. I've included
tests for Linux which test what I assume is desired functionality for Linux
(pretty much the same as Windows since nsIFile is an abstraction).

I don't have access to a linux box at the moment for testing, so I would
appreciate if someone else would compile and check it for me. 
Attachment #147788 - Flags: superreview?(dougt)
Attachment #147788 - Flags: review?(blizzard)
Comment on attachment 147789 [details] [diff] [review]
new patch

Apologies if you aren't the correct people to request reviews from. Let me know
if someone else would be preferred.
Attachment #147789 - Flags: superreview?(dougt)
Attachment #147789 - Flags: review?(blizzard)
Attachment #147789 - Flags: review?(blizzard) → review?(darin)
Attachment #147788 - Flags: review?(blizzard) → review?(darin)
Comment on attachment 147789 [details] [diff] [review]
new patch

You picked the right reviewers... I'll try to give this a close look sometime
this week.
Comment on attachment 147789 [details] [diff] [review]
new patch

>+typedef enum { 
>+    PT_INVALID, 
>+    PT_LOCAL_ROOT, 
>+    PT_LOCAL_LEAF, 
>+    PT_SHARE_ROOT, 
>+    PT_SHARE_LEAF 
>+} EPathType;

traditionally enums are eInterCaps

>+static EPathType
>+GetPathType(const nsACString & aFilePath, PRBool aValidate = PR_FALSE)
>+{
>+    // all valid paths have at least 3 characters (c:\)
		 ^^^ i know this is a windows file, but code duplication
happens, please consider clarifying "valid paths" as "valid windows paths" (yes
it's 1:30am and i should be sleeping).

>+        // windows paths don't have forward slashes

This restriction is actually fairly annoying and we might get rid of it at some
point...

>+    // determine if we are share or local

I'm not fond of 'we' in code. 'the path is local or a share' or something ...

>+    if (char1 >= 'A' && char1 <= 'Z' && char2 == ':' && char3 == '\\')
>+    {
>+        // local drive (e.g. c:\)
>+        return (++begin == end) ? PT_LOCAL_ROOT : PT_LOCAL_LEAF;
>+    }

in mozilla code, placing an |else| after a |return| is frowned upon.

>+    else

you don't need it because the only way to reach the code following the block
containing the return is if the if condition is false.

... if (char1 == '\\' && char2 == '\\' && char3 != '\\')
>+    {
>+        ++begin;
>+        if (!FindCharInReadable('\\', begin, end))
>+            return PT_SHARE_ROOT;

Can I have a multibyte computer name? I haven't checked the spec and it's too
hard for me to rename my computer (it's in a domain).

>+        return (++begin == end) ? PT_SHARE_ROOT : PT_SHARE_LEAF;
>+    }

else after return:

>+    else

>+    {
>+        return PT_INVALID;
>+    }
>+}

>@@ -731,44 +782,30 @@

general note, you might consider adding -p to your diff flags, it adds pretty
trails to the @@ line (not useful here, but worth noting).

> NS_IMETHODIMP
> nsLocalFile::InitWithNativePath(const nsACString &filePath)
> {
>+    // check path for validity and get the type (e.g. root or leaf, local or network share)
>+    EPathType type = GetPathType(filePath, PR_TRUE);
>+    if (type == PT_INVALID)
>         return NS_ERROR_FAILURE;
>+ 
>+    MakeDirty();
>+ 
>+    // this is a native path
>+    char * path = ToNewCString(filePath);
>+    if (!path)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    PRUint32 pathLen = filePath.Length();
>+ 
>+    // kill any trailing '\' provided it isn't the second char of DBCS 
>+    // and it isn't a local root

this code is the bit that should remind us to worry about multibyte earlier (it
sucks that I actually remembered this code when I read the earlier bit).

>+    if (type != PT_LOCAL_ROOT)
>+    {
>+        PRUint32 len = pathLen - 1;
>+        if (path[len] == '\\' && (!::IsDBCSLeadByte(path[len-1]) ||
>+                    _mbsrchr((const unsigned char *)path, '\\') == (const unsigned char *)path+len))
>+        {
>+            path[len] = '\0';
>+            pathLen = len;
>+        }

>@@ -1761,24 +1816,28 @@

this is where -p would be helpful

> {
>     NS_ENSURE_ARG_POINTER(aParent);
> 
>+    // we can't return the parent if we are at the root already

I think this will break the code which I worked so hard to add.

Without it, you can't find siblings of c:
Yes. please don't do that.
This includes tests for win32 drive enumeration and japanese locales.
Attachment #147788 - Attachment is obsolete: true
Attached patch patch version #3 (obsolete) — Splinter Review
This new patch addresses timeless's comments, fixes problems with MBCS support,
and fixes the problem with the drive enumerator. 

However in this version I have added the path type as a member variable to the
class, I'm sure this will not be desirable. However, it's basically a trade off
of either having the member var for speed, or calling the GetPathType function
all the time.
Attachment #147789 - Attachment is obsolete: true
Attachment #148403 - Flags: superreview?(dougt)
Attachment #148403 - Flags: review?(darin)
Attachment #147788 - Flags: superreview?(dougt)
Attachment #147788 - Flags: review?(darin)
Attachment #147789 - Flags: superreview?(dougt)
Attachment #147789 - Flags: review?(darin)
Comment on attachment 148404 [details] [diff] [review]
patch version #3

Sorry for the bug spam.
Attachment #148404 - Flags: superreview?(dougt)
Attachment #148404 - Flags: review?(darin)
Comment on attachment 148404 [details] [diff] [review]
patch version #3

Arg. My mozillas are all crashed, so i'm typing in this tiny ie window.

 nsLocalFile::OpenNSPRFileDesc(PRInt32 flags, PRInt32 mode, PRFileDesc
**_retval)
 {
+    // ensure we have a valid path for this action
+    if (mPathType == ePathInvalid || mPathType == ePathLocalEnumerator)
+	 return NS_ERROR_FILE_INVALID_PATH;

Can nspr really open "\\server"? I can't imagine it can.

A quick test app shows CreateFile("\\server",...) results in: Could not open
file (error 123), so i'd blacklist servers too.

 nsLocalFile::GetParent(nsIFile * *aParent)
 {
     NS_ENSURE_ARG_POINTER(aParent);

-    nsCAutoString parentPath(mWorkingPath);
+    // ensure that we have a valid path for this action
+    if (mPathType == ePathInvalid || mPathType == ePathLocalEnumerator)
+	 return NS_ERROR_FILE_UNRECOGNIZED_PATH;

you're changing behavior. parent of \\. must be null.

--

As for the rest of the changes in the middle. I want to be able to append to
\\., it's legal. If you want to special case it so that it does something
better than simply \\.\c:\ then we can do that..

Note that there are other things which are legal children of \\.,

See the documentation for CreateFile

\\.\TAPE0 for example, also there is actually a distinction between \\.\c: and
\\.\c:\ 

speaking of which, to accomodate that, the:
+    // if this isn't a local root drive (e.g. c:\) then kill any trailing
backslash
+    // (provided it isn't the second char of DBCS)

code in  nsLocalFile::InitWithNativePath should probably be friendly to the
distinction between them ...
Is this really desired behaviour for nsIFile/nsILocalFile? The way I understand
it, it is meant to be a cross-platform abstraction mechanism for file and
directory access. The sort of changes that you are wanting are supported by
Win32 only (and some by NT/2K/XP only). If you are writing Win32 only code,
wouldn't it be better for you to use CreateFile directly instead of sending it
through nsILocalFile?
context: i'd like to be able to do everything from js. I could write my own
win32 impl of nsIFile/nsILocalFile to expose extra bits of CreateFile, but
that'd be fairly silly. I may never add tape support, that'd be ok.

I'd like nsIFileTest to be writable using xpcshell js.

I do have plans to add a share enumerator and a server enumerator. But they're
not top priorities. Basically a xul app developer should be able to write a
complete app js and not have to care that the os is windows or unix. When I
actually finish w/ the computer enumerator, i'll probably join \\. with the
other servers and have the server enumerator be the parent of both \\. and all
other servers. (the server enumerator would have null as its parent.)

By complete app, I mean something where the app doesn't have arbitrary
limitations that make it feel broken to users of native apps on a given platform
(e.g. windows).
Comment on attachment 148404 [details] [diff] [review]
patch version #3

The patch for bug 243473 fixes this too.
Attachment #148404 - Attachment is obsolete: true
Attachment #148404 - Flags: superreview?(dougt)
Attachment #148404 - Flags: review?(darin)
Attachment #148403 - Attachment is obsolete: true
Attachment #148403 - Flags: superreview?(dougt)
Attachment #148403 - Flags: review?(darin)
*** Bug 222089 has been marked as a duplicate of this bug. ***
Might it be that this bug is resolved now (somone said so in Bug 222089)?
Thunderbird version 0.8 (20040913)

In a mail with attachements,
Save All...
Choose a directory where the file already exists...
When prompted to overwrite, click cancel...
It opens a requester for the new filename...

Problem:
The filename starts with "C-\" instead of "C:\".
This bug is not fixed.
 Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041019

With MailNews, the files are no longer stored in the Mozilla.exe program 
directory, but in
   C:\program files\common files\mozilla.org\GRE\(version).

In Thunderbird, they are stored in the tbird.exe program directory; TB does not 
have a separate GRE directory.
Yes, this bug is not fixed. The fix for bug 243473 was originally going to fix 
this, but it complicated that patch unduly and so I removed it. 

Fixing this bug isn't straight-forward as there are a number of dependencies 
on the internal representation of the path. The new nsIFile/nsILocalFile test 
harness at bug 247456 shows some of the problems with this.
This bug is still not fixed in Thunderbird version 0.9 (20041103)

(In reply to comment #40)
> Thunderbird version 0.8 (20040913)
> 
> In a mail with attachements,
> Save All...
> Choose a directory where the file already exists...
> When prompted to overwrite, click cancel...
> It opens a requester for the new filename...
> 
> Problem:
> The filename starts with "C-\" instead of "C:\".
Product: MailNews → Core
*** Bug 239890 has been marked as a duplicate of this bug. ***
*** Bug 265572 has been marked as a duplicate of this bug. ***
comment 40 / comment 43 are about bug 262866, not this bug.
Just confirming that this bug still exists in Thunderbird version 1.0+
(20050626) under Windows 2000 Professional. Saved attachments are placed in the
Thunderbird program directory instead of the selected "C:" location.
*** Bug 320910 has been marked as a duplicate of this bug. ***
Summary: "Save All" to root of drive (c:\) saves to wrong directory → "Save All" to root of drive (c:\) saves to wrong directory (to Working Folder/Current Directory)
Whoa, this bug already existed in 2002?

Anyway, I just experienced the bug myself for the first time. Tried saving 7 JPGs from an email that was in my Local Folders to C:\ (I usually never do that, but anyway, I did now) and couldn't find them there - they were in "C:\Program Files\Mozilla Thunderbird" instead. Exactly like Mike Stockman described in the previous message here,.. over two years ago :)

I'm using 2.0.0.6
Assignee: mscott → nobody
QA Contact: stephend → attachments
Product: Core → MailNews Core
Is this true on XP (sorry only have Vista who dislikes writing on c:\) ? can anyone confirm it is still an issue on recent thunderbird 3.0 releases (http://stage.mozillamessaging.com/en-US/thunderbird/early_releases/downloads/) ?
Reverified with current builds on Windows XP. While this is still reproducible for TB 2.0.0.22 (attachment saved to installation folder), the 20090715 nightly for TB 3.0b3pre saves correctly to the root of C:\ or any other drive selected. Also works for the SM 2.0b1pre nightly.

-> WFM (fixed on trunk, not a candidate to get fixed on the 1.8.1 branch)
Status: NEW → RESOLVED
Closed: 22 years ago15 years ago
Resolution: --- → WORKSFORME
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: