Closed Bug 381216 Opened 17 years ago Closed 17 years ago

prevent bookmarks dataloss when a user goes from Firefox 2, Firefox 3 beta, Firefox 2, and then back to Firefox 3 [was: After initial import, minefield with bookmarks-on-places throws out Firefox 2 bookmarks changes]

Categories

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

Product:
Firefox
Component:
Bookmarks & History
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3 beta1

People

(Reporter: hello, Assigned: moco)

References

Details

Attachments

(1 file, 2 obsolete files)

patch, don't create bookmarks.preplaces.html if we aren't going to overwrite (per dietrich over irc)
7.08 KB, patch
dietrich
: review+
beltzner
: approval1.9+
Details | Diff | Splinter Review 
The first time a user runs a bookmarks-on-places build, their existing bookmarks.html gets imported.  If a user then goes back to Firefox 2, and they edit their bookmarks, their changes will be thrown away upon returning to minefield.

This will likely be surprising to many users, and I suspect we may want to consider making this scenario work, or at least warn before overwriting their changes.

One possible solution: peek into the bookmarks.html from browserGlue, and try to guess if it was written by Firefox 2 (check for ID vs. ITEM-ID, for example), and force import in that case.

Discuss.
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3
thanks for catching this.  I think we want to fix this, as I can imagine lots of people switching between fx 2 and a beta, alpha or nightly of what will become fx 3.

checking for ITEM_ID seems reasonable.  see also bug #381217 for another current difference between places bookmarks.html and non-places bookmarks.html.
for ITEM_ID, I'm not sure how far into bookmarks.html we'll have to read.  maybe a html comment is the way to go, assuming that an extra html comment won't cause problems for other browsers.

alternatively, could we check something about bookmarks.html (like last modified date) of bookmarks.html, to see if it was modified since we last wrote it.  

note, we write it in nsBrowserGlue.js, _shutdownPlaces().
dietrich points out that if we fixed this, we'd have data loss in the other direction.

if you run fx 3, go to fx 2, and come back to fx 3 (and we force a re-import) you'd lose places data, like any bookmark annotations.

the workaround for this then is to manually reimport your bookmarks, although you have to get it out of the backups folder.

this is feeling like a WONTFIX
Why would you loose annotations? Import code doesn't (and isn't designed to) remove annotations from items already in the table.

If we do happen to run into a dataloos issue here, could we even rev the places scheme post Fx3?
for fx 3 to fx 3 forced bookmarks.html re-import (on a schema change), we have ITEM_IDs in bookmarks.html that we use which should make it so our existing moz_annos match up to what we put back into moz_bookmarks.  (I need to re-test that.)

for fx 2 -> fx 3 import, we don't have ITEM_IDs in bookmarks.html, as fx 2 will remove them (and we can't use ID attributes per bug #381208)
(In reply to comment #4)
> Why would you loose annotations? Import code doesn't (and isn't designed to)
> remove annotations from items already in the table.

if we do a force-migration, the id->anno mapping is lost.

> 
> If we do happen to run into a dataloos issue here, could we even rev the places
> scheme post Fx3?
> 

not sure what you are saying here..

all in all, i didn't think that we had a transparent-backward-then-forward-via-bookmarks.html requirement.

we knew when moving to sqlite that bookmarks.html would no longer be the sole source-of-truth. do the other sqlite features in the profile have this requirement, and do they work that way? form history, search engines, etc...

we decided to support backwards migration by always exporting bookmarks.html. bugs that prevent that from happening are valid.

we could do something that allows us to detect the change and re-import, however we run into issues there as well:

- if we merge, we start running into dupes, etc, all the problems we just talked about earlier today wrt to supporting seamless merges

- if we dump-and-import, then we lose annotation id mappings, and all the other stuff that we and extension developers have built against places data (unless we start building even more infrastructure on top of bookmarks.html to support those things).

finally: we don't want to encourage continued dependence on this flawed, unclosed-tag-ridden format. we should be spending effort moving users away from it.
This would seem to cause problems for those who use the pref override:

user_pref("browser.bookmarks.file", "bookmark pathname");

Some amount of users (as I) may be using this as a means to insure that their bookmarks file is centralize, irrespective of which browser, or browser version, is being run, e.g., for testing purposes, &c.

