when automatically importing from bookmarks html format, use bookmarks.postplaces.html, if it exists

RESOLVED FIXED

Status

()

Firefox
Bookmarks & History
P4
normal
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: dietrich)

Tracking

({dataloss, regression})

Trunk
dataloss, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

in nsBrowserGlue.js, when automatically importing from bookmarks html format, use bookmarks.postplaces.html, if it exists (otherwise, defer to bookmarks.html)

the reason is if places.sqlite gets corrupted, or we do a schema migration (before we have json), we should use bookmarks.postplaces.html if it exists.

That file, if it exists, is more up to date than bookmarks.html
Flags: blocking-firefox3?
note:

when we fix this, we should also fix the fix for bug #406094.  when recreating from bookmarks.postplaces.html, we should not reset the browser.places.createdDefaultQueries pref, as if it is set, bookmarks.postplaces.html will already have the special folder, and we'd end up with duplicates.
as with bug #406094, this is important for the places.sqlite corruption, migration and the case where the user (right now, power user / nightly tester) decides to remove places.sqlite manually

fix in hand.
Assignee: nobody → sspitzer
Target Milestone: --- → Firefox 3 M10
Created attachment 291578 [details] [diff] [review]
patch
Attachment #291578 - Flags: review?(dietrich)
here are some test cases:

note, by default, browser.bookmarks.overwrite is false (as we don't overwrite fx 2's bookmarks.html.  instead we write to bookmarks.postplaces.html)

with browser.bookmarks.overwrite to false (which is the default)

1)

create a profile using fx2, add some bookmarks, exit.  migrate to fx 3, add some bookmarks.  notice you have the "Smart Bookmarks" folder. exit.  for the purpose of testing this bug, when adding bookmarks, use the personal toolbar (as the "unfiled" root is not exported yet.)

remove places.sqlite, we should restore (as best we can, given that not everything is in the html format, bugs are logged on this), and you should see the bookmarks you created in fx 3 and you will have the "Smart Bookmarks" folder, as it was in bookmarks.postplaces.html.  You should not get a duplicate "Smart Bookmarks" folder.

2) 

create a profile using fx2, add some bookmarks, exit.  migrate to fx 3, add some bookmarks.  notice you have the "Smart Bookmarks" folder. exit.  for the purpose of testing this bug, when adding bookmarks, use the personal toolbar (as the "unfiled" root is not exported yet.)

remove places.sqlite and bookmarks.postplaces.html, we should restore (as best we can, given that not everything is in the html format, bugs are logged on this), and you should see the bookmarks you created in fx 2, but not fx 3.  note, we will not recreate the "Smart Bookmarks" folder.

Flags: in-litmus?
note to dietrich, the change to nsNavHistory.cpp is what I mentioned in comment #1
(Assignee)

Comment 6

10 years ago
Comment on attachment 291578 [details] [diff] [review]
patch

this looks ok. we should hammer on these scenarios on the testday on friday. r=me.
Attachment #291578 - Flags: review?(dietrich) → review+
this regressed ("use the right bookmarks.html file when places.sqlite is corrupt or removed") when I fixed bug #381216
Status: NEW → ASSIGNED
Depends on: 381216
Keywords: regression
Attachment #291578 - Flags: approval1.9?

Updated

10 years ago
Attachment #291578 - Flags: approval1.9? → approval1.9+
updating summary.

fixed.

Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.45; previous revision: 1.44
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.206; previous revision: 1.205
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Summary: in nsBrowserGlue.js, when automatically importing from bookmarks html format, use bookmarks.postplaces.html, if it exists (otherwise, defer to bookmarks.html) → when automatically importing from bookmarks html format (because schema changes, places.sqlite is gone or corrupt), use bookmarks.postplaces.html, if it exists (otherwise, defer to bookmarks.html)
      if (overwriteBookmarks) {
        rv = prefs->SetBoolPref(PREF_BROWSER_CREATEDSMARTBOOKMARKS, PR_FALSE);
        NS_ENSURE_SUCCESS(rv, rv);
      }

