1013947 - Remove legacy signons.sqlite files and references

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P5)

Description

Bug 853549 introduces the new "logins.json" file for storing Login Manager data, replacing the legacy "signons.sqlite" file. This SQLite file is kept around and may still be used in case the profile is downgraded.

In a similar way to bug 925101 for the legacy "signons.txt" file, the SQLite file should be deleted after it has not been used for some time.

Note that there might be similarly named files in case a backup was made because of a corrupted database, and these might be deleted as well.

This should not happen on Android, where the SQLite file is still used.

Comment 1

[Matthew N. [:MattN]]

The first bug mention in comment 0 should have been bug 853549.

To fix this bug we can add logic to `LoginHelper.removeLegacySignonFiles()`:

* Move the following code https://dxr.mozilla.org/mozilla-central/rev/b7e42143bbbc9dc3e5c05bd1e93b6485ce1d0ad4/browser/components/nsBrowserGlue.js#1982-1984 to the bottom of `_migrateUI`, updating the `if (currentUIVersion < NN) {` accordingly. When testing out this _migrateUI function you will have to rollback the about:config preference "browser.migration.version" to get it to run again.
* Update UI_VERSION to match the above: https://dxr.mozilla.org/mozilla-central/rev/b7e42143bbbc9dc3e5c05bd1e93b6485ce1d0ad4/browser/components/nsBrowserGlue.js#
* Update `LoginHelper.removeLegacySignonFiles`'s `toDeletes` set to include the following (https://dxr.mozilla.org/mozilla-central/rev/b7e42143bbbc9dc3e5c05bd1e93b6485ce1d0ad4/toolkit/components/passwordmgr/LoginHelper.jsm#659,668) 
** signons.sqlite
** signons.sqlite.corrupt
** signons.sqlite-shm
** signons.sqlite-wal (not sure if we even used WAL but just in case some extension did)
* Add signons.sqlite-N.corrupt (where 1 <= N < 10000)[1] to `toDeletes` if they exist.
* Add `Services.prefs.clearUserPref("signon.importedFromSqlite");` to `removeLegacySignonFiles`
* Remove any references to signon.importedFromSqlite by assuming that it's true for any non-Android code.
* We need to be careful not to delete any files on Android. I think we should add an early return in `removeLegacySignonFiles` if we're on Android to be safe.
* Add test tasks to toolkit/components/passwordmgr/test/unit/test_removeLegacySignonFiles.js to cover the code added above.

Let me know if you have any questions. Thanks.

[1] https://dxr.mozilla.org/mozilla-central/rev/b7e42143bbbc9dc3e5c05bd1e93b6485ce1d0ad4/xpcom/io/nsLocalFileCommon.cpp#59

Comment 2

[Matthew N. [:MattN]]

Type: defect → task

This is a defect since we're leaving stale data in profiles.

Comment 3

[Matthew N. [:MattN]]

We should also cleanup signon.importedFromSqlite pref values at the same time.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I tlooks like this code doesn't exist anymore, there's still a couple references to signons.sqlite that should be removed:
https://searchfox.org/mozilla-central/search?q=signons.sqlite&path=&case=false&regexp=false

Comment 5

[Matthew N. [:MattN]]

