1225800 - Investigate "item not found" errors returned by VaultGetItem on win10

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (out for now; he/him)]

+++ This bug was initially created as a clone of Bug #1225466 +++

From bug 1225466:

> Unable to get item: 1168 MSMigrationUtils.jsm:861:0
> some passwords did not successfully migrate.

I can reproduce this locally; assuming this is an ordinary HRESULT, it means ERROR_NOT_FOUND, per:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms681383%28v=vs.85%29.aspx

We should probably investigate why those items are in the enumeration we're using, if they're things we're interested in or not, and whether we should deal with this error specially (e.g. just ignore items for which this error is returned).

Comment 1

[:Gijs (out for now; he/him)]

On my local machine this is because of identification tokens that aren't for URLs, but for Twitter (?! I don't think I've ever logged in to Twitter on this machine...) and Skype (likewise). It seems like we should just check if we can make something of the URL listed for the password and bail early otherwise.

Vasilica, can you confirm on the machine(s) that you used for bug 1225466, if you go to control panel, search for "vault", click "Manage Web Credentials", the list includes non-URL items labeled "Skype" or "Twitter" or whatever, besides the ones you were expecting be imported?

Comment 2

[Vasilica Tamas, QA [:vtamas]]

(In reply to :Gijs Kruitbosch from comment #1)
> On my local machine this is because of identification tokens that aren't for
> URLs, but for Twitter (?! I don't think I've ever logged in to Twitter on
> this machine...) and Skype (likewise). It seems like we should just check if
> we can make something of the URL listed for the password and bail early
> otherwise.
> 
> Vasilica, can you confirm on the machine(s) that you used for bug 1225466,
> if you go to control panel, search for "vault", click "Manage Web
> Credentials", the list includes non-URL items labeled "Skype" or "Twitter"
> or whatever, besides the ones you were expecting be imported?

There are 3 such non-URL items, related to Skype: http://i.imgur.com/nGUWOMp.jpg

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > On my local machine this is because of identification tokens that aren't for
> > URLs, but for Twitter (?! I don't think I've ever logged in to Twitter on
> > this machine...) and Skype (likewise). It seems like we should just check if
> > we can make something of the URL listed for the password and bail early
> > otherwise.
> > 
> > Vasilica, can you confirm on the machine(s) that you used for bug 1225466,
> > if you go to control panel, search for "vault", click "Manage Web
> > Credentials", the list includes non-URL items labeled "Skype" or "Twitter"
> > or whatever, besides the ones you were expecting be imported?
> 
> There are 3 such non-URL items, related to Skype:
> http://i.imgur.com/nGUWOMp.jpg

Great, thanks for confirming. Then I can fix this.

Comment 4

[:Gijs (out for now; he/him)]

Attachment: MozReview Request: Bug 1225800 - only import items that have valid URLs, r?MattN

Bug 1225800 - only import items that have valid URLs, r?MattN

Comment 5

[:Gijs (out for now; he/him)]

The yield and catch were flagged by eslint when run with --no-ignore. I haven't adjusted the ignore set for these files because a number of other files in this directory cause issues. I can omit those changes here if you prefer. I'll likely update the other files so they parse normally in a separate bug if nobody beats me to it.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8697311 [details]
MozReview Request: Bug 1225800 - only import items that have valid URLs, r?MattN

https://reviewboard.mozilla.org/r/27549/#review25041

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4d478f1afdb9

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/4d478f1afdb9

Comment 9

[:Gijs (out for now; he/him)]

Comment on attachment 8697311 [details]
MozReview Request: Bug 1225800 - only import items that have valid URLs, r?MattN

Approval Request Comment
[Feature/regressing bug #]: Windows 10 updates with Skype and Twitter integration
[User impact if declined]: spurious errors in the error console
[Describe test coverage new/current, TreeHerder]: no :-(
[Risks and why]: low risk - it's early in the cycles, the patch is small, the patch doesn't add any new dependencies on outside code and is even more careful about catching error conditions (that is its point)
[String/UUID change made/needed]: no.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Vasilica, could you please confirm that the issues you mentioned in bug 1225466 including the spurious errors that Gijs mentioned in comment 9 are fixed as expected? This will help me uplift this sooner to Beta44. Thanks!

Comment 11

[:Gijs (out for now; he/him)]

(In reply to Ritu Kothari (:ritu) from comment #10)
> Vasilica, could you please confirm that the issues you mentioned in bug
> 1225466 including the spurious errors that Gijs mentioned in comment 9 are
> fixed as expected? This will help me uplift this sooner to Beta44. Thanks!

This bug only aims to fix the errors reported in the console for the passwords import, everything else is covered in other bugs.

Comment 12

[Vasilica Tamas, QA [:vtamas]]

Performed Exploratory testing around this bug and  I did no longer see the error related to password import.

But I noticed another errors:
[1] Failed to parse sessionstore state JSON to migrate tab groups: SyntaxError: JSON.parse: unexpected end of data at line 1 column 1 of the JSON data TabGroupsMigrator.jsm:44:0
- While importing from Edge, IE, Safari and Chrome

[2] Unexpected value type: object IEProfileMigrator.js:465:0
some settings did not successfully migrate.
- Only while importing from IE


Gijs, any thoughts about these errors?

Comment 13

[:Gijs (out for now; he/him)]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #12)
> Performed Exploratory testing around this bug and  I did no longer see the
> error related to password import.
> 
> But I noticed another errors:
> [1] Failed to parse sessionstore state JSON to migrate tab groups:
> SyntaxError: JSON.parse: unexpected end of data at line 1 column 1 of the
> JSON data TabGroupsMigrator.jsm:44:0
> - While importing from Edge, IE, Safari and Chrome

This is unrelated to importing, it's bug 1232677 which has to do with the tab groups removal migration code.

> 
> [2] Unexpected value type: object IEProfileMigrator.js:465:0
> some settings did not successfully migrate.
> - Only while importing from IE
> 
> 
> Gijs, any thoughts about these errors?

Well, it says "settings", so it's unrelated to passwords. Please file a separate bug for it.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8697311 [details]
MozReview Request: Bug 1225800 - only import items that have valid URLs, r?MattN

Taking it based on Vasilica's verification (new issues will be handled in separate bugs). Beta44+, Aurora45+

Comment 15

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

https://hg.mozilla.org/releases/mozilla-beta/rev/e1c63d16f05c

Comment 16

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/fc1f9e0e37f2

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e1c63d16f05c

Comment 18

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

While testing this I ran into bug 1233694.
The error mentioned in this issue were no longer noticed during testing on Windows 10 x64 using:
* Firefox 45.0b8, buildID 20160221141421 and
* 2016-02-23 Aurora 46.0a2.