this is true if we don't have places.sqlite and we have bookmarks.postplaces, but if we don't have any of them, we should however create the smart bookmarks, while this now depends on overwriteBookmarks value. there should be a check if bookmarks.postplaces.html exists like (pseudocode)
  if (overwriteBookmarks || !file_exists("bookmarks.postplaces.html")) 


(Assignee)

Comment 10

10 years ago
backing out to test Ts impact

Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.46; previous revision: 1.45
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.210; previous revision: 1.209
done
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

10 years ago
Summary: when automatically importing from bookmarks html format (because schema changes, places.sqlite is gone or corrupt), use bookmarks.postplaces.html, if it exists (otherwise, defer to bookmarks.html) → when automatically importing from bookmarks html format, use bookmarks.postplaces.html, if it exists
(Assignee)

Updated

10 years ago
Depends on: 406945
This had a 14 ms impact on Ts.

here's what I'm not understanding yet:

for an existing profile, browser.places.importBookmarksHTML would be false, so we shouldn't be hitting the code I added to nsBrowserGlue.js unless places.sqlite was corrupt or non-existent.

See http://lxr.mozilla.org/seamonkey/source/browser/components/nsBrowserGlue.js#350 for where we return early.

Sorry for asking this about the Ts test, but is it a new profile every time?  I think Ts kills firefox.exe, but  I don't think that should corrupt places.sqlite.

browser.places.importBookmarksHTML starts off as true.  So the first Ts should be slow.

I wonder if for Ts we a start with a new profile, import bookmarks.html, kill firefox, before the pref change is flushed to disk.  This would mean that each time we ran nsBrowserGlue.js for Ts, we would be executing a bunch of code we don't need to measure (as it is first time only import code.)

I'l see if I can verify this theory locally.
Status: REOPENED → ASSIGNED
This also seems to have affected Txul on Linux. It jumped from 212 to 220, when checked in, and dropped again when backed out.

http://build-graphs.mozilla.org/graph/query.cgi?tbox=bl-bldlnx03_fx-linux-tbox-head&testname=xulwinopen&autoscale=1&size=&units=ms&ltype=&points=&showpoint=2007%3A12%3A05%3A14%3A57%3A32%2C212&avg=1&days=1

Updated

10 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Target Milestone: Firefox 3 M10 → Firefox 3 M11
mconnor points out that once we have .json export / import, restoring after removing / corrupting places.sqlite should be using the .json file, not bookmarks.postplaces.html.

So this bug is really a wontfix, if we get the .json stuff.

Updated

10 years ago
Duplicate of this bug: 415836
So it might be a wontfix once json is done but right now not having this is causing dataloss.
Keywords: dataloss
And this isn't really a wontfix unless the json stuff actually fixes it, which the initial patch there doesn't look like it does.
(Assignee)

Updated

10 years ago
Target Milestone: Firefox 3 beta3 → ---
Should this still be assigned to Seth ?
mh should be fixed in JSON, unassigning. change back if that is not correct.
Assignee: moco → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

10 years ago
Assignee: nobody → dietrich
(Assignee)

Updated

10 years ago
Depends on: 384370
(Assignee)

Updated

10 years ago
Whiteboard: [patch in bug 384370]

Updated

10 years ago
Priority: P3 → P4
(Assignee)

Comment 19

10 years ago
bug 384370 causes initial import to be done from the latest json backup first, and if that doesn't exist, then bookmarks.html. bookmarks.postplaces.html is no longer exported. -> fixed.
Status: NEW → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [patch in bug 384370]
The bug is out of the scope of the library subgroup in FFT, so setting in-litmus flag- .
Flags: in-litmus? → in-litmus-
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
You need to log in before you can comment on or make changes to this bug.