Only run IdentityCredentialStorageCleaner if FedCM is enabled
Categories
(Toolkit :: Data Sanitization, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox114 | --- | fixed |
People
(Reporter: pbz, Assigned: osuolale49, Mentored, NeedInfo)
References
Details
(Keywords: good-first-bug)
Attachments
(5 files)
dom.security.credentialmanagement.identity.enabled
is currently disabled by default. We should not run the IdentityCredentialStorageCleaner
if the feature is disabled. Currently we run it every time we perform a clean operation and it instantiates the IdentityCredentialStorageService
even if the feature is disabled.
The cleaner is implemented here: https://searchfox.org/mozilla-central/rev/e941c8a6d49b4f62f361446de7a80214fb899186/toolkit/components/cleardata/ClearDataService.sys.mjs#1421
Updated•6 months ago
|
Comment 1•6 months ago
|
||
Marking this as a good first bug!
What needs to be done is to check the dom.security.credentialmanagement.identity.enabled
pref before running the IdentityCredentialStorageCleaner
. This could ether be done by not matching the cleaner if the pref is not enabled so the cleaner is not executed (here) or simply checking for the pref in each method of the cleaner (here).
Updated•6 months ago
|
Assignee | ||
Comment 2•6 months ago
|
||
I am looking into this bug
Comment 3•6 months ago
|
||
@Noah Osuolale, thank you for looking into this! I will assign you to the bug, please needinfo me if you have any questions.
Assignee | ||
Comment 4•6 months ago
|
||
``
Assignee | ||
Comment 5•6 months ago
|
||
``
Assignee | ||
Comment 6•6 months ago
|
||
``
Assignee | ||
Comment 7•6 months ago
|
||
Assignee | ||
Comment 8•6 months ago
|
||
Please kindly check this implementation, in the toolkit/components/cleardata/ClearDataService.sys.mjs file to be sure if I am on good track.
https://paste.mozilla.org/TLMp1OwR
Reporter | ||
Updated•6 months ago
|
Comment 9•6 months ago
|
||
Hey Noah! Sorry for the delay. Thank you for your effort so far :)
Your code snipped is a good start, to be able to read the pref you need to ask our pref service to get it for you, you can see an example here. Basically the first argument that you pass in is the pref you want to read an the second argument is the default value that should be returned in case something goes wrong, see the signature of getBoolPref
here
Assignee | ||
Comment 10•6 months ago
|
||
Hi hpeuckmann,
After going through your latest comment, I came up with this, https://paste.mozilla.org/q9Jw1F23. Am I on good track?
Comment 11•6 months ago
|
||
Thanks for the update!
I would rather have the default value to be false
here. We should really only run the cleaner if the pref is true
.
If something goes wrong during our call to getBoolPref
we would execute the cleaner although the pref might be set to false. I'd rather under clear than over clear here.
Besides that your patch looks good to me. I'd suggest you go ahead and submit it for review, you can also submit your patch as a WIP patch first and request review later. When you request review, please put me in as a blocking reviewer.
Assignee | ||
Comment 12•6 months ago
|
||
Assignee | ||
Comment 13•6 months ago
|
||
Hi hpeuckmann,
The above patch is open for review. Thanks
Updated•6 months ago
|
Comment 14•6 months ago
|
||
Pushed by hpeuckmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3522f568e1a5 Only run IdentityCredentialStorageCleaner if FedCM is enabled. r=hpeuckmann
Comment 15•6 months ago
|
||
Backed out changeset 3522f568e1a5 (Bug 1825749) for causing xpcshell failures at est_identity_credential_storage.js
Backout: https://hg.mozilla.org/integration/autoland/rev/49224cf22355ff1b62edae340db171aaa82ec01b
Failure log: https://treeherder.mozilla.org/logviewer?job_id=412271654&repo=autoland&lineNumber=2933
Comment 16•6 months ago
|
||
Pushed by hpeuckmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e35b8568fd3f Only run IdentityCredentialStorageCleaner if FedCM is enabled. r=hpeuckmann
Comment 17•5 months ago
|
||
bugherder |
Description
•