1102184 - Remove the ability to clear saved passwords/logins on shutdown

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Saurabh Anand [:sawrubh]]

I use Nightly on Windows, Linux and Android. I'm very frequently asked to reconnect sync (basically sign in again). I'm not sure what's the reason but we should have a system which doesn't ask users to sign in so frequently. Is this because of me being on multiple OS or is this to do with the validity of how long I stay signed in. It certainly doesn't make for a very good UX.

Comment 1

[Chris Karlof [:ckarlof]]

I'm sorry you're having this problem. It definitely sounds like a bad experience.

Some questions: 

1) We ask you to reconnect if you've reset or changed your password. Have you done this?
2) It would be great to get some error logs from your Desktop machines that experience this error. If you visit about:sync-log, do you see any error logs? If so, can you include them in this bug?

Comment 2

[Saurabh Anand [:sawrubh]]

No, I haven't changed nor reset my password. I'll update the bug with my Sync logs soon.

Comment 3

[Saurabh Anand [:sawrubh]]

Attachment: error logs on Windows

I saw a lot of error logs in about:sync-log so just attaching all of them. These are from my Windows machine. Will also attach the Linux errors soon.

Comment 4

[Saurabh Anand [:sawrubh]]

Attachment: error logs on Linux

Linux has much more error logs. In the latest folder is today's error log. In the recent folder are recent error logs, while outside any folder are the older error logs.

Comment 5

[carbohydrates]

I am having this issue as well.  I am asked to reauthenticate every single time I open Firefox.  My profile is never fully synced, addons and themes are installed but no preferences are transferred.  This only happens in Windows, it works flawlessly in Linux.  Firefox v34.0.5.  I tried deleting my local profile and creating a new one, it didn't help.  I have no error logs in Windows.

Comment 6

[carbohydrates]

So, this behavior is caused when you have Firefox set to delete any saved passwords when it closes.  I am staying logged in now after changing that setting.

This is really backwards behavior, that feature should not disconnect your sync account.  I do not want firefox to remember passwords for any of my accounts at various websites.  Even if you can set it to not offer to store them in the first place, I want them to be deleted anyway.  I DO want it to remember my sync account, and it is very counterintuitive that it doesn't.

Comment 7

[Chris Karlof [:ckarlof]]

Thanks for the diagnosis! This definitely seems like an oversight and should be fixed.

Comment 8

[Saurabh Anand [:sawrubh]]

I don't know how you set Firefox to delete your saved passwords on closing, but I (think?) I don't have that set and yet I was facing that issue. On the other hand I do have a master password set (so every time I open Firefox I need to enter it and also my passwords aren't synced because I have master password enabled) so probably it's the same effect as having Firefox set to delete any saved passwords upon closing. And fwiw I was facing this issue whenever I switched machines (like from Linux to Windows or vice versa or one Linux machine to another Linux machine).

Comment 9

[Justin Dolske [:Dolske]]

(In reply to Saurabh Anand [:sawrubh] from comment #8)
> I don't know how you set Firefox to delete your saved passwords on closing,
> but I (think?) I don't have that set and yet I was facing that issue.

First -- this bug has mutated a bit from Saurabh's original report. So if you're still seeing this, someone needs to debug it in a new bug. I'll also note that the related bugs under this one also seem to have different causes now. So I'm not really sure fixing "clear passwords at shutdown breaks Sync" is a very common problem. But it's still a fair to look at fixing...

One approach would be to make the "clear passwords at shutdown" not actually clear Sync creds (or any chrome:// creds?). But that seems likely to be surprising to someone who really _does_ want all passwords cleared, eg a shared computer setup. And then we'll risk getting bugs about not doing what we say on the tin, which is also fair. ;) We could clarify that the feature is only for "web logins" or something like that, but this starts to go down a rathole of strange usecases and assumptions.

That makes me think that, really, this entire feature is weird and should be removed. If you don't want your browser remembering passwords, just disable password manager. Done. Saving passwords just until shutdown is not a usecase we need to support.

So I think the fix here is simple:

1) Do a one time migration (at shutdown), for users with this pref enabled. Clear logins one last time, and then disable password manager. [Which, really, just disables saving web logins, not the whole password manager. That would be nice to clarify too, separately.]

2) Remove the support (UI) for clearing saved logins at shutdown.


The only thing that sorta leaves is the "Remove All" from the actual password manager itself. Bug I feel like that's less likely to be a problem, as the context of the forget-at-shutdown feature leads users to see it as "just" cleaning up tracks. But we've talked about killing "Remove All" too (bug 1121292) -- since wontfix'd, but could revisit that.

Comment 10

[Saurabh Anand [:sawrubh]]

Sorry for not noticing the comments and action going on here. I've recently been working only on one system hence can't really tell if this is still happening or not.

Comment 11

[Edouard Oger [:eoger]]

Attachment: bug-1102184.patch

I implemented what you had in mind, what do you think?

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 8633048 [details] [diff] [review]
bug-1102184.patch

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

Yeah, that's what I was thinking. Not as sure about removing the code from sanitizer.js, but I can't think of a great reason to keep it offhand... If it supported time ranges it might actually be useful for the Forget button, but it doesn't.

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 8633048 [details] [diff] [review]
bug-1102184.patch

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

r+ with changes.

Also... Looks like mobile has a fork of some of this. See mobile/android/modules/Sanitizer.jsm and the privacy.item.passwords pref... Seems like we should remove it from mobile too, for the same reasons. Although the contexts are subtly different...

On Desktop, History -> Clear Recent History only clears "Active Logins" (e.g. HTTP auth you've entered during the session). It does not offer to clear logins the user has explicitly _saved_. Clearing saved logins is only a part of the "Clear history when Firefox closes" stuff we're fixing here. Mobile, on the other hand, doesn't seem to have a "Clear when Firefox closes" feature (I suppose in large part because you generally don't close apps!), but does offer to clear _saved_ logins as part of this "Clear Private Data" function.

It's a little more plausible to keep, but I'd still suggest removing it (or at least defaulting it to false for passwords). All the other privacy.item things it clears are things users don't explicitly ask to store (history, cookies, etc -- just like desktop's Clear Recent History). Better still, make this "Clear Active Logins" like Desktop has.

