95906 - Browser reorders bookmarks randomly

Status: VERIFIED FIXED

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

Description

[Aleksey Nogin]

Something weird is going on in recent version of Mozilla (Build ID 2001081716
from RH7 RPMs on RedHat 7.1) that didn't exist two days ago (2001081607):

- The bookmarks in sidebar are displayed in some "random" order (which is
*different* from the order in the bookmarks.html).
- "Manage bookmarks" window shows the same order.
- Moving bookmarks around to change the order back to desired one causes
bookmarks to be reorded in some new way (not the one I asked), or often not to
be reordered at all.
- Personal Toolbar shows bookmarks in bookmarks.html
- Changing bookmark view to sorted and then back to unsorted does not change
anything.

Comment 1

[Aleksey Nogin]

This just started happening => regression.
I lost my bookmark order (That I was pretty fond of) => dataloss.

BTW, forgot to add - this order thing is happening both to subfolders and to
individual bookmarks in every folder.

Comment 2

[Michael Bergbauer]

Almost the same happened to me, after importing bookmarks to 2001081715 on a new
machine, the bookmarks were ordered upside down (each folder!), and when trying
to reorder them, all I could do was to insert the items at the first position of
each folder. When I put the elsewhere, the items were put at this position.

Comment 3

[Jarrod Gray]

I don't think there is any dataloss here because when I quit Mozilla my
bookmarks.html file was in the correct order. Of course starting it up again
showed the bookmarks in reverse order but it doesn't seem to be affecting the
file in any way.

Comment 4

[Aleksey Nogin]

But if user responds to this problem by trying to reorder bookmarks properly (as
I did), this produces weird unexpected results and kills the order even in the file.

Comment 5

[Paul Chen]

nav triage team:

Marking nsbeta1+, p2, and mozilla0.9.4. Ben, can you see what's going on here?

Comment 6

[Jarrod Gray]

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

Comment 7

[James Green]

Uh it's got nothing to do with importing bookmarks, since I've been suffering
this too, and I've been running daily builds all through (that is, maintaining
the same ~/.mozilla profile throughout), no profile migration has taken place.

I think I first saw this over the weekend, and a day or two later looked at
bonsai and saw that blake had checked something in touching bookmarks, can't
remember what though. CC him just in case.

Comment 8

[Chase Tingley]

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

Comment 9

[Chase Tingley]

This latest dup was on Solaris.  

Comment 10

[Robert Kaiser]

It seems to me that at every restart, items in personal toolbars (as well as
other bookmark folder's entries) are cycled one position to the right (not sure
if this is always true).
Also, if bookmarks are moved within their folder via drag&drop, the moved
bookmark is always placed on top of the folder, not at the position you dragged
it to.

Comment 11

[Gervase Markham [:gerv]]

I'm seeing this too. 

Asa: critical for 0.9.4? There are going to be a lot of annoyed people out there
if Mozilla rearranges their carefully-sorted bookmarks...

Gerv

Comment 12

[Robert Kaiser]

It already has 0.9.4 Milestone set, but I'm adding keyword so that it gets on
radars, this is one of the two most annoying bugs I'm seeing currently (beneath
bug 96911), and I'd never recommend an application to anyone showing those two
problems.

It seems to me now that it does not reorder everything at every start (or is it
at exit? no, looks more like it was at launch), and it's not shifting the whole
bookmarks menu, it's taking some entry, sometimes from the middle of the menu /
personal toolbar and reorders that one to the top/start, shifting all entries
between there and the original position one step to the bottom/right.

So that's the same behavior as with drag and drop shifting around entires -
something gets reordered to the top when it shouldn't.

Comment 13

[Peter Trudelle]

I don't see any evidence of dataloss here, or any reason for critical severity.
 Please either document why, or change these.  In the meantime, we should treat
this as 'normal' severity.

Comment 14

[Claudius Gayle]

Severity:
Critical - crashes, loss of data, severe memory leak.
Major - major loss of function.

I concur, let's not be flippant with the definition of dataloss. setting
severity back to normal, and removing dataloss keyword.

Comment 15

[Aleksey Nogin]

See my comment from 2001-08-18 06:36 - I consider bookmark order to be an 
important piece of [meta]data and this bug causes it to be lost. 

Comment 16

[Jens Heunemann]

my observations so far (build 2001082021)

