Closed Bug 1353041 Opened 3 years ago Closed 3 years ago

Bookmarks fail to import from Safari to Firefox

Categories

(Firefox :: Migration, defect, major)

All
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- verified
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: bogdan_maris, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(1 file)

[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.
Flags: needinfo?(gijskruitbosch+bugs)
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. :-(
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
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 on attachment 8854063 [details]
Bug 1353041 - fix Safari import of folders,

https://reviewboard.mozilla.org/r/126042/#review128634
Attachment #8854063 - Flags: review?(dtownsend) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/be3f73f200a9
fix Safari import of folders, r=mossop
Blocks: 1344759
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
Attachment #8854063 - Flags: approval-mozilla-beta?
Attachment #8854063 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/be3f73f200a9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment on attachment 8854063 [details]
Bug 1353041 - fix Safari import of folders,

Last minute regression fix, let's not leave out Safari!
Attachment #8854063 - Flags: approval-mozilla-beta?
Attachment #8854063 - Flags: approval-mozilla-beta+
Attachment #8854063 - Flags: approval-mozilla-aurora?
Attachment #8854063 - Flags: approval-mozilla-aurora+
Bogdan would you mind re-testing for Safari once this lands in beta 10 later this week? Thanks.
Flags: needinfo?(bogdan.maris)
I can confirm this is fixed on 53.0 RC. (sorry for being so late).
Flags: needinfo?(bogdan.maris)
You need to log in before you can comment on or make changes to this bug.