The default bug view has changed. See this FAQ.

Remove the ability to clear saved passwords/logins on shutdown

VERIFIED FIXED in Firefox 42

Status

()

Firefox
Sync
P1
normal
Rank:
15
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: sawrubh, Assigned: eoger)

Tracking

({relnote})

Trunk
Firefox 42
relnote
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox42 verified)

Details

(Whiteboard: [fxsync])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
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?
(Reporter)

Comment 2

2 years ago
No, I haven't changed nor reset my password. I'll update the bug with my Sync logs soon.
(Reporter)

Comment 3

2 years ago
Created attachment 8527172 [details]
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.
(Reporter)

Comment 4

2 years ago
Created attachment 8527187 [details]
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

2 years ago
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

2 years ago
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.
Summary: Very frequently asked to Reconnect to Sync → Don't delete Sync credentials in password manager if Firefox is set to delete any saved passwords when it closes
Thanks for the diagnosis! This definitely seems like an oversight and should be fixed.
(Reporter)

Comment 8

2 years ago
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).
See Also: → bug 1112962
Flags: qe-verify+
Flags: firefox-backlog+

Updated

2 years ago
Hardware: x86_64 → All

Updated

2 years ago
Priority: -- → P1

Updated

2 years ago
Blocks: 1182288

Updated

2 years ago
Rank: 15
Assignee: nobody → edouard.oger
(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.
(Reporter)

Comment 10

2 years ago
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.
(Assignee)

Comment 11

2 years ago
Created attachment 8633048 [details] [diff] [review]
bug-1102184.patch

I implemented what you had in mind, what do you think?
Attachment #8633048 - Flags: feedback?(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.
Attachment #8633048 - Flags: feedback?(dolske) → feedback+
(Assignee)

Updated

2 years ago
Attachment #8633048 - Flags: review?(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
Attachment #8633048 - Flags: review?(dolske) → review+
Margaret: what should mobile do here? (See top of last comment)
Flags: needinfo?(margaret.leibovic)

Comment 15

2 years ago
(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.
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 16

2 years ago
Created attachment 8635499 [details] [diff] [review]
bug-1102184.patch

Comments addressed.
Attachment #8633048 - Attachment is obsolete: true
Attachment #8635499 - Flags: review?(dolske)

Updated

2 years ago
Whiteboard: [fxsync]
(Assignee)

Updated

2 years ago
Attachment #8635499 - Flags: review?(dolske) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Updated

2 years ago
Summary: Don't delete Sync credentials in password manager if Firefox is set to delete any saved passwords when it closes → Remove the ability to clear saved passwords/logins on shutdown
This seems big enough that it should probably have a try push.
Status: NEW → ASSIGNED
Flags: needinfo?(edouard.oger)
Keywords: checkin-needed
(Assignee)

Comment 18

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74a9a0ca5766
(Assignee)

Comment 19

2 years ago
Let's try!
Flags: needinfo?(edouard.oger)
(Assignee)

Comment 20

2 years ago
Try looks okay
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 21

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/fb4f42bbeb01
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fb4f42bbeb01
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Keywords: relnote

Updated

2 years ago
Iteration: --- → 42.2 - Jul 27
QA Contact: catalin.varga
"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.
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
QA Contact: catalin.varga → paul.silaghi
Depends on: 1242176
Depends on: 1244908
You need to log in before you can comment on or make changes to this bug.