the bookmarks are displayed in exact reverse order of the bookmark-file.
if I move a bookmark, its placed at the bottom of the corresponding folder in
the bookmarkfile, and therefor at the top of the folder in the browser

stopping the browser and restarting (several times) didn't alter the bookmark-file.

Seems to me that we have 2 bugs here, one is the display in reverse order, and
the second one is that bookmarks are placed at the bottom of the folder its put
into (in the bookmark file).
btw I didn't try to use the sorting-facility yet, bookmarks are still "unsorted"

Comment 17

[Yoshiki Yazawa]

check in for bug#46925 causes this bug.

Comment 18

[Yoshiki Yazawa]

Attachment: simple work around patch.

Comment 19

[Blake Ross]

--> ftang

Comment 20

[Peter Toneby]

If the bug is fixed but that last patch, then good, but a small note that might
be of interest: If I run the same nightly (I got it this morning) and display at
home on Linux the bookmarks are all wrong, I'm not sure if they are reverse
ordered or not, but it seems likely, but if I display to an AIX 4.3.3 (from the
same Linux-binary as before), the bookmarks are sorted correctly. The sorting is
correct when displayed on Solaris 8 too.

Comment 21

[Chase Tingley]

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

Comment 22

[Frank Tang]

The last patch is not good. I will look at it. We cannot reproduce on Mac nor
Window , right ? could be a bug in the nsICollator unix implementation. 

Comment 23

[Frank Tang]

bstell -please work on this one thanks.

Comment 24

[Gervase Markham [:gerv]]

I'm getting misordered bookmarks on Linux, but it's not reverse order.

Gerv

Comment 25

[Jaime Rodriguez, Jr.]

agreed PDT+ per PDT.

Comment 26

[kill this account]

humm... it seems to sort correctly for me on a current build on my Redhat 7.1
system but perhaps I do not understand the problem.

Comment 27

[kill this account]

I am currently unable to reproduce this. My bookmarks sort correctly, my mail 
lists sort correctly.

I went on to #mozilla and several people and I got people saying they had seen
it but I could not get anyone that currently saw it.

Comment 28

[kill this account]

from email: robert somerville <[e-mail]>
>
> it is still happening on my SOLARIS sparc builds of this AM, and i would 
> assume also on your automated builds. All bookmarks & folders are reverse 
> sorted (i swear)

Comment 29

[kill this account]

Until I can reproduce it I doubt I can debug it.

Comment 30

[Jeremy Sanders]

I can reproduce this with the build:
http://ftp.mozilla.org/pub/mozilla/nightly/2001-09-06-08-trunk/mozilla-i686-pc-linux-gnu.tar.gz

on Redhat Linux 7.1

It appears all my bookmark folders are in reverse order, and their contents are
the actual bookmarks in reverse order, followed by any seperators, then by the
folders inside the folder in reverse order. I'll post a screenshot to
demonstrate, if you like.

Do you want a copy of my bookmarks file?

Comment 31

[Jeremy Sanders]

Attachment: Image of bookmarks with new build

Comment 32

[Jeremy Sanders]

Attachment: Image of bookmarks with mozilla 0.9.3 (which works)

Comment 33

[Aleksey Nogin]

> it seems to sort correctly for me on a current build on my Redhat 7.1
> system but perhaps I do not understand the problem.
>
Possibly - the problem is not about how the bookmarks are sorted, it is about
how they are displayed in *unsorted* mode. They used to be displayed (correcty)
in the same order they are in the bookmarks file, but now in unsorted mode they
are displayed in some random order and also reordering them by drag&drop is not
working correctly.

Comment 34

[kill this account]

I manually drag-n-dropped them to change their order and it seems to work
for me. I then tried using the menus to change the sort order and that
worked for me.

Comment 35

[kill this account]

Attachment: png image of bookmarks image

Comment 36

[kill this account]

System: Redhat 7.1, build Sept 7, 2001.
I made a new profile.
I created a bookmark folder "New Folder".
I addded bookmarks by dragging the "bookmark ribbon" from the URL bar on to the 
    "New Folder" button.
I bookmarked a couple of mozilla pages.
I went to google and started surfing.
The bookmarks are in the order I dropped them (first at top).
I restarted and did a screen capture.

Comment 37

[kill this account]

Somehow this got marked fixed. Re-opening

Comment 38

[kill this account]

