1226556 - Fix Microsoft Edge bookmark import now that they are stored in EDB database (like Reading List)

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (he/him)]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from bug 1225466 comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > Can you elaborate on [the failed bookmark import]? When did you create the bookmarks and when did
> > you delete them? Your comment didn't specify.
> 
> Before performing the migration process I deleted a few favorites and added
> new ones in Edge, but in Firefox are imported another bookmarks, such as
> very old ones.
> See screenshot: http://i.imgur.com/HMcT6cy.jpg

We should look at what's going on here.

Comment 1

[:Gijs (he/him)]

It looks like favorites are now no longer saved in the location where they used to be saved. I expect Edge is now using the same database as it is for History (see bug 1192034).

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #1)
> It looks like favorites are now no longer saved in the location where they
> used to be saved. I expect Edge is now using the same database as it is for
> History (see bug 1192034).

Hm, looks like it's actually the same db as the reading list rather than webcache. Which is good... but still is going to take work to get the import right here...

Comment 3

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1226556 - part 1: allow reading GUID columns in ESEDBReader.jsm, r=MattN

Review commit: https://reviewboard.mozilla.org/r/32931/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32931/

Comment 4

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1226556 - part 2: use ESE database to import Edge bookmarks, r=MattN

Review commit: https://reviewboard.mozilla.org/r/32933/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32933/

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8714100 [details]
MozReview Request: Bug 1226556 - part 1: allow reading GUID columns in ESEDBReader.jsm, r=MattN

https://reviewboard.mozilla.org/r/32931/#review29923

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8714101 [details]
MozReview Request: Bug 1226556 - part 2: use ESE database to import Edge bookmarks, r=MattN

https://reviewboard.mozilla.org/r/32933/#review29925

The more we touch ESE code the more I wish we implemented tests… please make sure this gets manual verification and continues to be tested by QE during the Beta cycle. I guess we also should uplift this somewhat after verification.

::: browser/components/migration/EdgeProfileMigrator.js:47
(Diff revision 1)
> + * @param tableName the name of the table to read.
> + * @param columns   a list of column specifiers (see ESEDBReader.jsm) or a function that
> + *                  generates them based on the database reference once opened.
> + * @param filterFn  a function that is called for each row. Only rows for which it returns

Can you add types to the JSDoc e.g.:
@param {String[]|function} columns…

::: browser/components/migration/EdgeProfileMigrator.js:367
(Diff revision 1)
> -    MSMigrationUtils.getBookmarksMigrator(MSMigrationUtils.MIGRATION_TYPE_EDGE),
> +    new EdgeBookmarksMigrator(),

Do we need to support users on older versions of Windows/Edge? If so, we could try the new code path and fallback to old. If not, we should at least comment on MSMigrationUtils.jsm Bookmark constructor to note that MIGRATION_TYPE_EDGE is no longer used.

Comment 7

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #6)
> Do we need to support users on older versions of Windows/Edge?

I thought the answer was "no", but it seems the update from an older Windows version to Windows 10 will initially not get you the November update where I believe all this changed. See the note at the top of:

http://windows.microsoft.com/en-gb/windows-10/upgrade-to-windows-10-faq

So I will likely have to make some way of determining what's going on and picking the right bookmarks implementation. :-(

As for tests, I just checked, and it seems that while these databases are huge, they compress down to about 60k-odd with just plain windows put-in-zip compression, so hopefully I can write some tests that use that to get reasonable-size testcases in the tree, or something.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8714100 [details]
MozReview Request: Bug 1226556 - part 1: allow reading GUID columns in ESEDBReader.jsm, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32931/diff/1-2/

Comment 9

[:Gijs (he/him)]

Comment on attachment 8714101 [details]
MozReview Request: Bug 1226556 - part 2: use ESE database to import Edge bookmarks, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32933/diff/1-2/

Comment 10

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1226556 - part 3: add tableExists to ESEDBReader, use it to fall back to normal bookmarks migration, r?MattN

Review commit: https://reviewboard.mozilla.org/r/33379/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33379/

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8715306 [details]
MozReview Request: Bug 1226556 - part 3: add tableExists to ESEDBReader, use it to fall back to normal bookmarks migration, r?MattN

https://reviewboard.mozilla.org/r/33379/#review30551

Sorry for the delay

::: browser/components/migration/ESEDBReader.jsm:328
(Diff revision 1)
> +    if (!this._opened) {
> +      throw "The database was closed!";
> +    }

I thought we're always supposed to throw an `Error` instead of string so we get a stack. That probably deserves as eslint rule if it's still the case.

Comment 12

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #11)
> Comment on attachment 8715306 [details]
> MozReview Request: Bug 1226556 - part 3: add tableExists to ESEDBReader, use
> it to fall back to normal bookmarks migration, r?MattN
> 
> https://reviewboard.mozilla.org/r/33379/#review30551
> 
> Sorry for the delay

No worries!

> ::: browser/components/migration/ESEDBReader.jsm:328
> (Diff revision 1)
> > +    if (!this._opened) {
> > +      throw "The database was closed!";
> > +    }
> 
> I thought we're always supposed to throw an `Error` instead of string so we
> get a stack. That probably deserves as eslint rule if it's still the case.

