474582 - With initial migration Shiretoko doesn't import favorites from Internet Explorer 7 anymore

Status: VERIFIED FIXED

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

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090104 Shiretoko/3.1b3pre ID:20090104034401

Having a clean system without any previously installed versions of Firefox the initial import of the favorites from Internet Explorer 7 fail. There is nothing imported at all. This is a regression from Firefox 3.0.0.6.

This will mostly affect any new user who is willed to switch from Internet Explorer to Firefox and will be annoyed by this failure. Running a further import afterward, imports the favorites correctly.

Steps:
1. Rename an existing Mozilla folder under %Appdata%
2. Start Shiretoko and select the migration from Internet Explorer
3. Close the dialog when it's ready
4. Open the Bookmarks menu
5. Select "File - Import" and re-run the import
6. Do step 3 and step 4 again

With step 4 you will see that no favorites from IE were imported. Only the default bookmarks of Firefox 3.1 are visible.

With step 6 all the favorites are now imported into the IE subfolder under the Bookmarks Menu.

Comment 1

[Robert Strong (they/them - no direct email)]

Marco, the calls to InsertBookmark appear to be succeeding but the bookmarks are never imported. Can you take a look at this?

Comment 2

[Marco Bonardo [:mak]]

yes, i can test migration from ie7, i'll take a look if i can find where we fail

Comment 3

[Marco Bonardo [:mak]]

indeed this is not a bug in the migrator, it's an issue in nsBrowserGlue, migration happens BEFORE initPlaces in browserGlue, we detect a new database has been created and we overwrite imported bookmarks.

moving to Places, i agree this should block.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Actually the migrators are still using the old importBookmarksHTML pref approach :\ We can detect if the database has been created but we already have bookmarks, and don't overwrite them in case.

first look, i guess if there's a way to test this.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.1

getIdForItemAt throws if the item does not exists, instead it should probably return -1 and throw if we pass an invalid folder or something other goes wrong.
Could be late-compat due to this change :(

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.2

humpf, checking for folder could be a perf hit, return -1 in case the item does not exist, nothing more.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 358601 [details] [diff] [review]
patch v1.2

we should not import at all instead. new patch coming, i've 4 new tests for our initialization in browserGlue and a unit test for getIdForItemAt.

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v2.0 + tests

Here is first version with tests. i can probably add some other test, will look.

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v2.1

Added another couple tests, and some cleanup

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 359511 [details] [diff] [review]
patch v2.1

>diff --git a/browser/components/migration/src/nsSeamonkeyProfileMigrator.cpp b/browser/components/migration/src/nsSeamonkeyProfileMigrator.cpp
>--- a/browser/components/migration/src/nsSeamonkeyProfileMigrator.cpp
>+++ b/browser/components/migration/src/nsSeamonkeyProfileMigrator.cpp
>@@ -738,23 +738,16 @@ nsSeamonkeyProfileMigrator::CopyBookmark
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     // Merge in the bookmarks from the source profile
>     nsCOMPtr<nsIFile> sourceFile;
>     mSourceProfile->Clone(getter_AddRefs(sourceFile));
>     sourceFile->Append(FILE_NAME_BOOKMARKS);
>     rv = ImportBookmarksHTML(sourceFile, PR_TRUE, PR_FALSE, EmptyString().get());
>     NS_ENSURE_SUCCESS(rv, rv);
>-
>-    // we need to set this pref so that on startup
>-    // we don't blow away what we just imported
>-    nsCOMPtr<nsIPrefBranch> pref(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    return pref->SetBoolPref("browser.places.importBookmarksHTML", PR_FALSE);
>   }
> 
>   return ImportNetscapeBookmarks(FILE_NAME_BOOKMARKS, 
>                                  NS_LITERAL_STRING("sourceNameSeamonkey").get());
> }