Attachment: patch; add a fallback sort of collation service not available

Comment 39

[kill this account]

If the collation service is not working Frank's change fallsback to no sorting.
In attachment 48666 [details] [diff] [review] I added a machine order fallback so if collation is not
available at least ascii will sort.


Can I get several people who are seeing the problem to try out my patch?

Thanks

Note: There are tabs in my patched lines. I do not like tabs but the surounding 
code had them so I followed suit.

Comment 40

[Jeremy Sanders]

Attachment: My bookmarks file

Comment 41

[Yoshiki Yazawa]

>bstell

Unfortunately, your patch does not fix this bug. I think the matter is
not existence of collation service but implementation of CompareString().

Comment 42

[Jeremy Sanders]

I'm confused. I've just tried building the same build as I had problems with,
and the ordering is much better! The folders are in the correct order, but the
seperators in the folders are at the end, and some of the folders in the folders
are in the wrong order.

Comment 43

[kill this account]

Yoshiki: thanks for testing my patch.

Does anyone who can reproduce this have any suggestions?

Comment 44

[D Wollmann]

Brian,

I wish I had more time to be helpful, but searching for a gig is taking all my time.

I've been seeing this problem since the build where it was first reported here.
Today I tried (with Linux build 2001090711) starting with a new profile and
adding new bookmarks. I see the same problems with a new profile, namely:

The first time bookmarks are listed, all the separators are stacked up at the
end of the list. Delete all separators from the bookmarks and the default
separators appear where they're supposed to be, but aren't visible in "manage
bookmarks".

I added new folders to the personal toolbar, but could not get them to stick
where I placed them. I added new bookmarks to the new folders in the personal
toolbar, and they would not stay in the order in which I placed them (view is
unsorted).

One bookmark I added to one of the new folders was missing after I closed
mozilla and restarted it.
For what it's worth, with my old profile, all the bookmarks seem to load in
reversed order. With the new profile, the order seems closer to random.

If you have specific things you'd like me to try, or if there's information I
can supply with regard to my system configuration, libs, etc. just let me know.

Comment 45

[Gervase Markham [:gerv]]

If it is the case that the Bookmarks sort being "Unsorted" should make the
bookmarks window and menus display the bookmarks in the order they are in in the
bookmarks file, then bstell can come up to my cube and see an installation where
this is not the case :-)

Gerv

Comment 46

[kill this account]

Okay, works on my linux system but does fail on my solaris build.

Comment 47

[kill this account]

Attachment: patch; fix strxfrm xfer len and add ascii fallback

Comment 48

[kill this account]

The problem is that the code that calls strxfrm in 
nsCollationUnix::CreateRawSortKey passes in strlen as the src buf length but 
the result is not defined if theoutput space required is >= src buf length 
(ie: do not transfer the whole str). It appears that some libraries transfer 
the chars but not the null terminator whereas other libs put in a null 
terminator but not the last char.

This did not show up when we used collation in the past since sorted items 
almost always differ before their last char.

For "unsorted" entries we actually sort them on their list postion.
"Unsorted" bookmarks sorted correctly in the past since we did not use a
collation sort, instead we used a machine order sort (just compare the bytes).

The machine order sort, of course, did not work for non-ascii sorted bookmarks.

When the collation was added for bookmark sorting the sorted bookmarks work
because they differ before the last char. 

The unsorted items lose their last char. The result is that the sort key for 
the 6th item ("000006") is converted to "00000" which equals the sort key for 
the 7th item ("000007") "00000". 

So items 0-9 get mixed together, items 10-19 get mixed together, ...

Comment 49

[kill this account]

Can I get several people who are seeing the problem to try out my new patch?

Thanks

Comment 50

[Yoshiki Yazawa]

the last patch (id=48989) works finely. thank you.

Comment 51

[robert somerville]

latest patch solves my solaris sparc problems, thanks
-robert somerville

Comment 52

[kill this account]

Can I get someone to r= this?

nhotta, ftang, yokoyama are all out of the office right now and I would
like to get this in to the trunk and branch soon.

Comment 53

[kill this account]

Attachment: patch; limit strxfrm to passed in buffer lenght

Comment 54

[kill this account]

After rereading the man pages I see I should have used the "passed in buffer
length" as the limit for strxfrm.

Can I get several people who have seen the problem to try out my newest patch?

Thanks

Comment 55

[robert somerville]