Fixed, as well as other occurrences in this file. And yes, it probably could do with eslint support - though note that none of these files are currently linted. :-(

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5b122e90478e
https://hg.mozilla.org/integration/fx-team/rev/d572ab4f2f11
https://hg.mozilla.org/integration/fx-team/rev/c5685dc73b4e

Comment 14

[:Gijs (he/him)]

Ryan, this basically fixes bookmark import from Edge after the November Windows 10 update. I would really like to uplift this (and bug 1229076 and bug 1237679) to 45 (beta, and going to be ESR!), but I would feel better about doing that if it had had some QA attention to verify there aren't gaping holes in what I implemented. Do you think that is feasible?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Rares, can you please assign this to someone on your team. Alternatively, we may want Vasilica to look at this since she already has experience testing this feature. I'll leave that to you and Florin to decide :)

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b122e90478e
https://hg.mozilla.org/mozilla-central/rev/d572ab4f2f11
https://hg.mozilla.org/mozilla-central/rev/c5685dc73b4e

Comment 17

[Simona Badau]

I could not reproduce the issue exactly as stated in the Description - unfortunately I didn't had any old bookmarks in Edge to work with.

Here are the results I got:

=>  When migration was done on the latest Nightly (Build ID: 20160209025057), none of the bookmarks saved under Favorites or under Favorites Bar were imported. The Reading list (from Edge) items were the only ones imported under the Bookmarks Menu. 

=> When migration was done on an fx-team build that contained the commits from Comment 13 (downloaded from http://archive.mozilla.org/pub/firefox/tinderbox-builds/fx-team-win32/1455015057/ ), all the bookmarks were imported as following: 
- The Bookmarks saved under Favorites in Edge were imported in Firefox under the Bookmarks Menu.
- The Bookmarks saved under Favorites Bar in Edge were imported in Firefox under the Bookmarks Toolbar section.
- The Reading list items from Edge were also imported under the Bookmarks Menu section.

Comment 18

[:Gijs (he/him)]

(In reply to Simona B from comment #17)
> I could not reproduce the issue exactly as stated in the Description -
> unfortunately I didn't had any old bookmarks in Edge to work with.
> 
> Here are the results I got:
> 
> =>  When migration was done on the latest Nightly (Build ID:
> 20160209025057), none of the bookmarks saved under Favorites or under
> Favorites Bar were imported. The Reading list (from Edge) items were the
> only ones imported under the Bookmarks Menu. 
> 
> => When migration was done on an fx-team build that contained the commits
> from Comment 13 (downloaded from
> http://archive.mozilla.org/pub/firefox/tinderbox-builds/fx-team-win32/
> 1455015057/ ), all the bookmarks were imported as following: 
> - The Bookmarks saved under Favorites in Edge were imported in Firefox under
> the Bookmarks Menu.
> - The Bookmarks saved under Favorites Bar in Edge were imported in Firefox
> under the Bookmarks Toolbar section.
> - The Reading list items from Edge were also imported under the Bookmarks
> Menu section.

I mean, works for me! Let's get this sorted for 45, then. Even if we missed something here, working some of the time > working none of the time.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8715306 [details]
MozReview Request: Bug 1226556 - part 3: add tableExists to ESEDBReader, use it to fall back to normal bookmarks migration, r?MattN

Approval Request Comment
[Feature/regressing bug #]: Windows 10 update broke this, feature being importing bookmarks from Edge
[User impact if declined]: No (or very much outdated) bookmarks imported from Edge
[Describe test coverage new/current, TreeHerder]: nope. :-(
[Risks and why]: low risk. Not because the code is perfect, but because what we're improving upon is "it doesn't work". Improving "it doesn't work" is not hard. The code itself is limited to the Edge importer, so it's hard to imagine it would have an impact outside that.
[String/UUID change made/needed]: no

Note that I'm requesting uplift for all 3 patches on this bug, and will also be requesting uplift for 2 bugs that are required to make this work right: bug 1237679 and bug 1229076.

Comment 20

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 47.0a1 (2016-02-01) under Windows 10 64-bit.

Verified fixed on Firefox 47.0a1 (2016-02-09) under Windows 10 64-bit. All bookmarks, both old and new ones, are successfully imported from Edge.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8715306 [details]
MozReview Request: Bug 1226556 - part 3: add tableExists to ESEDBReader, use it to fall back to normal bookmarks migration, r?MattN

Improve the import, taking it.
Should be in 45 beta 5

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eca358321df0
https://hg.mozilla.org/releases/mozilla-aurora/rev/597d12f469ac
https://hg.mozilla.org/releases/mozilla-aurora/rev/1d99a7907e32

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This isn't applying correctly to beta. Can we get rebased patches for it?

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Ah, this just needed bug 1229076 to get uplifted first.
https://hg.mozilla.org/releases/mozilla-beta/rev/662307603fde
https://hg.mozilla.org/releases/mozilla-beta/rev/693a12195711
https://hg.mozilla.org/releases/mozilla-beta/rev/1e3087f1e782

Comment 25

[Cornel Ionce [:noni] [Hubs QA]]

We've also tested this issue using Fx 45 beta 8 (build ID 20160221141421) and 2015-02-23 Aurora 46.0a2 on Windows 10x64.
Every bookmark is now imported correctly from Edge, so we can consider this issue verified.