Last Comment Bug 406114 - when automatically importing from bookmarks html format, use bookmarks.postplaces.html, if it exists
: when automatically importing from bookmarks html format, use bookmarks.postpl...
Status: RESOLVED FIXED
: dataloss, regression
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: P4 normal (vote)
: ---
Assigned To: Dietrich Ayala (:dietrich)
:
Mentors:
: 415836 (view as bug list)
Depends on: 381216 384370 406945
Blocks:
  Show dependency treegraph
 
Reported: 2007-11-29 19:59 PST by (not reading, please use seth@sspitzer.org instead)
Modified: 2014-06-14 02:19 PDT (History)
13 users (show)
mconnor: blocking‑firefox3+
mozaakash: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.60 KB, patch)
2007-12-04 17:59 PST, (not reading, please use seth@sspitzer.org instead)
dietrich: review+
dsicore: approval1.9+
Details | Diff | Review

Description (not reading, please use seth@sspitzer.org instead) 2007-11-29 19:59:36 PST
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
Comment 1 (not reading, please use seth@sspitzer.org instead) 2007-12-03 20:20:50 PST
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.
Comment 2 (not reading, please use seth@sspitzer.org instead) 2007-12-04 17:56:34 PST
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.
Comment 3 (not reading, please use seth@sspitzer.org instead) 2007-12-04 17:59:09 PST
Created attachment 291578 [details] [diff] [review]
patch
Comment 4 (not reading, please use seth@sspitzer.org instead) 2007-12-04 18:08:34 PST
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.

Comment 5 (not reading, please use seth@sspitzer.org instead) 2007-12-04 18:34:43 PST
note to dietrich, the change to nsNavHistory.cpp is what I mentioned in comment #1
Comment 6 Dietrich Ayala (:dietrich) 2007-12-04 22:32:52 PST
Comment on attachment 291578 [details] [diff] [review]
patch

this looks ok. we should hammer on these scenarios on the testday on friday. r=me.
Comment 7 (not reading, please use seth@sspitzer.org instead) 2007-12-04 22:39:56 PST
this regressed ("use the right bookmarks.html file when places.sqlite is corrupt or removed") when I fixed bug #381216
Comment 8 (not reading, please use seth@sspitzer.org instead) 2007-12-04 23:15:45 PST
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
Comment 9 Marco Bonardo [::mak] 2007-12-05 06:02:00 PST
      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")) 


Comment 10 Dietrich Ayala (:dietrich) 2007-12-05 09:14:28 PST
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
Comment 11 (not reading, please use seth@sspitzer.org instead) 2007-12-05 13:02:45 PST
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.
Comment 12 Justin Dolske [:Dolske] 2007-12-05 15:17:36 PST
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
Comment 13 (not reading, please use seth@sspitzer.org instead) 2007-12-13 15:52:55 PST
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.
Comment 14 Dave Townsend [:mossop] 2008-02-05 15:59:03 PST
*** Bug 415836 has been marked as a duplicate of this bug. ***
Comment 15 Dave Townsend [:mossop] 2008-02-05 16:01:04 PST
So it might be a wontfix once json is done but right now not having this is causing dataloss.
Comment 16 Dave Townsend [:mossop] 2008-02-08 12:03:19 PST
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.
Comment 17 Nick Thomas [:nthomas] 2008-02-21 01:30:34 PST
Should this still be assigned to Seth ?
Comment 18 Marco Bonardo [::mak] 2008-02-21 01:37:06 PST
mh should be fixed in JSON, unassigning. change back if that is not correct.
Comment 19 Dietrich Ayala (:dietrich) 2008-03-14 09:59:33 PDT
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.
Comment 20 Aakash Desai [:aakashd] 2009-03-04 15:23:33 PST
The bug is out of the scope of the library subgroup in FFT, so setting in-litmus flag- .
Comment 21 Gervase Markham [:gerv] 2009-11-26 06:13:27 PST
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

Note You need to log in before you can comment on or make changes to this bug.