Some consideration should be given to disabling this pref whenever fx3 is invoked and moving a copy of the bookmarks file to the fx3 user profile and then issue some type of warning to that effect.
Blocks: 381280
This isn't a blocker on the release, to be sure. Connor and I disagree a little in that I think it would be a nice fix for our testing community.
Flags: blocking-firefox3? → blocking-firefox3-
(In reply to comment #8)
> This isn't a blocker on the release, to be sure. Connor and I disagree a little
> in that I think it would be a nice fix for our testing community. 

The more I think about this bug, the more worried I get.

I think that it is not uncommon for someone to try Firefox N+1, decide they don't like it (doesn't work with their favorite extension yet, for example) and temporarily go back to Firefox N.  That scenario would almost inevitably result in dataloss on re-upgrade if we leave things the way they are now.  And not only that--the longer they postpone trying Firefox 3 for the 2nd time, the more data they will probably have accumulated that we will throw out the window.

The thing that bugs me the most about that scenario is that non-powerusers are just as likely to be affected by the problem.  You don't need to mess with your profile or do anything funny, except go back to an older version for a while.

Now, I agree that blindly doing a re-import would result in dataloss as well (annotations, etc).  So what I think we should do is never write to bookmarks.html.  We can read it once on first run, but from that point on it would be unused by Firefox 3.  We would use a different file (possibly even a different format) to do database migrations as well as backups.

The good: No dataloss on either version.

The bad:
1) Users who switch back and forth between 2 and 3 will have their bookmarks diverge.
2) Users who depend on bookmarks.html being automatically exported will not have that anymore.
3) Users who depend on being able to browse to their bookmarks.html will be disappointed. 

(1) Is not great, but no data is lost so the user could manually export/import.  Definitely acceptable imo.
(2) Will hurt power-users, mostly, but that situation can easily be improved by writing an extension that automatically exports bookmarks.  If they wish, they can even auto-export to Firefox 2's bookmarks.html at that point, and get exactly the current behavior.
(3) Is extension fodder.

Thoughts?
al asks:

Question for you, how often and when does FF3 import existing bookmark files in a profile into places.sqlite?

Answer:

a) the first time your run fx 3 (from fx 2).  meaning, first migration we will import bookmarks.html

b) if you remove places.sqlite, we will also import bookmarks.html automatically

c) if we migrate the internal schema, we way also import (but not always).

Q:  So, if I'm using the same profile and switching between ff2 and ff3?
A:  right, that will not import
Renominating.

See http://blog.mozilla.com/thunder/2007/08/02/bookmarks-and-upgradedowngrade-fun/ for more comments from me.
Flags: blocking-firefox3- → blocking-firefox3?
we need to come up with a solution.  The low bar is to store write time for bookmarks.html, and if Fx3 didn't change it last, offer to re-import.
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: Firefox 3 → Firefox 3 M10
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Assignee: thunder → dietrich
Priority: -- → P1
Hardware: PC → All
Target Milestone: Firefox 3 M11 → Firefox 3 M10
Attached patch patch, testing now (obsolete) — — Splinter Review 
if browser.bookmarks.overwrite is false, stop writing out bookmarks.html to the profile directory.

(note, we continue to write out bookmarkbackups\bookmarks-YYYY-MM-DD.html)
Comment on attachment 287912 [details] [diff] [review]
patch, testing now

this is not going to work, as we *use* bookmarks.html to make our backups our backups
Attachment #287912 - Attachment is obsolete: true
discussion from irc:

