192124 - Filing more than one bookmark in a newly created folder cause bookmarks not to be saved

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect, P1)

Description

[Rija MENAGE]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3b) Gecko/20030205
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3b) Gecko/20030205

Filing more than one bookmark in a newly created folder cause bookmarks (all or
all except the first one) not to be saved after quiting (normally, without
crashing) the application.

If the folder has been created in previous session, adding several bookmarks in
that folder during the same session is OK.
It' s also OK when adding bookmark directly in the "root" without creating folder.



Reproducible: Always

Steps to Reproduce:
1. add a bookmark with "File bookmark..."
2. Create a new folder in the dialog, select it then valid
3. within the same session file other bookmarks in the same folder
4. Quit Mozilla
5. Relaunch Mozilla

Actual Results:  
Most time: Only the first filed bookmark (the one for wich we created the
folder) is present in the folder

Sometimes: the folder is here but there's no bookmark at all inside

Expected Results:  
the folder should contain all filed bookmarks

bug reproduced on 2 computers:
IBook 600 with MacOSX 10.2.3
Dual G4/450 with MacOSX 10.2.3
(I'm on HFS+ filesystem and the Journaling is OFF)

with the MachO version of Mozilla :
2003013103
2003020303
2003020503

My bookmarks.html is big (216k) but the problem occured on a brand new profile too.

The sites I tried to add are not already in my bookmarks and the name for the
folder are different from existing ones and contains no spaces ( I used dummy
names like 'test', 'demo', etc for testing)

The problem also occured when bookmarks (except the first one) are dragged from
the url bar to the folder into the Bookmark view of the sidebar 

And it happens when the application is quitting correctly

Comment 1

[guanxi]

worksforme Moz 1.3a 2002121215 / Win2K

Comment 2

[Greg K.]

Probably a dup of bug 192011.

Comment 3

[Rija MENAGE]

There are chances that the problem is the same as for 192011.

But in this case the description of 192011 does not reflect the data loss aspect

192011 is about moving existing bookmarks not reliable

This is about bookmarks not saved

Comment 4

[Bjarne D Mathiesen]

Mozilla/5.0   Gecko/20030210
(Macintosh; U; PPC Mac OS X Mach-O; da-DK; rv:1.3b; MultiZilla v1.1.33 (a)) 

I'm seeing this too, but also in the following case:
when creating more than one sub-folder only the first one created is retained on
restart - all others are lost

Question is, if this bug and bug 192011 ought to be merged ?
In both cases the behavior is essentially the same : only the first action
performed on the bookmarks is registered - all subsequent changes whether these
are bookmarking a site, moving a bookmark or folder, creating folders etc are
lost - in some cases leading to dataloss

Comment 5

[Larry]

It's quite a bad problem ... continuing in 1.3b (Mach-O).
I had resorted to Exporting my bookmarks at some point,
so now I tried going into bookmarks Manage .. importing
the file, and deleting the previous folders (so that only
the imported contents would be there).  Quit and restart,
these edits weren't retained either.  Thanks; Larry.

Comment 6

[Larry]

I just started a new bookmarks.html file (using vi, so I'm
sure it's a new inode and default file modes, etc. ...
reading in the stuff that I had exported at some point).
Then (after starting and exiting Moz) I started using the
testcase specified in this bug description -- my build is
2003021017 i.e. 1.3b Mach-O.  It's a good testcase, works
exactly as Rija documented above.  (Just want to affirm
that it's still "there" and this testcase is good;)  Larry.

Comment 7

[Asa Dotzler [:asa]]

We should get these OSX bookmarks bugs fixed for final. Samir, can someone on
your team take this? 

Comment 8

[Conrad Carlen (not reading bugmail)]

This does happen for me using this simple test case. The behavior is slightly
different between my debug trunk build and an opt nightly:
1. With an opt build, after step 3, bookmarks after the 1st are not visible in
the bookmarks menu.
2. With a debug build, after step 3, added bookmarks are visible in the menu.

In both cases, the bookmarks are gone after quitting and relaunching. Reason I
bring this up is this is the 2nd time I've seen a difference WRT bookmark
persistence between opt and debug.

Comment 9

[Samir Gehani]

Nominating nsbeta1.

Comment 10

[Samir Gehani]

-> Jan

Comment 11

[Magnus]

I have experienced a similar problem, also 1.3b on MacOS X. I've twice (before
reading this report) created new bookmark folders and moved bookmarks around --
both times the changes are not saved. The second time I exported the bookmarks
and substitued the bookmarks file so as not to lose my work. Bookmarks that I've
created otherwise, without creating a new folder, have been saved correctly
during seprate sessions in recent days.

Comment 12

[Jeremy Stock]

I have seen something similar but not quite the same if I'm reading right. I can
no longer save new bookmarks in the root (no folder) location either. They show
up but are not saved on exit. I was able to add one by exporting and re
importing my bookmarks but all other attempts now fail. Also any attempt to
modify existing doesn't work either.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3b) Gecko/20030210

Comment 13

[Jan Varga [:janv]]

Conrad, I suspect it's a problem with nsLocalFileOSX::Clone() method

nsBookmarksService::Flush() ->
  NS_GetSpecialDirectory(BOOKMARKS_FILE) ->
    nsDirectoryService->Get() ->
      cachedFile->Clone()

NS_GetSpecialDirectory seems to return NS_OK, but when we call GetNativePath()
on the cloned file it returns NS_ERROR_FILE_NOT_FOUND.

I added a debug printf in nsLocalFile::Clone, mFSRef is always the same for
bookmarks.html.
It starts to fail after multiple (actually 2) calls to ::FSRefMakePath(&mFSRef, ...)

I also tried to modify Clone() method to use default copy constructor or
InitWithFile(), doesn't work either :(

Any ideas ?

Comment 14

[Conrad Carlen (not reading bugmail)]

> It starts to fail after multiple (actually 2) calls to
::FSRefMakePath(&mFSRef, ...)

Calling FSRefMakePath more than once on the very same FSRef fails?

Comment 15

[Jan Varga [:janv]]

>Calling FSRefMakePath more than once on the very same FSRef fails?
yeah

http://developer.apple.com/techpubs/macosx/Carbon/Files/FileManager/File_Manager/DataTypes/FSRef.html#//apple_ref/c/tag/FSRef
"The client of the File Manager cannot examine the contents of an  FSRef  to
extract information about the parent directory or the object’s name. Similarly,
an  FSRef  cannot be constructed directly by the client; the  FSRef  must be
constructed and returned via the File Manager. There is no need to call the File
Manager to dispose an  FSRef ."

Does it mean that we can't just copy mFSRef in the copy ctor ?

Comment 16

[Samir Gehani]

Nav triage team: nsbeta1+/adt1

Comment 17

[Conrad Carlen (not reading bugmail)]

> Does it mean that we can't just copy mFSRef in the copy ctor ?

I sure hope not. I'll ask on CarbonIT.

Comment 18

[Jan Varga [:janv]]

Never mind, it seems that WriteBookmarks() is doing something strange. I don't
see the problem when I comment out WriteBookmarks().
I'll try to debug it more tomorrow.

Comment 19

[Jan Varga [:janv]]

Actually, it creates a temp file first and writes to that file.
Then it deletes the old file and renames temp file to the original name.
And that proably causes the problem.

Comment 20

[Jan Varga [:janv]]

Yeah, I hacked the bookmarks service to not use a temp file and I don't see the
problem anymore.

Comment 21

[Jan Varga [:janv]]

Maybe we should you FSSpec instead of FSRef ?

Comment 22

[Conrad Carlen (not reading bugmail)]

Whew! From what you said before, I was really afraid so I wrote this test program:

#include <Carbon/Carbon.h>
#include <assert.h>

int main (int argc, const char * argv[]) {
    OSErr err;
    FSRef fsRef1, fsRef2;
    UInt8 pathBuf[512];
    
    err = FSFindFolder(kUserDomain,
                       kDomainTopLevelFolderType,
                       kDontCreateFolder,
                       &fsRef1); 
    assert(err == noErr);
    err = FSRefMakePath(&fsRef1, pathBuf, sizeof(pathBuf));
    assert(err == noErr);
    printf("Path #1 of 1st FSRef is: %s\n", (char *)pathBuf);
    err = FSRefMakePath(&fsRef1, pathBuf, sizeof(pathBuf));
    assert(err == noErr);
    printf("Path #2 of 1st FSRef is: %s\n", (char *)pathBuf);
    
    fsRef2 = fsRef1;
    err = FSRefMakePath(&fsRef2, pathBuf, sizeof(pathBuf));
    assert(err == noErr);
    printf("Path #1 of 2nd FSRef is: %s\n", (char *)pathBuf);
    err = FSRefMakePath(&fsRef2, pathBuf, sizeof(pathBuf));
    assert(err == noErr);
    printf("Path #2 of 2nd FSRef is: %s\n", (char *)pathBuf);
    
    return 0;
}

and it worked.

Comment 23

[Conrad Carlen (not reading bugmail)]

> Maybe we should you FSSpec instead of FSRef ?

No way. FSSpec isn't Unicode safe and it's limited to short file names. Takes us
back to the bad old days ;-)

