Closed
Bug 474582
Opened 16 years ago
Closed 16 years ago
With initial migration Shiretoko doesn't import favorites from Internet Explorer 7 anymore
Categories
(Firefox :: Bookmarks & History, defect, P2)
Tracking
()
VERIFIED
FIXED
Firefox 3.6a1
People
(Reporter: whimboo, Assigned: mak)
References
Details
(Keywords: addon-compat, regression, verified1.9.1)
Attachments
(1 file, 8 obsolete files)
63.75 KB,
patch
|
Details | Diff | Splinter Review |
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.
Flags: blocking-firefox3.1?
Comment 1•16 years ago
|
||
Marco, the calls to InsertBookmark appear to be succeeding but the bookmarks are never imported. Can you take a look at this?
Assignee | ||
Comment 2•16 years ago
|
||
yes, i can test migration from ie7, i'll take a look if i can find where we fail
Assignee | ||
Comment 3•16 years ago
|
||
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.
Component: Migration → Places
QA Contact: migration → places
Assignee | ||
Updated•16 years ago
|
Target Milestone: --- → Firefox 3.1
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Comment 4•16 years ago
|
||
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.
Assignee | ||
Comment 5•16 years ago
|
||
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 :(
Attachment #358598 -
Attachment is obsolete: true
Assignee | ||
Comment 6•16 years ago
|
||
humpf, checking for folder could be a perf hit, return -1 in case the item does not exist, nothing more.
Attachment #358600 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Attachment #358601 -
Attachment is patch: true
Attachment #358601 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Updated•16 years ago
|
Keywords: late-compat
Updated•16 years ago
|
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Priority: -- → P2
Assignee | ||
Updated•16 years ago
|
Whiteboard: [writing tests for browserGlue]
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 7•16 years ago
|
||
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.
Attachment #358601 -
Attachment is obsolete: true
Assignee | ||
Comment 8•16 years ago
|
||
Here is first version with tests. i can probably add some other test, will look.
Assignee | ||
Updated•16 years ago
|
Attachment #359437 -
Attachment is patch: true
Attachment #359437 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 9•16 years ago
|
||
Added another couple tests, and some cleanup
Attachment #359437 -
Attachment is obsolete: true
Attachment #359511 -
Flags: review?(dietrich)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [writing tests for browserGlue] → [needs review dietrich]
Comment 10•16 years ago
|
||
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.
Attachment #359511 -
Flags: review?(dietrich) → review-
Assignee | ||
Comment 11•16 years ago
|
||
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.
Assignee | ||
Comment 12•16 years ago
|
||
Attachment #359511 -
Attachment is obsolete: true
Attachment #359822 -
Flags: review?(dietrich)
Assignee | ||
Comment 13•16 years ago
|
||
filed bug 476220 about the sync issue.
Comment 14•16 years ago
|
||
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
Attachment #359822 -
Flags: review?(dietrich) → review+
Updated•16 years ago
|
Whiteboard: [needs review dietrich]
Assignee | ||
Comment 15•16 years ago
|
||
fixed dietrich's comments, added the required additional test.
Attachment #359822 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Whiteboard: [can land]
Assignee | ||
Comment 16•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: Firefox 3.1 → Firefox 3.2a1
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite? → in-testsuite+
Assignee | ||
Comment 17•16 years ago
|
||
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 18•16 years ago
|
||
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.
Assignee | ||
Comment 19•16 years ago
|
||
the crash is exactly caused by:
pip.DBConnection.close();
Assignee | ||
Comment 20•16 years ago
|
||
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.
Assignee | ||
Comment 21•16 years ago
|
||
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?
Attachment #360303 -
Attachment is obsolete: true
Attachment #360552 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [backed-out due to crashes][has new patch][needs review sdwilsh]
Comment 22•16 years ago
|
||
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.
Attachment #360552 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Updated•16 years ago
|
Whiteboard: [backed-out due to crashes][has new patch][needs review sdwilsh]
Assignee | ||
Comment 23•16 years ago
|
||
alright, not going to remove observers, since that could throw. Ready to land, again.
Attachment #360552 -
Attachment is obsolete: true
Assignee | ||
Comment 24•16 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [baking on trunk]
Assignee | ||
Comment 25•16 years ago
|
||
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•16 years ago
|
||
Creating a new nsI[Local]File bypasses the cache. The download manager has to do this as well.
Assignee | ||
Comment 27•16 years ago
|
||
(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.
Assignee | ||
Comment 28•16 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/33a365d53aa5
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e8093b1c996a
Keywords: fixed1.9.1
Whiteboard: [baking on trunk]
Reporter | ||
Comment 29•16 years ago
|
||
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.
Assignee | ||
Comment 30•16 years ago
|
||
(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.
Reporter | ||
Comment 31•16 years ago
|
||
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•15 years ago
|
||
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.
Description
•