(In reply to Marco Bonardo [:mak] from comment #4)

I tlooks like this code doesn't exist anymore, there's still a couple references to signons.sqlite that should be removed:
https://searchfox.org/mozilla-central/search?q=signons.sqlite&path=&case=false&regexp=false

See comment 2. The main focus of this bug is deleting the leftover signons.sqlite files in the profile directory of users.

Comment 6

[Riya]

Is this issue still open? I would love to work.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Yes, it's available. there's a few references to remove in comment 4, and you should write a UI Migration function to remove leftover files from the profiles.
See for example how migration 119 was implemented https://searchfox.org/mozilla-central/rev/2e3b0483e31abffe0b4374480a34c6d23f5186ea/browser/components/BrowserGlue.jsm#3300,3303,3931
Feel free to Request Info from me using the checkbox below if you have questions, or just ask on Phabricator once you submit a patch.

Comment 8

[Riya]

Thank you, I'll soon update you with my work on this issue.

Comment 9

[Riya]

Hi, I have a few queries. How current UI versions are determined? Up to which UI version, these signons.sqlite files are required.
Thank you.

Comment 10

[Riya]

Attachment: Bug 1013947 - Remove legacy signons.sqlite files and references. r?mak

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Riya from comment #9)

Hi, I have a few queries. How current UI versions are determined? Up to which UI version, these signons.sqlite files are required.
Thank you.

UI version is just an increasing number, when a new profile is created the version number is bumped to the current one, and no migration runs because it assumes it's up-to-date being a new profile. Old versions on upgrade will instead run all the migrations from their version number to the current one. Migrations run only once per profile. A new Firefox version can have multiple ui migrations or none, there's no relatione between the Firefox version number and the migration version.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 13

[yayaazeez222]

(In reply to Marco Bonardo [:mak] from comment #7)

Yes, it's available. there's a few references to remove in comment 4, and you should write a UI Migration function to remove leftover files from the profiles.
See for example how migration 119 was implemented https://searchfox.org/mozilla-central/rev/2e3b0483e31abffe0b4374480a34c6d23f5186ea/browser/components/BrowserGlue.jsm#3300,3303,3931
Feel free to Request Info from me using the checkbox below if you have questions, or just ask on Phabricator once you submit a patch.

is this still open? if so can i get assigned the isssue?

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 15

[Ivan]

Hello, I would like to take this over if possible!

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Feel free to attach a patch when you have it, there is an old one that can be used as a starting point

Comment 17

[Ivan]

Hello Marco,

I got the basic removals done, but I was unable to find browser/components/BrowserGlue.jsm. Am I missing something? I am fairly new, so I may be missing the obvious.

Thank you! :)

Comment 18

[Ivan]

(In reply to Ivan from comment #17)

Hello Marco,

I got the basic removals done, but I was unable to find browser/components/BrowserGlue.jsm. Am I missing something? I am fairly new, so I may be missing the obvious.

Thank you! :)

Also, I have built and run everything successfully. Just FYI.

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Ivan from comment #17)

Hello Marco,

I got the basic removals done, but I was unable to find browser/components/BrowserGlue.jsm. Am I missing something? I am fairly new, so I may be missing the obvious.

Many .jsm files are in the process of being renamed to .sys.mjs https://searchfox.org/mozilla-central/source/browser/components/BrowserGlue.sys.mjs

Comment 20

[Ivan]

Attachment: Bug 1013947 - Implement removal of legacy signons.sqlite files and references. r?mak

Comment 21

[Ivan]

Hello Marco,

I attempted to apply the corrections that you suggested, but my local version ends at UI_VERSION < 121, but you mention us being on version 137 (one I am adding). How do I update my local build to reflect the latest update? I have looked, but can't find information about it.

Thanks!

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Ivan from comment #21)

Hello Marco,

I attempted to apply the corrections that you suggested, but my local version ends at UI_VERSION < 121, but you mention us being on version 137 (one I am adding). How do I update my local build to reflect the latest update? I have looked, but can't find information about it.

You can find some documentation at https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html and https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/workflows.html#

To summarize, you must hg pull central to update your local tree to the latest version, then you can use hg wip to visually check the tree situation. you should see both the central head and your revision. Then you must hg rebase -s <your_revision> -d central to rebase your revision on top of mozilla-central. You may have to merge some changes. Then just hg update <your_revision>, make changes, and hg amend to merge changes into the revision.

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 24

[vitaspiros]

Attachment: Bug 1013947 - Removed signons.sqlite references r?mak

Depends on D189346

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11b3d438788a
Removed signons.sqlite references r=mak,credential-management-reviewers,dimi

Comment 26

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/11b3d438788a