<mconnor> dietrich: ping?
<dietrich> mconnor: hi
<mconnor> dietrich: so, QA ran across the bookmarks.html up/down/up thing, they're pretty adamant that we don't stomp bookmarks.html after import
<mconnor> dietrich: can we just default writing that on exit to off, via a pref, and see what breaks when we do?
<dietrich> mconnor: sure
<dietrich> what'll break is that any new bookmarks you added in Fx3 will not be present when you downgrade back to Fx2
<dietrich> but i guess that makes our treatment of bookmarks and history consistent in that regard
<mconnor> dietrich: yep
<mconnor> dietrich: I don't remember what depended on bookmarks.html, but I kinda don't care anymore, let them parse JSON
<dietrich> srsly
<mconnor> or, set the pref
<mconnor> how long to patch?
<sspitzerMsgMe> dietrich / mconnor: something to keep in mind:
<sspitzerMsgMe> we have code that does this:  if places.sqlite is removed (or corrupt), we attempt to re-import from bookmarks.html
<sspitzerMsgMe> perhaps we should fix that to use the "latest" backup instead?
<mconnor> yes
<mconnor> we absolutely should
<sspitzerMsgMe> worth a read:  http://blog.mozilla.com/thunder/2007/08/02/bookmarks-and-upgradedowngrade-fun/
<mconnor> I'd love to know how often we ever hit that code
<sspitzerMsgMe> which code?
<mconnor> the corrupt/re-import code
<sspitzerMsgMe> we also use bookmarks.html on schema upgrade
<mconnor> we should use the JSON backup
<mconnor> once we get native JSON
<dietrich> i think i already have that bit written as part of bug 384370
<dietrich> (using latest backup)
<mconnor> ok
<mconnor> we can fix the recovery cases in M10
<sspitzerMsgMe> mconnor:  you want this fixed for beta, it sounds like?
<dietrich> mconnor: the pref fix could be done real soon. why you thinking m9?
<mconnor> yeah
<abillings> Can you guys detail the new behavior in a bug so we can track it outside of IRC?
<abillings> mconnor: we're going to respin for this?
<mconnor> abillings: looks like
<dietrich> this is bug 381216
<abillings> lame when we've done it this way for months.
<abillings> And it was "Won't Fix" before effectively
<dietrich> abillings: i kinda agree, but it's more of an issue w/ the broader audience a beta brings
<abillings> I'm just curious why it got brought up now instead of a month ago.
<abillings> Since the behavior isn't new
<dietrich> mconnor: i need to go offline for about 30 mins, can tackle then
<dietrich> sspitzerMsgMe: or feel free to take it. it's a minor change in nsBrowserGlue for the pref, iirc
<sspitzerMsgMe> dietrich:  taking.
<dietrich> sspitzerMsgMe: awesome, thx. bbiab.
Attached patch patch (obsolete) — — Splinter Review 
low risk change.

if browser.bookmarks.overwrite is true, we do what we've always done, and write out to bookmarks.html and when doing backup, copy that file to bookmarkbackups (when desired.  note, we don't always force that.)

if browser.bookmarks.overwrite is false (the default value), we write out to bookmarks.postplaces.html and when doing backup, copy that file to bookmarkbackups
Attachment #287921 - Flags: review?(dietrich)
<sspitzerMsgMe> dietrich / mconnor: something to keep in mind:
<sspitzerMsgMe> we have code that does this:  if places.sqlite is removed (or
corrupt), we attempt to re-import from bookmarks.html
<sspitzerMsgMe> perhaps we should fix that to use the "latest" backup instead?
<mconnor> yes

note, this is not fixed.  chatting with dietrich over irc, what'd we do is in _initPlaces(), if the overwrite pref is false, use bookmarks.postplaces.html instead of bookmarks.html (if it exists).

note, the first time we need to use bookmarks.html (for proper fx 2 - > fx 3 import)

note, there is a manual workaround available, use the bookmarks organizer to restore from a bookmarks.postplaces.html or from a backup in the backups directory.

I'll log a spin off bug, but this is something dietrich's fix for bug #384370 (use json instead of html) will address, too (so it might be moot.)


I think mconnor (and others) want this for m9 / b1
Assignee: dietrich → sspitzer
Target Milestone: Firefox 3 M10 → Firefox 3 M9
Comment on attachment 287922 [details] [diff] [review]
patch, don't create bookmarks.preplaces.html if we aren't going to overwrite (per dietrich over irc)

looks ok, thanks for the change. this does fix the up/down/up migration path so that there's no data loss from the 2nd upgrade. however, the downsides to this solution are:

1. after downgrading, you don't automatically get any bookmarks added in fx3. you need to manually import the bookmarks.postplaces.html file to get them (or manually export from the organizer and then import).