you removed the early return. please change this code to not return early.

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js
>@@ -504,16 +504,28 @@ BrowserGlue.prototype = {
>                   getService(Ci.nsINavHistoryService);
> 
>     // If the database is corrupt or has been newly created we should
>     // import bookmarks.
>     var databaseStatus = histsvc.databaseStatus;
>     var importBookmarks = databaseStatus == histsvc.DATABASE_STATUS_CREATE ||
>                           databaseStatus == histsvc.DATABASE_STATUS_CORRUPT;
> 
>+    if (databaseStatus == histsvc.DATABASE_STATUS_CREATE) {
>+      // If the database has just been created, but we already have any
>+      // bookmark, this is not the initial import.  This can happen after a
>+      // migration from a different browser since migrators run before us.
>+      // In such a case we should not import, unless some pref has been set.
>+      var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+                  getService(Ci.nsINavBookmarksService);
>+      if (bmsvc.getIdForItemAt(bmsvc.bookmarksMenuFolder, 0) != -1 ||
>+          bmsvc.getIdForItemAt(bmsvc.toolbarFolder, 0) != -1)
>+        importBookmarks = false;
>+    }
>+

hrm, do not like the fragility of this kind of implicit detection. i'd prefer something explicit that signaled that a migration has occurred. see if the migration or other first-run code sets any flags for detecting this. i'll be very surprised if there's no way to detect a first-run. however, that might not be enough, as the user can decide to not import anything. maybe we should add a new notification for the migrators.

Comment 11

[Marco Bonardo [:mak]]

there's a problem with notifications, as i anticipated through IRC, profile migrators starts in nsAppRunner.cpp, very early. "profile-after-change" is called after profile migrators, so when migrators run at very first start, Places is not initialized by nsPlacesDBFlush but from the migrator itself.
This has 2 drawbacks:
- We are not syncing during migration, this means if we crash during migration we end up with bookmarks orphan of a moz_place. I'll file a separate bug for this.
- If we notify during migration the notification will be enqueued after places-init-complete (notified at first Places use), so it will be server after we import in nsBrowserGlue.
Due to this the only way would be to set a pref (like it was before), ideally we could also set importBookmarksHTML to false since it's not a default pref, and detect if the pref exists, but this would be even more fragile.

Per IRC i'm going to fix the early return but retain the current implementation that looks for pre-existing bookmarks, also as a first protection against dataloss.

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v2.2

Comment 13

[Marco Bonardo [:mak]]

filed bug 476220 about the sync issue.

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 359822 [details] [diff] [review]
patch v2.2


>+/**
>+ * Creates a bookmarks.html file in the profile folder.

add "from a given source file."

>+ *
>+ * @param aFilename
>+ *        Name of the file to copy to the profile folder.  This file must
>+ *        exist in the test directory.

s/test directory/directory that the test files are in/

(since the parent dir of all of our tests is "test")

>+/**
>+ * Creates a JSON backup in the profile folder.
>+ *
>+ * @param aFilename
>+ *        Name of the file to copy to the profile folder.  This file must
>+ *        exist in the test directory.
>+ *
>+ * @return nsIFile object for the file.
>+ */

same comments from create_bookmarks_html()

>+function create_JSON_backup(aFilename) {
>+  remove_all_JSON_backups();
>+  let bookmarksBackupDir = gProfD.clone();
>+  bookmarksBackupDir.append("bookmarkbackups");
>+  if (!bookmarksBackupDir.exists()) {
>+    bookmarksBackupDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0700);
>+    do_check_true(bookmarksBackupDir.exists());
>+  }

just make it world-writable. we have enough weird issues on the text boxes.

>+  let date = new Date().toLocaleFormat("%Y-%m-%d");
>+  const FILENAME_BOOKMARKS_JSON = "bookmarks-" + date + ".json";

make it global like the html name, since it's redefined and used elsewhere.

>+/**
>+ * Tests that nsBrowserGlue correctly restores default bookmarks if database is
>+ * corrupt but a JSON backup is not available.
>+ */

should add a test that bookmarks.html in the profile dir gets imported if no backup. only if there's no bookmarks.html will default bookmarks be imported.

>diff --git a/browser/components/places/tests/unit/test_browserGlue_shutdown.js b/browser/components/places/tests/unit/test_browserGlue_shutdown.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/components/places/tests/unit/test_browserGlue_shutdown.js

>+/**
>+ * Tests that nsBrowserGlue is correctly interpreting the preferences settable
>+ * by the user or by other components.
>+ */

need to update comment to indicate this is about shutdown.

>+// Initialize naBrowserGlue.
>+let bg = Cc["@mozilla.org/browser/browserglue;1"].
>+         getService(Ci.nsIBrowserGlue);

typo: naBrowserGlue

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v2.3

fixed dietrich's comments, added the required additional test.

Comment 16

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/1cf6c5b25fe5

Comment 17

[Marco Bonardo [:mak]]

backed out due to strange crashes on Linux and Mac unit test boxes.

http://hg.mozilla.org/mozilla-central/rev/ae2b920a712c
http://hg.mozilla.org/mozilla-central/rev/5a924dc19720 (merge)

Comment 18

[Marco Bonardo [:mak]]

tests were not crashing on my custom build, but doing a clobber build i can now reproduce the crash. looks like something has been pushed that now causes my tests to crash when i close the database connection...
i'm looking through recent storage changes to see if they are related.

We could take the fix and wait to fix the tests in the meantime.

Comment 19

[Marco Bonardo [:mak]]

the crash is exactly caused by:
pip.DBConnection.close();

Comment 20

[Marco Bonardo [:mak]]

crashes exactly are due to bug 476292, exactly when i close the db connection we could still try to call recalculateFrecencies due to the fact we have not yet executed that event.
I'll add some code to consume pending events on the mainThread, but also we should stop observing places-init-complete if we are going to close the connection.

Comment 21

[Marco Bonardo [:mak]]

Attachment: patch v2.4

So, i've added the consume events loop to tail_bookmarks, but i've also fixed commitPendingChanges so it will forcibly call places-init-complete observers, that could make sense since we are asking to commit all pending changes to the db, and that is most likely one of the pending changes.
Notice the latter is enough to fix the crashes, but i've still put both for clarity, so future changes will notice that.

Shawn, could you please take a look at the interdiff between this patch and the previous and tell me what you think?

Comment 22

[Dietrich Ayala (:dietrich)]

Comment on attachment 360552 [details] [diff] [review]
patch v2.4

r=me, assuming observers who remove themselves can do so w/o throwing, if they've already been removed.

Comment 23

[Marco Bonardo [:mak]]

Attachment: patch v2.5

alright, not going to remove observers, since that could throw. Ready to land, again.

Comment 24

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/1ced22c95fef

Comment 25

[Marco Bonardo [:mak]]

additional changeset to fix an issue with lastModifiedTime and linux unit box (looks like filestats are cached, so checking lastModifiedTime is not realiable)
http://hg.mozilla.org/mozilla-central/rev/dcb3845d3134

Comment 26

[Shawn Wilsher :sdwilsh]

Creating a new nsI[Local]File bypasses the cache.  The download manager has to do this as well.

Comment 27

[Marco Bonardo [:mak]]

(In reply to comment #26)
> Creating a new nsI[Local]File bypasses the cache.  The download manager has to
> do this as well.

ok, thanks for the hint!
actually the test is still doing a filesize comparison and that works as expected (and is enough to ensure the file is different), so test results should be ensured.
If this should create randomness in future then we could convert the test to use nsILocalFile. i'll take a look at next tinderboxes cycles.

Comment 28

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/33a365d53aa5
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e8093b1c996a

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk and 1.9.1 branch with:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090211 Minefield/3.2a1pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090205 Shiretoko/3.1b3pre

Marco, we still have different target folders for the import, depending if it happens on initial startup or a later import. With an initial import the folders are directly placed under the Bookmarks Menu, while with a later import we are using a subfolder. Is that behavior expected?

We have a litmus test which already covers the initial import. Setting in-litmus flag.

Comment 30

[Marco Bonardo [:mak]]

(In reply to comment #29)
> Marco, we still have different target folders for the import, depending if it
> happens on initial startup or a later import. With an initial import the
> folders are directly placed under the Bookmarks Menu, while with a later import
> we are using a subfolder. Is that behavior expected?

This is how things were designed, indeed the import function has a "initialImport" flag that sets exactly that behavior.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Alright. So I'm fine with that. I've checked this too on Windows 7 and the favorites get imported with Shiretoko and Minefield too.

Comment 32

[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