1353041 - Bookmarks fail to import from Safari to Firefox

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

[Affected versions]:
- Firefox 53 beta 8
- latest Developer Edition 54.0a2
- latest Nightly 55.0a1

[Affected platforms]:
- macOS 10.12.2

[Steps to reproduce]:
1. Have some Bookmarks in Safari
2. Open Firefox
3. Click on Show your Bookmarks
4. Click on Show All Bookmarks
5. Click Import and Backup your Bookmarks
6. Click on Import Data from Another Browser
7. Select Safari from Import Wizard and click Continue
8. Select Bookmarks and click Continue
9. Go to Bookmarks Toolbar and click From Safari folder

[Expected result]:
- Depending on how many bookmarks are in Safari, the import Wizard will show that the bookmarks are imported and then that the import is complete. All bookmarks from Safari are successfully imported.

[Actual result]:
- Bookmark import fails.

[Regression range]:
- This is a recent regression. Here is the output from mozregression:

Last good revision: 8dd496fd015a2b6e99573070279d9d1593836ea9 (2017-03-15)
First bad revision: ff04d410e74b69acfab17ef7e73e7397602d5a68 (2017-03-16)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8dd496fd015a2b6e99573070279d9d1593836ea9&tochange=ff04d410e74b69acfab17ef7e73e7397602d5a68

Probably caused by:
ce47c3bc026a	Gijs Kruitbosch — Bug 1344759 - batch-insert bookmarks when importing from Edge, IE, Safari or Chrome, r=dao

[Additional notes]:
- Here is the output from Browser console and Terminal:
https://pastebin.com/Q0836de4
- This is Mac only and Firefox 52.0.2 is not affected by this.

Comment 1

[:Gijs (he/him)]

Bogdan: thanks so much for finding this.

Paging Liz because this'll want uplift.

This only happens if you have folders to import, and OF COURSE the existing automated test didn't test that codepath because this week is my lucky week. :-(

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1353041 - fix Safari import of folders,

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

Comment 3

[:Gijs (he/him)]

Comment on attachment 8854063 [details]
Bug 1353041 - fix Safari import of folders,

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

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 8854063 [details]
Bug 1353041 - fix Safari import of folders,

https://reviewboard.mozilla.org/r/126042/#review128624

::: browser/components/migration/SafariProfileMigrator.js:187
(Diff revision 1)
>          if (entry.has("URIDictionary"))
>            title = entry.get("URIDictionary").get("title");
>          return { url, title };
>        }
>        return null;
> -    }).filter(e => !!e);
> +    }.bind(this)).filter(e => !!e);

You can just pass |this| as the second argument to map

::: browser/components/migration/tests/unit/test_Safari_bookmarks.js:23
(Diff revision 1)
>    let bmObserver = {
>      onItemAdded(aItemId, aParentId, aIndex, aItemType, aURI, aTitle) {
>        if (aTitle != label) {
>          itemCount++;
>        }
> +      do_print("Got: " + aTitle + "\n");

Remove the logging please.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8854063 [details]
Bug 1353041 - fix Safari import of folders,

https://reviewboard.mozilla.org/r/126042/#review128634

Comment 6

[:Gijs (he/him)]

Comment on attachment 8854063 [details]
Bug 1353041 - fix Safari import of folders,

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be3f73f200a9
fix Safari import of folders, r=mossop

Comment 8

[:Gijs (he/him)]

Comment on attachment 8854063 [details]
Bug 1353041 - fix Safari import of folders,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1344759
[User impact if declined]: users with folders in their safari bookmarks won't be able to import anything more after those folders
[Is this code covered by automated tests?]: it is now. I thought it was before, or we would have caught this. My own safari bookmarks are the default, which means no folders, or I would have caught this when testing the patches manually... :-\
[Has the fix been verified in Nightly?]: by me, but I wrote the patch, so...
[Needs manual test from QE? If yes, steps to reproduce]: see comment #0.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no.
[Why is the change risky/not risky?]: The actual code change is just 1 line, and comes with updates to the tests so that this time we notice if stuff breaks. :-\
[String changes made/needed]: nope

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/be3f73f200a9

Comment 10

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8854063 [details]
Bug 1353041 - fix Safari import of folders,

Last minute regression fix, let's not leave out Safari!

Comment 11

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Bogdan would you mind re-testing for Safari once this lands in beta 10 later this week? Thanks.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/17f121d897d2

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/168d76e8a965

Comment 14

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

I can confirm this is fixed on 53.0 RC. (sorry for being so late).