Closed Bug 1448920 Opened 6 years ago Closed 6 years ago

Browsing History item is not displayed anymore in the Import Wizard on the recent Insider Preview builds from Edge

Categories

(Firefox :: Migration, defect)

All
Windows 10
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 - wontfix
firefox61 - fixed

People

(Reporter: cgeorgiu, Assigned: Gijs)

References

Details

Attachments

(1 file)

[Affected versions]:
- latest Nightly 61.0a1
- Beta 60.0b6
- Release 59.0.2
- esr 52.7.3   

[Affected platforms]:
- Windows 10 x64 and x86 Version 1803 (OS Build 17128.1)

[Preconditions]:
- Edge is installed and closed
- browsing history
- saved favorites
- saved pages to reading list
- saved passwords

[Steps to reproduce]:
1. Launch Firefox on a clean profile with firefox -no-remote -migration parameter.
2. From the Import Wizard choose Microsoft Edge and click Next.

[Expected result]:
- The Browsing History item is not displayed in the Import Wizard, therefore the history is not imported

[Actual result]:
- Only the following items are imported: Cookies, Saved Passwords, Favorites

[Regression range]:
- Not a regression, I was able to reproduce it on 50.0b4 as well.

[Additional notes]:
- note that on esr 52.7.3 the Import Wizard migrates only Cookies and Favorites, please let me know if I should file a separate bug for this
- not reproducible on the current release version of Windows 10, Version 1709 (OS Build 16299.309)
(In reply to Ciprian Georgiu, QA [:ciprian_georgiu] from comment #0)
...
> [Expected result]:
> - The Browsing History item is not displayed in the Import Wizard, therefore
> the history is not imported

I have to make a correction here, as this is the current behavior, alongside with the one already written.

The right [Expected result]: Browsing History is displayed in the Import Wizard and the history is properly imported from Edge
Flags: needinfo?(gijskruitbosch+bugs)
Hi Gijs, Microsoft is expected to start rolling out the Spring Creators Update this week. Have you had a chance to look at this more?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #2)
> Hi Gijs, Microsoft is expected to start rolling out the Spring Creators
> Update this week. Have you had a chance to look at this more?

No. FWIW, I haven't (internally) prioritized this very highly because:

1) we no longer show the import dialog on startup. Ever. The user has to go find it (e.g. via History > Show All history > star button > Import...). I don't think a lot of people coming from Edge would bother going through all those steps. We do still show a prompt to import data from another browser in activity stream for new profiles, but I'm still skeptical that many people would choose to go through that (based on previous use of the import wizard, where the most popular action was to get out of it without any imports). That said, I don't have any *current* data about this.
2) even before this bug, the only history we imported was typed history (ie full URLs users literally typed into the urlbar - not navigations, or searches, or bookmark items - which for most users would be most of their history). So it was never very useful.
3) this isn't really manifesting as "broken" just as "it's not there/supported". So it's not something that users will miss. In that sense, it's almost better than the previous state which would have imported only a subset of what users might expect it to import. (Something about which, fwiw, I don't recall ever seeing complaints / bugs filed by users, which strengthens (1).
4) I *suspect* (but haven't verified) that the data has moved into the same database where the rest of the history data is. We've had significant difficulty trying to access data in that database (see https://bugzilla.mozilla.org/show_bug.cgi?id=1192034#c6 ). IE used to expose an API for history, and the last time I checked that API didn't return results from Edge. I'd be surprised if that had changed. Despite repeated requests to Microsoft both on non-public mailing lists and via direct email for a way to access this data, I'm not aware of any.

All of this means this hasn't exactly been at the top of my priority list. I don't think it should track for 60. If by some miracle the data is somewhere useful and it's safe/easy for us to access it, we could consider backporting something. But my tea leaf reading makes me skeptical.

I'll see if I can have a look tomorrow or later this week if relman is anxious to have a resolution here despite the points above.
Thanks, that's a really helpful comment. Agreed that it doesn't sound like this needs to be tracked then.
So the good news is it seems this data has moved into the spartan db that we already read for favourites and reading list items, which at least means we can fix the importer...
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
While I would have liked to add a test, I don't have cycles right now to create one, as it'd require using some ESE tools to create/update a test database to use.
See Also: → 1455116
Comment on attachment 8968962 [details]
Bug 1448920 - add support for importing Edge typed URL history from its database instead,

https://reviewboard.mozilla.org/r/237662/#review243644

::: browser/components/migration/EdgeProfileMigrator.js:178
(Diff revision 1)
> +      {name: "AccessDateTimeUTC", type: "date"},
> +    ];
> +
> +    let typedUrls = [];
> +    try {
> +      typedUrls = readTableFromEdgeDB("TypedUrls", columns, () => true, this.db);

nit: it sounds a bit crazy that filterFn is mandatory it's a pointless function call for each row, when the code in readTableFromEdgeDB could just do:
if (!filterFn || filterFn(row))

In all the previous usages the function was used, this is the first case where it's pointless.

::: browser/components/migration/EdgeProfileMigrator.js:184
(Diff revision 1)
> +    } catch (ex) {
> +      // Maybe the table doesn't exist (older versions of Win10).
> +      // Just fall through and we'll return because there's no data.
> +    }
> +    if (!typedUrls.length) {
> +      return;

Can we somehow make the exception catch more specific, or maybe reportError regardless, because this is hiding any possible errors, that means this could start failing and nobody will notice.

ESEDB exposes a tableExists, thus potentially the Error could be customized for that case.

::: browser/components/migration/EdgeProfileMigrator.js:189
(Diff revision 1)
> +      return;
> +    }
> +
> +    let pageInfos = [];
> +    // Sometimes the values are bogus (e.g. 0 becomes some date in 1600),
> +    // and places will throw *everything* away, not just the bogus ones,

We could maybe add an option to ignore bogus entries in insertMany, if useful. What happens currently is normal input validation, if one of the entries is bogus, input is bogus and we don't want to hide possible mistakes from the caller.

::: browser/components/migration/EdgeProfileMigrator.js:190
(Diff revision 1)
> +    }
> +
> +    let pageInfos = [];
> +    // Sometimes the values are bogus (e.g. 0 becomes some date in 1600),
> +    // and places will throw *everything* away, not just the bogus ones,
> +    // so deal with that by having a cutoff date.

