Closed Bug 462366 Opened 16 years ago Closed 16 years ago

correct bogus importBookmarksHTML behaviour

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3.1b2

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: dataloss, dev-doc-complete, verified1.9.1)

Attachments

(1 file, 5 obsolete files)

this is awesome, set this two vars as subject and you'll never see your bookmarks persist, they will always be overwritten by the last backup.

STR:
1. have at least one json backup in bookmarksbackup folder
2. set importBookmarksHTML to true
3. set restore_default_bookmarks to false

change bookmarks, restart the browser, notice we have restored old ones, change again restart, notice we have restored again, and so on...
So we don't reset importBookmarksHTML after an import which causes the re-import each time Firefox is started?
Severity: normal → critical
Keywords: dataloss
in this particular situation, yes
Attached patch patch (obsolete) — Splinter Review
notice i'm setting the pref to false even if it's already false, i think doesn't matter here since setting only if trye would require to dupe importBookmarks variable to importBookmarksHTML and manage 2 vars. Since we are already restoring from disk, the additional cost of a setBoolPref should not be a problem at all.
Attachment #345505 - Flags: review?(dietrich)
Could we get an xpcshell unit test for this so we do not regress it again?
Priority: -- → P2
Target Milestone: --- → Firefox 3.1b2
(In reply to comment #4)
> Could we get an xpcshell unit test for this so we do not regress it again?

it needs to go through nsBrowserGlue, so an xpcshell test won't suffice. we need a test framework that can start the browser, do stuff, restart the browser, do stuff. mozmill maybe?
Attachment #345505 - Flags: review?(dietrich)
Comment on attachment 345505 [details] [diff] [review]
patch

per irc: the existing code is broken. if importBookmarksHTML is true, then bookmarks.html should be imported at startup.
morphing bug to globally fix import configs in browserGlue
Summary: if restoreDefaultBookmarks is false and importBookmarksHTML is true we never stop restoring bookmarks → correct bogus importBookmarksHTML behaviour
Attachment #345505 - Attachment is obsolete: true
(In reply to comment #5)
> it needs to go through nsBrowserGlue, so an xpcshell test won't suffice. we
> need a test framework that can start the browser, do stuff, restart the
> browser, do stuff. mozmill maybe?

Clint, is mozmill capable of doing this at the current stage?
Blocks: 459377
in nsNavhistory we don't do any difference between a corrupt db or a not existant one, so we only use importBookmarksHTML as config for both cases
(In reply to comment #9)
> in nsNavhistory we don't do any difference between a corrupt db or a not
> existant one, so we only use importBookmarksHTML as config for both cases

i'd like to see that changed to be more specific, and to remove any knowledge of the import stuff from toolkit. see bug 394205 for some (long bitrotted) work towards decoupling browser and toolkit initialization this way.
Attached patch wip 0.1 (obsolete) — Splinter Review
Flags: blocking-firefox3.1?
Whiteboard: [needs new patch]
this does not work due to a strange behaviour on first profile creation, the places.sqlite is created and history property is correctly setup, but then history service is started again and since places.sqlite exists property is false and we don't import in the frontend.

Dietrich, while trying to decouple frontend and backend, could we take attachment 345505 [details] [diff] [review] as a temporary workaround? i see a bunch of "bookmarks revert" reports in bugzilla that could be caused by this and would allow to distinguish better real corruptions.
i mean taking that as temporary workaround for b2
I agree that this should be a blocker if it's a bookmark import regression, but why a Beta 2 blocker? Do we expect a lot of people to have importBookmarksHTML set in that scenario?
Flags: blocking-firefox3.1? → blocking-firefox3.1+
we have a certain number of users reporting database corruption that is not "real" due to this, and having a first workaround would allow to distinguish real corruptions from fake ones in feedback we will get from the beta.
i probably found the culprit, nsPlacesDBFlush is registering a bookmarks observer BEFORE we call _initplaces (it is started in profile-after-change), and is doing that also in profile creation path when initPlaces is not called at all and we don't need to sync.
This practically makes us initialize the history service too early through the bookmarks service, that was what i was seeing when looking at our Ts regression (history initialized by bookmarks service), so when we do real init in initPlaces the db has been already created by previous profile setup run, and we can't detect history database status correctly.
Attached patch wip 0.2 (obsolete) — Splinter Review
Works fine in browser, but if a third party implementer tries to init bookmarks service before history service, we fail due to recursive initing of bookmarks service. That's due to the notification that causes nsPlacesDBFlush adding a bookmarks observer. For the same reason tests that directly init bookmarks service are not passing. Looking for a solution.
Attachment #346163 - Attachment is obsolete: true
Attached patch wip 0.3 (obsolete) — Splinter Review
this is working as expected, however i had to dispatch initPlaces to the mainthread to avoid a recursive create on nsNavHistory.
Attachment #347284 - Attachment is obsolete: true
Blocks: 394205
Attached patch patch v1 (obsolete) — Splinter Review
instead of enqueueing at target i'm enqueueing at source, so implementers should be safe observing the notification and initializing history service in the callback. This creates a good separation between toolkit and browser code (and prefs). So we are ready for first review.

tests needed:
1. create a new profile, bookmarks are imported from default bookmarks.html
2. delete places.sqlite, bookmarks are restored from JSON backup
3. delete all profile folder contents, bookmarks are imported from default bookmarks.html
4. corrupt places.sqlite with an hex editor, bookmarks are restored from JSON backup, places.sqlite.corrupt is created
5. migrate from 3.0 branch profile, bookmarks retained
6. migrate from 2.0 branch profile, bookmarks retained
7. users asks to import from bookmarks.html through importBookmarksHTML config
8. in safe mode user asks to restore default bookmarks

at each step should check for coherence:
1. Bookmarks toolbar, Bookmarks menu, Smart bookmarks, Library

QA may be aware of more test paths though, most likely this will need a QA test session, since testing with browser restart is not feasible.
Attachment #347441 - Attachment is obsolete: true
Attachment #347525 - Flags: review?(dietrich)
Flags: in-litmus?
Whiteboard: [needs new patch] → [needs QA testing]
Comment on attachment 347525 [details] [diff] [review]
patch v1


>@@ -146,16 +150,17 @@ BrowserGlue.prototype = {
>     osvr.addObserver(this, "xpcom-shutdown", false);
>     osvr.addObserver(this, "prefservice:after-app-defaults", false);
>     osvr.addObserver(this, "final-ui-startup", false);
>     osvr.addObserver(this, "sessionstore-windows-restored", false);
>     osvr.addObserver(this, "browser:purge-session-history", false);
>     osvr.addObserver(this, "quit-application-requested", false);
>     osvr.addObserver(this, "quit-application-granted", false);
>     osvr.addObserver(this, "session-save", false);
>+    osvr.addObserver(this, "places-init-complete", false);
>   },

need to remove this observer at some point.

>   /**
>    * Initialize Places
>-   * - imports the bookmarks html file if bookmarks datastore is empty
>+   * - imports the bookmarks html file if bookmarks database is empty, try to
>+   *   restore bookmarks from a JSON backup if the backend indicate that the
>+   *   database was corrupt.

nit: s/indicate/indicates/

>    *
>-   * These prefs are set by the backend services upon creation (or recreation)
>-   * of the Places db:
>+   * These prefs can be set up by the frontend:
>    * - browser.places.importBookmarksHTML
>-   *   Set to false by the history service to indicate we need to re-import.
>+   *   Set to true to indicate we need to re-import bookmarks.html.

"Set to true will import the bookmarks.html file." or something more direct like that.

>    * - browser.places.smartBookmarksVersion
>    *   Set during HTML import to indicate that Smart Bookmarks were created.
>    *   Set to -1 to disable Smart Bookmarks creation.
>    *   Set to 0 to restore current Smart Bookmarks.
>-   *
>-   * These prefs are set up by the frontend:
>    * - browser.bookmarks.restore_default_bookmarks
>    *   Set to true by safe-mode dialog to indicate we must restore default
>    *   bookmarks.

Please add a note of warning that setting these prefs to true will overwrite existing bookmarks.

>    */
>   _initPlaces: function bg__initPlaces() {
>-    // we need to instantiate the history service before checking
>-    // the browser.places.importBookmarksHTML pref, as
>-    // nsNavHistory::ForceMigrateBookmarksDB() will set that pref
>-    // if we need to force a migration (due to a schema change)
>+    // We must instantiate the history service since it will tell us if we
>+    // need to import or restore bookmarks due to first-run, corruption or
>+    // forced migration (due to a major schema change).
>     var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
>                   getService(Ci.nsINavHistoryService);
>+    var databaseStatus = histsvc.databaseStatus;
> 
>-    var importBookmarks = false;
>+    // If the database is corrupt or has been created we should import bookmarks
>+    var importBookmarks = databaseStatus != histsvc.DATABASE_STATUS_OK;

mega-nit for clarity: "newly created"

>+    // If the user did not required to restore default bookmarks or import
>+    // from bookmarks.html we will try to restore from JSON
>+    if (importBookmarks && !restoreDefaultBookmarks && !importBookmarksHTML) {

s/required/require/

>@@ -368,16 +378,17 @@ nsNavHistory::nsNavHistory() : mBatchLev
>                                mPreviousChunkOffset(-1),
>                                mAutoCompleteFinishedSearch(PR_FALSE),
>                                mExpireDaysMin(0),
>                                mExpireDaysMax(0),
>                                mExpireSites(0),
>                                mNumVisitsForFrecency(10),
>                                mTagsFolder(-1),
>                                mInPrivateBrowsing(PRIVATEBROWSING_NOTINITED)
>+                             , mDatabaseStatus(DATABASE_STATUS_OK)

change all commas or none.

>@@ -521,16 +532,22 @@ nsNavHistory::Init()
>   // Swallow errors here to not block initialization.
>   if (migrationType != DB_MIGRATION_NONE)
>     (void)RecalculateFrecencies(mNumCalculateFrecencyOnMigrate,
>                                 PR_FALSE /* don't recalculate old */);
> 
>   // Don't add code that can fail here! Do it up above, before we add our
>   // observers.
> 
>+  // Notify we have finished database initialization
>+  // Enqueue the notification, so if we init another service that requires
>+  // nsNavHistoryService we don't recursive try to get it.
>+  nsCOMPtr<PlacesInitCompleteEvent> completeEvent = new PlacesInitCompleteEvent();
>+  (void)NS_DispatchToMainThread(completeEvent);

should at least warn if this fails (we should do same for everything here that is currently ignored).

>+  // We have done a new databse init, so we mark this as if the database has
>+  // been created now, so the frontend can distinguish this status and import
>+  // if needed.
>+  mDatabaseStatus = DATABASE_STATUS_CREATE;
>+
>+  return NS_OK;
> }

spelling nit: database

>+
>+#define PLACES_INIT_COMPLETE "places-init-complete"

please name it so that it describes that it's a notification topic.

please add dev-doc-needed keyword, with a summary of the interface changes. given the nature of the change, shouldn't need late-compat keyword afaict.

r=me with these fixed, thanks!
Attachment #347525 - Flags: review?(dietrich) → review+
Whiteboard: [needs QA testing] → [needs QA testing][has review]
(In reply to comment #21)
> Created an attachment (id=347525) [details]
> patch v1
> 
> instead of enqueueing at target i'm enqueueing at source, so implementers
> should be safe observing the notification and initializing history service in
> the callback. This creates a good separation between toolkit and browser code
> (and prefs). So we are ready for first review.
> 
> tests needed:
> 1. create a new profile, bookmarks are imported from default bookmarks.html
> 2. delete places.sqlite, bookmarks are restored from JSON backup
> 3. delete all profile folder contents, bookmarks are imported from default
> bookmarks.html
> 4. corrupt places.sqlite with an hex editor, bookmarks are restored from JSON
> backup, places.sqlite.corrupt is created
> 5. migrate from 3.0 branch profile, bookmarks retained
> 6. migrate from 2.0 branch profile, bookmarks retained
> 7. users asks to import from bookmarks.html through importBookmarksHTML config
> 8. in safe mode user asks to restore default bookmarks
> 
> at each step should check for coherence:
> 1. Bookmarks toolbar, Bookmarks menu, Smart bookmarks, Library
> 
> QA may be aware of more test paths though, most likely this will need a QA test
> session, since testing with browser restart is not feasible.

Tracy, in case this isnt on your radar yet, can you cross check if our current litmus subset covers any of these scenarios?  If not, can we have them added?  Thanks.
FWIW, i was talking to marco on irc about automation opportunities here, but i think mozmill is only capable of handling maybe #1, #5, #7, #8.   These however, aren't trivial js tests to write, which i think we should check these manually in the short time we have.

The remaining ones listed are outside of the chrome, which mozmill doesnt support handling natively to the OS.  

For future reference though, the last test of checking input in the toolbar, menu, library, and smart bookmarks should be a good testcase we should create for future smoketests.   The default list for en-us rarely changes, so we can use mozmill to diff profiles at the end of the test.
Keywords: dev-doc-needed
Attached patch patch v1.1Splinter Review
fixed review comments, asking explicit approval to land for b2
Attachment #347525 - Attachment is obsolete: true
Attachment #347668 - Flags: approval1.9.1b2?
Comment on attachment 347668 [details] [diff] [review]
patch v1.1

a=beltzner, but it's not needed; beta 2 blockers are automatically approved.
Attachment #347668 - Flags: approval1.9.1b2? → approval1.9.1b2+
Whiteboard: [needs QA testing][has review] → [needs QA testing][has approval]
http://hg.mozilla.org/mozilla-central/rev/8c6d2ac4b1c4
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 464767
Depends on: 469301
Depends on: 474582
updated MDC documentation
Whiteboard: [needs QA testing][has approval]
Bug is outside the realm Import subgroup on litmus FFT. Setting in-litmus- flag.
Flags: in-litmus? → in-litmus-
most of this actually should be covered by automatic tests added with bug 474582
(In reply to comment #46)
> most of this actually should be covered by automatic tests added with bug
> 474582

Marking verified fixed based on passing unit tests on trunk and 1.9.1.
Status: RESOLVED → VERIFIED
Blocks: 484634
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
The "Read Only" attribute activates by itself.
Delete that last post, I messed up... thanks.
You need to log in before you can comment on or make changes to this bug.