Reading List articles are no longer imported from Edge to Firefox

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bogdan_maris, Assigned: Gijs)

Tracking

({regression})

Trunk
Firefox 55
All
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 verified, firefox54 fixed, firefox55 fixed)

Details

Attachments

(1 attachment)

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

[Affected platforms]:
- Windows 10

[Steps to reproduce]:
1. Add some articles in Edge reading list
2. Start Firefox
3. Import bookmarks from another browser
4. Check the folder Reading list (from Edge) from Library

[Expected result]:
- Reading list articles are imported in Fx.

[Actual result]:
- Reading list (from Edge) folder is empty.

[Regression range]:
- This is a recent regression:

Mozregression gui output:

https://hg.mozilla.org/integration/autoland/json-pushes?changeset=ce47c3bc026a7d6357d5c007a34ac69f1a68d946&full=1

Bug 1344759 - batch-insert bookmarks when importing from Edge, IE, Safari or Chrome, r=dao
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → gijskruitbosch+bugs
Blocks: 1344759
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8854477 [details]
Bug 1353373 - fix Edge readinglist imports,

https://reviewboard.mozilla.org/r/126414/#review129044

::: browser/components/migration/EdgeProfileMigrator.js:217
(Diff revision 1)
>        } catch (ex) {
>          continue;
>        }
>        bookmarks.push({ url: item.URL, title: item.Title, dateAdded });
>      }
> -    yield MigrationUtils.insertManyBookmarksWrapper(readingListItems, destFolderGuid);
> +    yield MigrationUtils.insertManyBookmarksWrapper(bookmarks, destFolderGuid);

It's too bad that no-unused-vars doesn't catch this (because bookmarks.push() is called).
Attachment #8854477 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> It's too bad that no-unused-vars doesn't catch this (because
> bookmarks.push() is called).

This is arguably still an eslint bug because:
1) nobody assigns to bookmarks, so it can know it's an array, and so .push() is the 'real' .push() and has no side effects beyond modifying the array
2) .push is a write-only operation, and the result is unused
3) hence, nobody reads the array.

I'll file an eslint issue, I guess.
Comment on attachment 8854477 [details]
Bug 1353373 - fix Edge readinglist imports,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1344759
[User impact if declined]: we don't import reading list items from Edge
[Is this code covered by automated tests?]: well, I thought it was before, but it definitely is now...
[Has the fix been verified in Nightly?]: I wrote automated tests, verified they caught this issue, fixed the issue, verified tests passed, and manually tested myself - but then, I wrote all the code, so ymmv...
[Needs manual test from QE? If yes, steps to reproduce]: see comment #0
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: I realize the patch looks big, but this is effectively a 1-line change (the line highlighted in comment 2), where I passed the wrong thing to an API after a small code refactor from an "r+ with these comments addressed" in bug 1344759. And accidentally broke the entire import of reading list items. I thought for the refactor, just running automated tests would be sufficient, and clearly I was wrong. Anyway, all the other changes are to (facilitate) test that it ACTUALLY WORKS so that I (or someone else) don't get caught out by this again. I tested both that the new tests I wrote would have caught the issue, that they pass now, and I manually tested reading list items were imported correctly. So no, I don't think it's risky. I am, however, very very sorry for missing this the first time around and for the extra work this is now causing. :-\
[String changes made/needed]: none
Attachment #8854477 - Flags: approval-mozilla-beta?
Attachment #8854477 - Flags: approval-mozilla-aurora?
(In reply to :Gijs from comment #4)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> > It's too bad that no-unused-vars doesn't catch this (because
> > bookmarks.push() is called).
> 
> This is arguably still an eslint bug because:
> 1) nobody assigns to bookmarks, so it can know it's an array, and so .push()
> is the 'real' .push() and has no side effects beyond modifying the array
> 2) .push is a write-only operation, and the result is unused
> 3) hence, nobody reads the array.
> 
> I'll file an eslint issue, I guess.

https://github.com/eslint/eslint/issues/8414
https://hg.mozilla.org/mozilla-central/rev/5bd8f7158ace
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment on attachment 8854477 [details]
Bug 1353373 - fix Edge readinglist imports,

One more.... we are now arguing about whether we should back out the whole deal. I don't want to have to fix more stuff in the RC(s)
Attachment #8854477 - Flags: approval-mozilla-beta?
Attachment #8854477 - Flags: approval-mozilla-beta+
Attachment #8854477 - Flags: approval-mozilla-aurora?
Attachment #8854477 - Flags: approval-mozilla-aurora+
Bogdan, could you please take a look at this on 53?
Flags: needinfo?(bogdan.maris)
Verified fixed using Firefox 53.0 RC build 6 on Windows 10 64bit. Firefox is successfully importing Reading list items from Edge.
Flags: needinfo?(bogdan.maris)
You need to log in before you can comment on or make changes to this bug.