TL;DR: I'd probably import only entries more recent than a cutoff date, empty dated entries could be imported with the cutoff date.

From the comment (and var name) it looks like we throw away anything that is older than cutoff, rather than converting those dates. Maybe rename to kMinDate, if that's the case?
Also, if dates are older than cutoff, maybe they should get the cutoff date, rather than now? Do we have evidence those are new enough to go over entries with a real recent date?

I'd suggest to have a more realistic cutoff date that could either be:
1. 2016/01/01 - Edge was released half 2016
2. 6 months ago, because if the Places db is full we'll likely expire anything older as soon as it's added. On the other side, it's unlikely for a user to import into a full Places db, so it may not matter in practice.
Attachment #8968962 - Flags: review?(mak77) → review+
Comment on attachment 8968962 [details]
Bug 1448920 - add support for importing Edge typed URL history from its database instead,

https://reviewboard.mozilla.org/r/237662/#review244260

::: browser/components/migration/EdgeProfileMigrator.js:178
(Diff revision 1)
> +      {name: "AccessDateTimeUTC", type: "date"},
> +    ];
> +
> +    let typedUrls = [];
> +    try {
> +      typedUrls = readTableFromEdgeDB("TypedUrls", columns, () => true, this.db);

Fixed by making this optional.

::: browser/components/migration/EdgeProfileMigrator.js:184
(Diff revision 1)
> +    } catch (ex) {
> +      // Maybe the table doesn't exist (older versions of Win10).
> +      // Just fall through and we'll return because there's no data.
> +    }
> +    if (!typedUrls.length) {
> +      return;

The helper will report the error. I made this clear in the comment. I also made sure the error message was more readable by checking what error code ESE returns if the table doesn't exist.

::: browser/components/migration/EdgeProfileMigrator.js:190
(Diff revision 1)
> +    }
> +
> +    let pageInfos = [];
> +    // Sometimes the values are bogus (e.g. 0 becomes some date in 1600),
> +    // and places will throw *everything* away, not just the bogus ones,
> +    // so deal with that by having a cutoff date.

Ignoring entries before the cutoff now, and using the cutoff date for entries without a date.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1f04da36cb83
add support for importing Edge typed URL history from its database instead, r=mak
https://hg.mozilla.org/mozilla-central/rev/1f04da36cb83
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.