225055 - symlinked bookmark files are broken AGAIN

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The fix for bug 200498 knowingly broke symlinked bookmark files. This is
something that has worked on Unix for a while (once it got unbroken after the
first time it had gotten broken).  It can continue working if some use is made
of the nsIFile API here (the isSymlink() function and the functions to resolve
the symlink).

What we have now constitutes dataloss as far as users are concerned, since they
save bookmarks and then can't find them (when multiple profiles share a single
bookmark file, which is by far the most common reason to have it be a symlink).

Comment 1

[Boris Zbarsky [:bzbarsky]]

By the way, I agree that the design of nsIFile makes this unnecessarily painful.
 A moveToFollowingLinks or something would have helped a hell of a lot here.

Comment 2

[Boris Zbarsky [:bzbarsky]]

OK, this is pretty nasty dataloss.  Could we please get some traction here? 
Especially since the other patch went in on explicit condition that this will
NOT be left just to rot?

A simple .isSymlink()/getNativeTarget() loop should do it, no?

Comment 3

[Henrik Lynggaard Hansen]

Suggest blocking 1.7a for this dataloss regression

Comment 4

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Does this matter for Firebird as well? I'd like to symlink the bookmarks between
the root account on my laptop and my user account, but I saw the original bug a
long time ago and decided not to.

Comment 5

[Boris Zbarsky [:bzbarsky]]

The firebird bug is bug 206567.  This bug has nothing to do with firebird.

Comment 6

[chris hofmann]

can someone try and get a patch together for this?  it would be good to get
fixed before 1.7b

Comment 7

[Darin Fisher]

> What we have now constitutes dataloss as far as users are concerned, since they
> save bookmarks and then can't find them (when multiple profiles share a single
> bookmark file, which is by far the most common reason to have it be a symlink).

use hard links as a work around?

Comment 8

[Boris Zbarsky [:bzbarsky]]

You can't hard link a bookmarks file on your Windows partition.... (which is the
  most common reason I've encountered for having multiple profiles in the first
place -- they happen to be under different OSes and our profiles save enough
OS-specific crud that actually sharing the profile is not feasible).

Comment 9

[Henrik Lynggaard Hansen]

I have also tested hardlinks. It appears they are killed as well. mainly because
we do

* Write to a backup file.
* delete the orignal
* copy over the backup 

or something similar.

On windows only NTFS supports hardlinks.

Comment 10

[Brendan Eich [:brendan]]

Is comment 9 right?  I thought we used rename(2) or equivalent to atomicly
replace the bookmarks file.  We should *never* delete the file or overwrite it
in place.

/be

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Is comment 9 right?

In some cases, yes.

1) On Unix if /tmp and the actual bookmarks file are on different filesystems
(rather common, since /tmp is often on its own filesystem) the rename() will
naturally fail and we will fall back on opening the bookmarks file via PR_Open
with PR_TRUNCATE and copying the data over.  This shouldn't clobber hard links,
I should think.  But a rename() actually _would_ clobber hard links, I believe.
 Fun, eh?  ;)

2) On Windows, it looks like we do the following ($dest is the filename of the
destination file, $tmp is the name of the temporary file):

A) Create $dest.moztmp
B) Call MoveFile($dest, $dest.moztmp);  (this is a Windows API function that
   well... moves the file).
C) Call MoveFile($temp, $dest);
D) If Step C failed, MoveFile($dest.moztmp, $dest)

I have no idea how MoveFile behaves wrt what Windows has in the way of hard links.

Comment 12

[Brendan Eich [:brendan]]

bz: if by clobber hard links you mean unlink the hard link (directory entry),
then yes: rename will unlink, open will truncate the hardlinked inode.

SOP for Unix says to rename from the same directory, if possible -- don't assume
/tmp is on the same fs as the file being updated.  We should do that.  For some
reason I thought we did, last time Ben and I looked at the bookmarks service
code (both tines of the fork).

varga: are you going to work on this for 1.7b (next week)?  If not, we need
someone to step up now.

/be

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK, looks like the /tmp thing I was talking about is a non-issue now -- we put
the temp file in the same dir as the bookmarks file.  So hard links won't work,
since rename() will simply unlink the hard link instead of overwriting the file
it points to, just like it'll unlink the soft link (what this bug is about).

The right thing to do would seem to be to follow the soft link, if any, before
we create the temp file, so that we preserve the "temp file and actual bookmarks
file are in the same directory" invariant.

Comment 14

[Brendan Eich [:brendan]]

Reminder to fix this in the Firefox tine of the fork, too.  File a separate bug
you say?  Cc'ing Ben.

/be

Comment 15

[timeless]

Attachment: fix symlinked bookmark files again

Comment 16

[timeless]

Attachment: Testcase

Comment 17

[Benjamin Smedberg]

Comment on attachment 143269 [details] [diff] [review]
fix symlinked bookmark files again

>     rv = tempFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, /*octal*/ 0600);
>     NS_ENSURE_SUCCESS(rv, rv);

1) If the tempfile doesn't exist, createUnique won't return a random filename,
it will return the actual file. Then your ->Equals check below will fail. Can't
we just change the ->Equals to return NS_OK if the tempfile is the same as
bookmarksfile.

>+        // If we wrote to the file successfully (i.e. if the disk wasn't full) 
>+        // then move the temp file to the bookmarks file so it takes its place. 
>+        PRBool equals;
>+        rv = tempFile->Equals(aBookmarksFile, &equals);
>+        if (NS_FAILED(rv))
>+            break;
>+        NS_ENSURE_TRUE(!equals, NS_ERROR_FAILURE);

>+    PRUint32 oldPermissions = 0600;
>+    rv = aBookmarksFile->GetPermissions(&oldPermissions);
>+    if (NS_FAILED(rv))
>+        oldPermissions = 0600;

>+    aBookmarksFile->SetPermissions(oldPermissions);

2) move the permissions-copying up to the CreateUnique call, unless there is an
issue I'm missing with that.

Comment 18

[timeless]

Attachment: handle new files

Comment 19

[Boris Zbarsky [:bzbarsky]]

Can you attach diff -w so I can tell what exactly got changed in that big block
you reindented?

Comment 20

[timeless]

Attachment: 143285 -w

it's nowhere near as useful as you'd think, because i also renamed variables to
a local version which i modified. I could have just assigned the local comptr
to the argument variable, but that'd eventually confuse someone...

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143291 [details] [diff] [review]
143285 -w

It's actually reviewable unlike the other....

>Index: nsBookmarksService.cpp
>+    rv = bookmarksFile->Normalize();

Add a comment as to why this Normalize() call is needed (because Unix ignores
the followSymlinks thing, yadda, yadda).

>     rv = NS_NewLocalFileOutputStream(getter_AddRefs(out),
>                                      tempFile,
>                                      PR_WRONLY,
>                                      /*octal*/ 0600,
>                                      0);
>-    if (NS_FAILED(rv))

Um... You do want to check rv here.  And break on failure.

>     strm->Close();
>     out->Close();

The rv of those should be checked too (I know the old code didn't do it,
but...) since some data can actually attempt to be writte on close (eg over
NSF), according to brendan.

With that, sr=bzbarsky

Comment 22

[timeless]

mozilla/xpfe/components/bookmarks/src/nsBookmarksService.cpp 	1.309
mozilla/xpfe/components/bookmarks/src/nsBookmarksService.cpp 	1.311

firebird already has a bug, it's in the dependency list.

Comment 23

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Since I don't use symlinked bookmark files (the Bookmarks FTP extension takes
care of such needs), removing self from CC list.