Comment 24

[Conrad Carlen (not reading bugmail)]

> Then it deletes the old file and renames temp file to the original name.
And that proably causes the problem.

This is probably where the bug lies in nsLocalFileOSX.cpp.

Comment 25

[Conrad Carlen (not reading bugmail)]

Attachment: Icky patch that works

The problem with an FSRef is that, if it refers to an existing file, and that
file is deleted, that FSRef will be invalid - even if another file of the same
name and location replaces the deleted file. So, using an FSRef to specify a
file which may be deleted is no good (and pretty much means that the OSX file
impl has to be rewritten to use CFURL instead). In the meanwhile, this patch
makes directory service return a fresh copy of the bookmarks file each time it
is requested. Then, we don't end up with a dangling, invalid FSRef.

Comment 26

[Jan Varga [:janv]]

That sounds reasonable, but it will probably decrease performance.
Is there any other (long term) solution ?

Comment 27

[Jan Varga [:janv]]

> (and pretty much means that the OSX file impl has to be rewritten to use CFURL
instead)

ah, so that's answer for my previous question, never mind

Comment 28

[Jan Varga [:janv]]

please file a new bug about rewritting OSX file impl, thanks

Comment 29

[Peter Van der Beken [:peterv]]

Comment on attachment 114855 [details] [diff] [review]
Icky patch that works

