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

Status: RESOLVED FIXED

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

Description

[(not reading, please use [e-mail] instead)]

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 [e-mail] instead)]

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 [e-mail] instead)]

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 [e-mail] instead)]

Attachment: patch

Comment 4

[(not reading, please use [e-mail] instead)]

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 [e-mail] instead)]

note to dietrich, the change to nsNavHistory.cpp is what I mentioned in comment #1

Comment 6

[Dietrich Ayala (:dietrich)]

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 [e-mail] instead)]

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 [e-mail] instead)]

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]]

      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)]

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 [e-mail] instead)]

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]]

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 [e-mail] instead)]

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 15

[Dave Townsend [:mossop]]

So it might be a wontfix once json is done but right now not having this is causing dataloss.

Comment 16

[Dave Townsend [:mossop]]

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] (UTC+12)]

Should this still be assigned to Seth ?

Comment 18

[Marco Bonardo [:mak]]

mh should be fixed in JSON, unassigning. change back if that is not correct.

Comment 19

[Dietrich Ayala (:dietrich)]

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]]

The bug is out of the scope of the library subgroup in FFT, so setting in-litmus flag- .

Comment 21

[Gervase Markham [:gerv]]

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