Probably best to do this in a separate bug?

::: browser/base/content/sanitize.js
@@ -449,5 @@
>  
> -    passwords: {
> -      clear: function ()
> -      {
> -        TelemetryStopwatch.start("FX_SANITIZE_PASSWORDS");

Please remove this probe from toolkit/components/telemetry/Histograms.json.

@@ +762,5 @@
> +  if (!Services.prefs.getBoolPref("privacy.sanitize.migrateClearSavedPwdsOnExit")) {
> +    let deprecatedPref = "privacy.clearOnShutdown.passwords";
> +    let doUpdate = Sanitizer.prefs.getBoolPref(Sanitizer.prefShutdown) &&
> +                   Services.prefs.prefHasUserValue(deprecatedPref) &&
> +                   Services.prefs.getBoolPref(deprecatedPref);

I'd move the migration into _checkAndSanitize. That lets you skip checking .prefShutdown, and ensures that when the sanitizer runs on _startup_ that the migration and clearing still happens as expected.

@@ +765,5 @@
> +                   Services.prefs.prefHasUserValue(deprecatedPref) &&
> +                   Services.prefs.getBoolPref(deprecatedPref);
> +    if (doUpdate) {
> +      var pwmgr = Components.classes["@mozilla.org/login-manager;1"]
> +                            .getService(Components.interfaces.nsILoginManager);

Was going to suggest using Cc/Ci instead, but better yet just use |Services.logins.removeAllLogins()|!

::: browser/components/places/tests/unit/test_clearHistory_shutdown.js
@@ -56,5 @@
>    Services.prefs.setBoolPref("privacy.clearOnShutdown.history", true);
>    Services.prefs.setBoolPref("privacy.clearOnShutdown.downloads", true);
>    Services.prefs.setBoolPref("privacy.clearOnShutdown.cookies", true);
>    Services.prefs.setBoolPref("privacy.clearOnShutdown.formData", true);
> -  Services.prefs.setBoolPref("privacy.clearOnShutdown.passwords", true);

Heh. Clearly this wasn't testing it much! :)

::: browser/components/preferences/sanitize.xul
@@ -88,5 @@
>          </columns>
>          <rows>
>            <row>
> -            <checkbox label="&itemPasswords.label;"
> -                      accesskey="&itemPasswords.accesskey;"

These two strings should be removed from browser/locales/en-US/chrome/browser/sanitize.dtd

Comment 14

[Justin Dolske [:Dolske]]

Margaret: what should mobile do here? (See top of last comment)

Comment 15

[:Margaret Leibovic]

(In reply to Justin Dolske [:Dolske] from comment #13)

> Also... Looks like mobile has a fork of some of this. See
> mobile/android/modules/Sanitizer.jsm and the privacy.item.passwords pref...
> Seems like we should remove it from mobile too, for the same reasons.
> Although the contexts are subtly different...
> 
> On Desktop, History -> Clear Recent History only clears "Active Logins"
> (e.g. HTTP auth you've entered during the session). It does not offer to
> clear logins the user has explicitly _saved_. Clearing saved logins is only
> a part of the "Clear history when Firefox closes" stuff we're fixing here.
> Mobile, on the other hand, doesn't seem to have a "Clear when Firefox
> closes" feature (I suppose in large part because you generally don't close
> apps!), but does offer to clear _saved_ logins as part of this "Clear
> Private Data" function.

We do have a "Clear on exit" feature, where you can choose to clear saved logins on exit, but this follows the same code path as the "Clear now" feature (going through Sanitizer.jsm as you mentioned above).

> It's a little more plausible to keep, but I'd still suggest removing it (or
> at least defaulting it to false for passwords). All the other privacy.item
> things it clears are things users don't explicitly ask to store (history,
> cookies, etc -- just like desktop's Clear Recent History). Better still,
> make this "Clear Active Logins" like Desktop has.
> 
> Probably best to do this in a separate bug?

Yes, please file a separate bug in the Firefox for Android product to track this.

If I'm understanding this bug correctly, it's not longer just about not deleting your sync credentials, but rather removing this "clear saved logins at shutdown" feature altogether. Could we update the bug summary to reflect that?

Note: We don't use the Firefox login manager to store sync credentials on mobile, since our sync implementation is all in Java, so the original bug report wouldn't apply to mobile.

Comment 16

[Edouard Oger [:eoger]]

Attachment: bug-1102184.patch

Comments addressed.

Comment 17

[Matthew N. [:MattN]]

This seems big enough that it should probably have a try push.

Comment 18

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74a9a0ca5766

Comment 19

[Edouard Oger [:eoger]]

Let's try!

Comment 20

[Edouard Oger [:eoger]]

Try looks okay

Comment 21

[Pulsebot]

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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb4f42bbeb01

Comment 23

[Paul Silaghi, QA [:pauly]]

"Clear saved passwords on shutdown" option is gone now and sync no longer asks to reconnect after the browser restarts. Verified fixed 42.0a2 (2015-08-19), 43.0a1 (2015-08-19), Win 7, OS X 10.10, Ubuntu 14.04.