Ugh :-/.

Comment 30

[Conrad Carlen (not reading bugmail)]

See bug 164396.

Comment 31

[Asa Dotzler [:asa]]

Comment on attachment 114855 [details] [diff] [review]
Icky patch that works

a=asa (on behalf of drivers) for checkin to 1.3

Comment 32

[Jan Varga [:janv]]

Conrad, can I check in for you ?

Comment 33

[Conrad Carlen (not reading bugmail)]

Yes. Thanks.

Comment 34

[Jan Varga [:janv]]

checked in

Comment 35

[Chris Casciano]

*** Bug 194191 has been marked as a duplicate of this bug. ***

Comment 36

[Chris Casciano]

*** Bug 194490 has been marked as a duplicate of this bug. ***

Comment 37

[Chris Casciano]

*** Bug 194516 has been marked as a duplicate of this bug. ***

Comment 38

[Chris Casciano]

*** Bug 194719 has been marked as a duplicate of this bug. ***

Comment 39

[R.K.Aa.]

*** Bug 194851 has been marked as a duplicate of this bug. ***

Comment 40

[Boris Zbarsky [:bzbarsky]]

*** Bug 194902 has been marked as a duplicate of this bug. ***

Comment 41

[R.K.Aa.]

*** Bug 195310 has been marked as a duplicate of this bug. ***

Comment 42

[Chris Casciano]

*** Bug 195838 has been marked as a duplicate of this bug. ***

Comment 43

[Chris Casciano]

*** Bug 195973 has been marked as a duplicate of this bug. ***

Comment 44

[Chris Lyon]

*** Bug 196639 has been marked as a duplicate of this bug. ***

Comment 45

[Chris Petersen]

Verified in the 2003-04-22-08 Macho trunk build.