so far, latest patch "09/11/01 10:41" works fine on solaris sparc

Comment 56

[André Dahlqvist]

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

Comment 57

[Jeremy Sanders]

New patch (49027) seems to work for me on linux (though I can't reproduce
/exactly/ the same problem with my own build, without the patch, and the
standard builds).

Comment 58

[jag (Peter Annema)]

Comment on attachment 49027 [details] [diff] [review]
patch; limit strxfrm to passed in buffer lenght

r=jag

Comment 59

[D Wollmann]

patch 49027 seems to have addressed all the problems I was experiencing.

Comment 60

[Brendan Eich [:brendan]]

bstell, looks good.  I can only pick nits and make a performance improvement
suggestion:

+      if (len >= *outLen) {
+
res = NS_ERROR_FAILURE;
+
len = -1;
+      }

I think the two statements in the "then" block are indented with tabs, but
nsCollationUnix.cpp's Emacs modeline says indent-tabs-mode: nil, so please space
fill these tabs.

 if(collationService)

			{

				nsAutoString v1(uni1);

				nsAutoString v2(uni2);
-
				collationService->CompareString(
+
				rv = collationService->CompareString(

					kCollationCaseInSensitive,

					v1,v2,&sortOrder);
+
			}

Here, indent-tabs-mode: t means tabs are allowed ("When in Rome", and we are in
rjc's version of Rome :-).

However (and I realize you didn't change these lines), I think we can avoid the
copies to nsAutoStrings.  How about this instead (the ! lines are the ones I
changed)?

 if(collationService)

			{
!
				nsDependentString v1(uni1);
!
				nsDependentString v2(uni2);
-
				collationService->CompareString(
+
				rv = collationService->CompareString(

					kCollationCaseInSensitive,
!
					v1.get(),v2.get(),&sortOrder);
+
			}

/be

Comment 61

[Brendan Eich [:brendan]]

Oops, ignore the nsDependentString zero-copy idea -- nsICollation::CompareString
takes const nsString&, darnit.

I'm checking bstell's patch (with tab elimination in nsCollationUnix.cpp) into
the 0.9.4 branch.  Thanks, bstell, for fixing this.

/be

Comment 62

[kill this account]

Brendan: do we check the same thing into the trunk?

Comment 63

[Brendan Eich [:brendan]]

jag has kindly offered to spiff up the string-fu in the patch and in
nsICollation::CompareStrings.  I think he'll come up with a trunk patch soon, if
it reviews and tests well, bstell, could you r= it?  I'll then sr= it and we can
declare victory.

/be

Comment 64

[Michael M]

I just download the latest nightly, and things appear worse. Moving bookmarks
around still chucks them in random locations, and using the scroll bar just
fills the sidebar with garbage.
Please disregards this if the before mentioned patch is not yet in the nightlies.

Comment 65

[kill this account]

It is fixed in the branch but not yet in the trunk.
Jag and Brendan want the trunk fix to use the new string classes.

Comment 66

[kill this account]

Moving target to 0.9.5.
The branch was patched yesterday.

Comment 67

[Jim Robinson]

Does this mean that when 0.9.4 comes out it will *not* have the fix? If so, I wouldimagine that a lot of linux users will give 0.9.4 a pass. 

Comment 68

[Claudius Gayle]

Jim, no, quite the opposite. The 'branch' refers to the 0.9.4 branch ie the
actual bits that will be released as 0.9.4. Look at the status whiteboard and
keywords...

Comment 69

[kill this account]

We put the "old-style" (ie: minimal changes) patch in the branch.

We want to revise the string handling in this part of the code to use
the "new and improved" string handling, hence the delay for the trunk.

Comment 70

[Jim Robinson]

Sorry, I obviously don't understand how the system works. For my own
understanding, could someone explain what "Target Milestone: mozilla0.9.5" 
means, since it doesn't mean that the fix ends up in 0.9.5?

Comment 71

[kill this account]

0.9.5 will be fixed but not with the patch in attachment 49027 [details] [diff] [review].

Comment 72

[kill this account]

Jag: based on Brendan's comments of 2001-09-12 13:57 I've re-assigned the
bug to you. If this is a problem, kindly let me know.

I will be glad to be a reviewer.

thanks

Comment 73

[kill this account]

added ftang and nhotta

(nhotta: I know that you are a Mac developer not a Unix developer and only did 
the Unix collation as an i18n courtesy but I wanted to let you know that I was
fiddling with it.)

Comment 74

[jag (Peter Annema)]

Yeah, I'll see what I can do, this kinda ended up below my radar (sorry
Brendan).

Comment 75

[jag (Peter Annema)]

Attachment: Partial patch (Unix only) to show where I'm going with this. Maybe IDL-ize nsICollation.

Comment 76

[Jaime Rodriguez, Jr.]

Adding correctness Status Whiteboard, correct/expected behavior does not occur.

Comment 77

[Frank Tang]

Comment on attachment 49886 [details] [diff] [review]
Partial patch (Unix only) to show where I'm going with this. Maybe IDL-ize nsICollation.

looks ok on Unix

Comment 78

[Frank Tang]

Please be careful that nsICollation is implmented on Win, Mac, and Linux
idfferently. (and also OS/2, probably). this mean we need to change all three
(or four) implementation at once. 

Comment 79

[jag (Peter Annema)]

Updating status board. There is already a patch for this on the 0.9.4 branch, we
just want cleaner code on the trunk.

Jaime: what's the best way to signal PDT that this was already fixed on the
branch? Remove the nsbranch+ / PDT+, instead of my whiteboard comment?

Frank: Yeah, I'm aware of that, hence the "Unix only" in the latest attachment
comment ;-)

Comment 80

[Jaime Rodriguez, Jr.]

Removing 0.9.4 --> 0.9.5 

Removing nsbranch+ and PDT+ ... already checked into the 0.9.4 branch.

Comment 81

[Jeremy Sanders]

This bug is in the nightly build Gecko/20010924 for Linux. Is this because no
patch has been applied to the tree yet?

Comment 82

[jag (Peter Annema)]

Yes, I (or someone else) will need to finish this patch and get it checked into
the trunk.

Comment 83

[jag (Peter Annema)]

-> 0.9.6

Comment 84

[kill this account]

We cannot have this bug get out to a "release".

This needs to be fixed in 0.9.5 even if it is the 0.9.4 patch.

Comment 85

[jag (Peter Annema)]

Alternatively you just check in the 0.9.4 fix and file a bug on me to clean up
the code.

Brendan, would that be okay with you?

Comment 86

[Asa Dotzler [:asa]]

let's get the 0.9.4 fix in now. a=asa (on behalf of drivers) for checkin to
0.9.5 (which patch is it)

Comment 87

[Brendan Eich [:brendan]]

I'm with Asa.  jag, I'll let you bug yourself as needed.

/be

Comment 88

[Brendan Eich [:brendan]]

actually, I filed 103222 for jag.

/be

Comment 89

[jag (Peter Annema)]

I'll be checking in attachment 49027 [details] [diff] [review] when the tree opens.

Comment 90

[jag (Peter Annema)]

Checked in, marking fixed, using new bug for follow-up.

Comment 91

[André Dahlqvist]

I think this temp fix broke quiet a few things in Manage Bookmarks. When I
create new folders they don't show up until I close down manage bookmarks. The
same is true for moving bookmarks; they "dissapear" until you close and open the
bookmark manager again. I remember the bookmark manager being in this bad shape
a long time ago but not recently.

Comment 92

[kill this account]

André: The fix is not a "temp" fix. Brendan asked that the string class
used to pass data to the collator be update to a newer class. This is an
API change that has nothing to do with the fix.

This patch should only affect sorting not display. 

I'd do a rebuild and see if I can see this behavior I undo the patch
and see if it affects this.

Is anyone else seeing this?

Comment 93

[André Dahlqvist]

I just tested 2001-10-04-21 and it happens there too, so I guess this checkin
didn't cause it. Oh well...

Comment 94

[kill this account]

okay, I see the bookmark update problem with the patch and with the
patch backed out. 

I created bug 103394 and copied André's comments.

Comment 95

[sairuh (rarely reading bugmail)]

mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31.

if you think this particular bug is not fixed, please make sure of the following
before reopening:

a. retest with a *recent* trunk build.
b. query bugzilla to see if there's an existing, open bug (new, reopened,
assigned) that covers your issue.
c. if this does need to be reopened, make sure there are specific steps to
reproduce (unless already provided and up-to-date).

thanks!

[set your search string in mail to "AmbassadorKoshNaranek" to filter out these
messages.]