2. if your places.sqlite is corrupt, we'll import the old fx2 bookmarks. the workaround for this is to manually import the bookmarks.postplaces.html file. this is not a big concern, as we have yet to encounter a places.sqlite corruption bug.
Attachment #287922 - Flags: review?(dietrich) → review+
> however, the downsides to this solution are:

Also note:

3. If we happen to change the schema, we'll import the old fx2 bookmarks.

(In reply to comment #22)
> > however, the downsides to this solution are:
> 
> Also note:
> 
> 3. If we happen to change the schema, we'll import the old fx2 bookmarks.
> 

I assumed that the soonest that'd happen would be beta 2, and we could fix it to use the latest backup at that point.
> I assumed that the soonest that'd happen would be beta 2, and we could fix it
> to use the latest backup at that point.

someone could, in theory, switch back and forth between old trunk builds with different internal places.sqlite schemas, right?
fix landed on the GECKO190_20071106_RELBRANCH, a=schrep

I have not yet landed on the trunk, so keeping the bug open.

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.219.2.1; previous revision: 1.219
done
Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.36.2.1; previous revision: 1.36
done
Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v
  <--  nsPlacesImportExportService.cpp
new revision: 1.30.2.1; previous revision: 1.30
done
Up/down/up is a common "early adopter" migration scenario, and should be included in Litmus.
Flags: in-litmus?
Comment on attachment 287922 [details] [diff] [review]
patch, don't create bookmarks.preplaces.html if we aren't going to overwrite (per dietrich over irc)

a=beltzner for trunk as well, please find whomever has the baton and land when you can; it would be nice to have this in nightlies.
Attachment #287922 - Flags: approval1.9+
to clarify:

I've added a new pref ("browser.bookmarks.overwrite") which defaults to false.

If false, we:

1)  do not copy bookmarks.html to bookmarks.preplaces.html on initial import (from Fx 2 -> Fx 3), since we aren't going to stomp on it
2)  do not export bookmark data to bookmarks.html on quit.  Instead we export to bookmarks.postplaces.html.  We will continue to make backups (using bookmarks.postplaces.html as our "source", not bookmarks.html

If true, we do what we used to do, which was:

1)  copy bookmarks.html to bookmarks.preplaces.html on initial import (from Fx 2 -> Fx 3), since we *are* going to stomp on it
2)  export bookmark data to bookmarks.html on quit.  We make backups (using bookmarks.html as our "source")

As Dietrich pointed out over IRC, existing users (of the trunk or of fx 3 alpha ) will have bookmarks.preplaces.html, bookmarks.html and bookmarks.postplaces.html.

if fx 3 beta 1 is the first fx 3 build you've run, you'll only have bookmarks.html and bookmarks.postplaces.html
Status: NEW → ASSIGNED
Just tested this for a while. It looks good.

Test Cases

1. Verify that browser.bookmarks.overwrite is set to false by default. - Pass

2. Verify that bookmarks.preplaces.html is not created on first run of FF3 on an FF2 profile by default. - Pass

3. Verify that bookmarks.html is *not* written to on FF3 exit by default. - Pass

4. Verify that bookmarks.postplaces.html *is* written on FF3 exit by default. - Pass

If browser.bookmarks.overwrite is set to true:

5. Verify that bookmarks.html is copied to bookmarks.preplaces.html on initial FF3 run and import.

6. Verify that bookmarks.html is written to on FF3 exit. - Pass

7. Verify that bookmarks.postplaces.html is no longer writen on FF3 exit. - Pass

8. Verify that boomarks.preplaces.html is written if places.sqlite is deleted, forcing reimport of bookmarks.html. - Pass
Thanks for fixing this, Seth!

