1096787 - Reset Firefox losing saved passwords

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[Daniel Dawson]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141105223254

Steps to reproduce:

1. Create a new testing profile.
2. Populate it with one or more passwords.
3. Open Saved Passwords to confirm that your passwords are saved.
4. Go to the "Troubleshooting Information" page, and click on the "Reset Firefox…" button, and then on the "Reset Firefox" button in the popup window.


Actual results:

The Import Wizard confirms that some data was successfully imported, but notably absent is "Saved Passwords".


Expected results:

Saved passwords should also be imported.

Comment 1

[:Gijs (he/him)]

Were the passwords actually gone in your new profile, or is the bug you're reporting merely that "Saved Passwords" isn't explicitly called out in the list of things the import wizard says you've imported?

Comment 2

[Daniel Dawson]

Ah, sorry. I forgot that part. The passwords are really not in the new profile. Not only does Saved Passwords show an empty list, but there is no logins.json in the directory.

I found out after reporting this about Old Firefox Data, the folder that Reset Firefox makes on a user's desktop to back up the old profile, so I realize the database can be copied over manually. But it's still something that is supposed to be handled automatically, right?

Comment 3

[:Gijs (he/him)]

(In reply to Daniel Dawson from comment #2)
> I found out after reporting this about Old Firefox Data, the folder that
> Reset Firefox makes on a user's desktop to back up the old profile, so I
> realize the database can be copied over manually. But it's still something
> that is supposed to be handled automatically, right?

Yes.


So... to borrow a phrase from our UI, "well, this is embarrassing...".

We switched to using json, and promptly forgot to include the file in the profile migration code:

http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/FirefoxProfileMigrator.js#96

which, um, would explain why it's not copied!

I'm fairly sure that all it takes is updating that list. Right, Matt?

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #3)
> I'm fairly sure that all it takes is updating that list. Right, Matt?

I imagine it might make sense to do a parse/stringify round trip (or something else) to ensure it's sane?

Comment 5

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #3)
> I'm fairly sure that all it takes is updating that list. Right, Matt?

Yep, I think so.

(In reply to Mark Hammond [:markh] from comment #4)
> I imagine it might make sense to do a parse/stringify round trip (or
> something else) to ensure it's sane?

We've been leaving that up to each consumer to do upon initialization since it should be doing it outside of a reset anyways. If storage-json doesn't already handle malformed files then we should fix that.

Comment 6

[Matthew N. [:MattN]]

We should have a moztrap test for all of the reset data types until we get bug 940954.

Comment 7

[:Gijs (he/him)]

Attachment: copy new logins.json database when using fx reset,

I suspect we might want to drop the signons.sqlite thing here, but I won't take the risk for beta uplift, in an effort to make last beta for this week so we can fix this in 34 still...

Comment 8

[:Gijs (he/him)]

Anthony, are you the right person to talk to for comment #6 ?

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8521046 [details] [diff] [review]
copy new logins.json database when using fx reset,

Review of attachment 8521046 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, we should remove signons.sqlite eventually.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8521046 [details] [diff] [review]
copy new logins.json database when using fx reset,

Review of attachment 8521046 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/migration/FirefoxProfileMigrator.js
@@ +93,5 @@
>    let types = MigrationUtils.resourceTypes;
>    let places = getFileResource(types.HISTORY, ["places.sqlite"]);
>    let cookies = getFileResource(types.COOKIES, ["cookies.sqlite"]);
>    let passwords = getFileResource(types.PASSWORDS,
> +                                  ["signons.sqlite", "logins.json", "key3.db"]);

Actually, am I reading correctly that getFileResource will return `null` if any of the files don't exist? If so, that needs to be fixed since new profiles won't have signons.sqlite.

I think you should change `return null;` to `continue` in getFileResource.

Comment 11

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (UTC+1 until Nov. 22) from comment #10)
> Comment on attachment 8521046 [details] [diff] [review]
> copy new logins.json database when using fx reset,
> 
> Review of attachment 8521046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/migration/FirefoxProfileMigrator.js
> @@ +93,5 @@
> >    let types = MigrationUtils.resourceTypes;
> >    let places = getFileResource(types.HISTORY, ["places.sqlite"]);
> >    let cookies = getFileResource(types.COOKIES, ["cookies.sqlite"]);
> >    let passwords = getFileResource(types.PASSWORDS,
> > +                                  ["signons.sqlite", "logins.json", "key3.db"]);
> 
> Actually, am I reading correctly that getFileResource will return `null` if
> any of the files don't exist? If so, that needs to be fixed since new
> profiles won't have signons.sqlite.

Yeah, and that also explains why the wizard then goes on to say it's not been migrated (see comment #0)...

Comment 12

[:Gijs (he/him)]

Attachment: copy new logins.json database when using fx reset,

Comment 13

[:Gijs (he/him)]

   https://hg.mozilla.org/integration/fx-team/rev/210b1691f0c7

Comment 14

[:Gijs (he/him)]

Comment on attachment 8521327 [details] [diff] [review]
copy new logins.json database when using fx reset,

Approval Request Comment
[Feature/regressing bug #]: new logins/passwords format
[User impact if declined]: dataloss when using firefox reset
[Describe test coverage new/current, TBPL]: no :-(
[Risks and why]: pretty low, targeted fix to the code to add extra file to import, ensuring all extant files get imported if doing a migration (reset).
[String/UUID change made/needed]: no

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/210b1691f0c7

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8521327 [details] [diff] [review]
copy new logins.json database when using fx reset,

This patch looks simple enough and Gijs tells me he verified locally. Taking this in beta9 to avoid data loss. I would like to see this verified on Beta before we release.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cbd6dc3be1d5
https://hg.mozilla.org/releases/mozilla-beta/rev/b718e8c0d423

Comment 18

[Bogdan Maris, Desktop QA]

Reproduced the initial issue on Firefox 34 beta 8, verified that using Firefox 34 beta 9 and latest Nightly (I found no way to reset Developer Edition, no reset button present in about:support) on Windows 8.1 64bit, Mac OS X 10.7.5 and Ubuntu 12.04 32bit, the saved passwords are automatically imported after resetting Firefox and logins.json file is present in profile folder with the right data.

Comment 19

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #18)
> (I found no way to reset Developer
> Edition, no reset button present in about:support)

Does anyone know why this is not showing up on Aurora?

Comment 20

[Mark Hammond [:markh] [:mhammond]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #19)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #18)
> > (I found no way to reset Developer
> > Edition, no reset button present in about:support)
> 
> Does anyone know why this is not showing up on Aurora?

It is for me - but note it is now labelled "Refresh" and I only tried my regular profile and not the devtools-specific one.

Comment 21

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] from comment #20)
> (In reply to Florin Mezei, QA (:FlorinMezei) from comment #19)
> > (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #18)
> > > (I found no way to reset Developer
> > > Edition, no reset button present in about:support)
> > 
> > Does anyone know why this is not showing up on Aurora?
> 
> It is for me - but note it is now labelled "Refresh" and I only tried my
> regular profile and not the devtools-specific one.

The button showing is related to whether the current profile is the default or not. I bet it regressed because of the separate devtools profile stuff. Florin, can you file a bug about this and cc me, MattN, and past, and needinfo MattN? I bet he knows what's up there. Thanks!

(nb: that issue is orthogonal to this bug)

Comment 22

[Bogdan Maris, Desktop QA]

The thing is that the button is missing if I start Aurora with a brand new profile different from dev-edition default profile, logged bug 1100334 on that.

Just tested across platforms using Developer Edition/Aurora and this issue is not reproducible anymore.

Comment 23

[u279076]

(In reply to :Gijs Kruitbosch from comment #8)
> Anthony, are you the right person to talk to for comment #6 ?

I think QE verification should be sufficient here, and that's already been done. Moztrap is not well suited as a stopgap for automation. In the future you should use the in-moztrap flag to ask for Moztrap coverage, in-qa-testsuite for QA owned automation suites.

Comment 24

[:Gijs (he/him)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #23)
> (In reply to :Gijs Kruitbosch from comment #8)
> > Anthony, are you the right person to talk to for comment #6 ?
> 
> I think QE verification should be sufficient here, and that's already been
> done. Moztrap is not well suited as a stopgap for automation. In the future
> you should use the in-moztrap flag to ask for Moztrap coverage,
> in-qa-testsuite for QA owned automation suites.

I think we'd like coverage of this feature in general in moztrap so we don't have dataloss-causing regressions again, considering we don't currently have the technical means to test this using the engineering-owned automation suites.

Comment 25

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #24)
> (In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #23)
> > (In reply to :Gijs Kruitbosch from comment #8)
> > > Anthony, are you the right person to talk to for comment #6 ?
> > 
> > I think QE verification should be sufficient here, and that's already been
> > done. Moztrap is not well suited as a stopgap for automation. In the future
> > you should use the in-moztrap flag to ask for Moztrap coverage,
> > in-qa-testsuite for QA owned automation suites.
> 
> I think we'd like coverage of this feature in general in moztrap so we don't
> have dataloss-causing regressions again, considering we don't currently have
> the technical means to test this using the engineering-owned automation
> suites.

Sorry, perhaps I should have read more carefully - but if you really don't want to do this because Moztrap is "not well suited as a stopgap for automation", can you expand on why that is the case?

Comment 26

[u279076]

(In reply to :Gijs Kruitbosch from comment #25)
> Sorry, perhaps I should have read more carefully - but if you really don't
> want to do this because Moztrap is "not well suited as a stopgap for
> automation", can you expand on why that is the case?

I don't want to go too far down the rabbit hole in this bug. Moztrap should *only* be used to provide coverage for things that cannot be automated. 

Having a Moztrap test will not guarantee you have sufficient coverage to detect a regression as soon as it occurs. I can try to mitigate this by adding the test to our Beta Regression suite which gets cloned every version, but best case scenario that's only going to get run once every 6 weeks in Beta. The effectiveness of Moztrap tests largely depends on the person running the test.

It's a perfectly valid concern if this can't be automated in an Engineering test suite. However, the escalation path for test coverage should then go to see if it can be covered in a QA test suite (Mozmill, Marionette, etc). 

Adding a Moztrap test should be an absolute last resort.

I'm happy to discuss this further with you but not on this bug.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We could perfectly cover this with Mozmill for now. Andreea, can you please take care of it? Thanks.

Comment 28

[Andreea Matei [:AndreeaMatei]]

Henrik, Cosmin filed bug 1113217 for this. Thanks.