177635 - Compact all folders does not follow symlinks

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[David Grant]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0) Gecko/20020623 Debian/1.0.0-0.woody.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0) Gecko/20020623 Debian/1.0.0-0.woody.1

I have all my mail mbox files on my Windows drive on a FAT32 partition.  I use
Mozilla 1.0.1 for Windows and Debian Woody Mozilla 1.0 on Linux.  My Linux
.mozilla/.../Mail/pop.telus.net/ folder has tons of symlinks to all the
different mail files.  I didn't want to symlink the entire folder, for fear of
something bad happening later for whatever reason.  This system works great for
me.  But then I noticed that sometimes the Inbox and Sent folder would not have
their symlink anymore, but would instead be replaced with an identical local
version.  I think I figured out why.  I'm pretty sure it is the Compact All
Folders operation that is causing this.  And the reason that these two folders
are getting affected are because they are BIG and they need compacting
(according to Mozilla).  Sorry I haven't done enough testing on this, but I am
90% sure.  I thought it might be easier for someone to just quickly check the
code and confirm that Mozilla isn't following symlinks here.  In fact there are
other instances as well....the bookmarks.html file doesn't seem to like to be
symlinked either.

p.s. Is mbox the right terminology for the mail files?  I can't remember.

Reproducible: Always

Steps to Reproduce:
1. Create some symlinks to some mbox mail files in your mail directory to
targets outside that directory
2. Play around with it
3. Compact all folders

Actual Results:  
The symlink will be gone, and replaced with a generated local file

Expected Results:  
Mozilla should compact the symlinked file, not compact it, then create a local file

Comment 1

[David :Bienvenu]

-> Navin

Comment 2

[David Grant]

OK, in order to make your life easier, I tested this more fully than what I said
in my initial bug submission.  I am now confident 100% that this is happening
due to the compact all folders step.

Thank you.

Comment 3

[David Grant]

Would it be possible to put a target for this bug, or some keywords?  Just so
that there is at least some hope that it gets fixed at some point....even if it
misses the target the first time.

Comment 4

[David :Bienvenu]

when we compact a folder, we create a temp folder, copy the non-deleted messages
into the temp folder, then replace the original folder with the temp folder.
This way, we minimize the risk of losing any data from the original folder and I
don't think we're going to change that. I don't know if we have any way of
restoring the sym link, or if perhaps converting to nsIFile from nsFileSpec
might fix this.

Comment 5

[David Grant]

I don't understand.

You don't have to worry about "restoring" the symlink as you say, as long as you
just respect the existance of the symlink from the beginning.  When you first
start compacting folders, look at the actual destination.  Then change your path
string variable containing the mail folder from the local to the symlink (or
actual location).  Then do everything from the actual location.

So I have Inbox -> /mnt/win_c/Documents and Settings/David/Application
Data/Mozilla/Profiles/default/ntx5f644.slt/Mail/pop.telus.net/Inbox

Instead of of reading from ./Inbox and replacing back to ./Inbox, grab the
symlinked/actual location /mnt/win_c/.../Inbox, and read from there and copy back.

They were able to fix this for the bookmarks.html file symlinking very recently.
See bug: http://bugzilla.mozilla.org/show_bug.cgi?id=156814

Comment 6

[David :Bienvenu]

thx, I think we can do what the bookmarks code is doing. I guess instead of
restore I should have said "maintain"...

taking...

Comment 7

[David Grant]

Thanks a lot.

Comment 8

[David :Bienvenu]

Attachment: proposed fix

I think this should work - I can't really test it (other than seeing that it
doesn't break on windows) since I don't have linux handy. I'll try to find
someone with a linux build to apply this patch and test it.

Comment 9

[David :Bienvenu]

Dan, can I get a review? And maybe try it out on linux? Let me know if I should
find someone like Seth to try it out instead...thx!

Comment 10

[(not reading, please use [e-mail] instead)]

Comment on attachment 107791 [details] [diff] [review]
proposed fix

sr=sspitzer, after someone tests on linux.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 107791 [details] [diff] [review]
proposed fix


>-  m_fileSpec.Rename((const char*) idlName);
>+  m_fileSpec.Rename((const char*) leafName);

This seems like a good place for an NS_STATIC_CAST.

Otherwise, looks fine.

Tested, and it fixes the problem on Linux.

r=dmose

Comment 12

[(not reading, please use [e-mail] instead)]

>>+  m_fileSpec.Rename((const char*) leafName);
> This seems like a good place for an NS_STATIC_CAST.

Why would NS_STATIC_CAST be need?

actually, since leafName is an nsXPIDLCString, just do:

m_fileSpec.Rename(leafName.get());

or

m_fileSpec.Rename(leafName);

$1 says the cast to (const char *) is just old school string stuff.

or am I mis-understanding?

Comment 13

[David :Bienvenu]

Attachment: fix addressing comments

I'll just use .get(). Thanks for testing this, Dan!

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Sure, .get() would work also.  My point was just that a C++ cast more explicitly
expresses what's being done here than a C-style cast does, and a C++-style cast
gives the compiler a better chance of catching an error should this (or the
surrounding) code change at some point in the future.

Comment 15

[David :Bienvenu]

understood, thanks. I think not having the cast at all is best. My bad for
copying the bookmarks code that has a needless cast.

Comment 16

[David :Bienvenu]

fix checked in.