One note, allowing Fx2 & Fx3 bookmarks to diverge means that if a user tries the beta, then goes back to Fx2 and continues to use that until major update, it may feel to them like they lost some bookmarks (even though they'll still be there in bookmarks.html).  If you install in parallel you can open up Fx2, but if you don't you'll have to just know to browse to your profile to find your bookmarks.

It'll probably (?) be a small percentage of users, but maybe we could prompt to re-import around major update time (just once)? It could use a heuristic, like your places.sqlite mtime being more than N weeks/months older than your bookmarks.html mtime.

Or maybe instead of prompting, we could simply copy bookmarks.html into the backups dir.  If we name it something like "Bookmarks (Firefox 2)", it won't get expired and it'll be obvious what to do in the menu, provided they check the menu at all.
al billings, thanks for testing and for reporting what you tested.  

in addition to what you tested, I also tested that we're doing the "right thing" in the bookmarkbackups directory.  

by that, for either setting, you should see backups get made.  

I'd recommend testing with a new profile, as we don't make backups every time if the backup exists.

But with a new profile, on first exit, you should then see bookmarks-YYYY-MM-DD.html in bookmarkbackups (under your profile), and it should match bookmarks.html (if overwrite pref is true) or bookmarks.postplaces.html (if overwrite pref is false).

let me know if you have questions.
thunder, I'll log your suggestion as a spin of bug.

fix landed on the trunk:

Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.37; previous revision: 1.36
done
Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v
  <--  nsPlacesImportExportService.cpp
new revision: 1.31; previous revision: 1.30
done
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.222; previous revision: 1.221
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Summary: After initial import, minefield with bookmarks-on-places throws out Firefox 2 bookmarks changes → prevent bookmarks dataloss when a user goes from Firefox 2, Firefox 3 beta, Firefox 2, and then back to Firefox 3 [was: After initial import, minefield with bookmarks-on-places throws out Firefox 2 bookmarks changes]
Seth, yes, I used a new profile and I do get the backups directory created and the initial backup as well.

As to the matching, I'll have to check that. I hadn't thought of that either.
> As to the matching, I'll have to check that. I hadn't thought of that either.

keep in mind, if there is a backup already for "today" we won't overwrite

(see http://lxr.mozilla.org/seamonkey/source/browser/components/places/src/nsPlacesImportExportService.cpp#2628)

so, if you do this test with a new profile, the first time you exit, the .html files should match (but if you were to start up, change a bookmark, etc) they would not match.

Thanks again, Al.  I appreciate the extra eyeballs on this.
> thunder, I'll log your suggestion as a spin of bug.

see bug #403237
Verified in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b1) Gecko/2007110903 Firefox/3.0b1
Status: RESOLVED → VERIFIED
Hello, all!

"False" as default for "browser.bookmarks.overwrite" could cause issues with third-party applications that are set to read bookmarks.html.

For example, I use Launchy in Windows, which indexes bookmarks.html, and I cannot launch my recent bookmarks from Launchy, because on shut-down only bookmarks.postplaces.html is updated.

Firefox version: latest trunk
Launchy version: latest beta from here:
https://sourceforge.net/forum/forum.php?forum_id=451017

Greetings,
Demetris
note, this bug caused a regression.  

after this fix, if you were to remove places.sqlite or corrupt it, we would use bookmarks.html and not bookmarks.postplaces.html to restore it.

see bug #406114 for details and the fix.
this isn't really a good case for Litmus. It's something we should check for releases, but not a good every day regression test case; in-litmus-
Flags: in-litmus? → in-litmus-
If one has a test harness that can launch instances of Firefox, shutdown instances of Firefox, re-start them, and specify the profile for all of them, one can test this automatedly.

I have scripts that can do this, but I understand that MoCo has other tools for this. How are they working?
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks

Description:
Please provide an explanation of the feature or change. Include a description of the user scenario in which it would be used and how the user would complete the task(s).
Screenshots and visual UI specs are welcome, but please include sufficient accompanying explanation so that blind members of the accessibility team are able to understand the feature/change.

How do we test this?
If there is an implementation to test, please provide instructions for testing it; e.g. setting preferences, other preparation, how to trigger the UI, etc.

When will this ship?
Tracking bug/issue:not working
Design documents (e.g. Product Requirements Document, UI spec):
Engineering lead:
Product manager:

The accessibility team has developed the Mozilla Accessibility Release Guidelines which outline what is needed to make user interfaces accessible:
https://wiki.mozilla.org/Accessibility/Guidelines
Please describe the accessibility guidelines you considered and what steps you've taken to address them:

Describe any areas of concern to which you want the accessibility team to give special attention:

a11y-review: --- → requested
a11y-review: requested → ---

Restricting comments as this has become a spam trap.

Restrict Comments: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: