Closed Bug 1225800 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Migration, defect)

All
Windows 10
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 46
Tracking Status
firefox44 --- fixed
firefox45 --- verified
firefox46 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

+++ 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).
Blocks: 1225466
No longer blocks: 1225798
No longer depends on: 1225466
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?
Flags: needinfo?(vasilica.mihasca)
(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
Flags: needinfo?(vasilica.mihasca)
(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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Bug 1225800 - only import items that have valid URLs, r?MattN
Attachment #8697311 - Flags: review?(MattN+bmo)
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 on attachment 8697311 [details]
MozReview Request: Bug 1225800 - only import items that have valid URLs, r?MattN

https://reviewboard.mozilla.org/r/27549/#review25041
Attachment #8697311 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/4d478f1afdb9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
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.
Attachment #8697311 - Flags: approval-mozilla-beta?
Attachment #8697311 - Flags: approval-mozilla-aurora?
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!
Flags: needinfo?(vasilica.mihasca)
(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.
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?
Flags: needinfo?(vasilica.mihasca)
(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 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+
Attachment #8697311 - Flags: approval-mozilla-beta?
Attachment #8697311 - Flags: approval-mozilla-beta+
Attachment #8697311 - Flags: approval-mozilla-aurora?
Attachment #8697311 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: cornel.ionce
You need to log in before you can comment on or make changes to this bug.