Closed Bug 1398932 Opened 7 years ago Closed 7 years ago

add a preference to make firefox use the sqlite-backed NSS databases

Categories

(Core :: Security: PSM, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(3 files)

In preparation for using the sqlite-backed NSS certificate/key databases, it would be good to have an easy toggle to control which format Firefox uses. Since we're a bit too close to the 57 merge day, and since we don't want to introduce potential issues late in the cycle, let's add the option to use the new format but not make it the default until Nightly is 58.
(Unfortunately moving this from bug 783994 means we lost the interdiff - the only changes should be the name of the pref, the default is now false, and there are two tests (as well as some comment changes).)
Comment on attachment 8906775 [details]
bug 1398932 - add a preference for enabling the sqlite-backed NSS databases

https://reviewboard.mozilla.org/r/178500/#review183586

Other than the test comments, this LGTM. And maybe we can't do what I'm asking? Just let me know...

::: security/manager/ssl/tests/unit/test_db_format_pref_new.js:11
(Diff revision 1)
> +
> +// Tests that if "security.use_sqldb" is set to true when PSM initializes,
> +// we create the sqlite-backed certificate and key databases.
> +
> +function run_test() {
> +  let profileDir = do_get_profile();

I know it's pedantic, but this would be a more valuable test if it checked that those files do not exist before asserting that setting the pref and starting the service created them. As-is, some other test could pollute this one.

::: security/manager/ssl/tests/unit/test_db_format_pref_old.js:12
(Diff revision 1)
> +// Tests that if "security.use_sqldb" is set to false when PSM initializes,
> +// we create the system-default certificate and key databases, which currently
> +// use the old BerkeleyDB format. This will change in bug 1377940.
> +
> +function run_test() {
> +  let profileDir = do_get_profile();

same as above.
Attachment #8906775 - Flags: review?(jjones) → review+
Comment on attachment 8906775 [details]
bug 1398932 - add a preference for enabling the sqlite-backed NSS databases

https://reviewboard.mozilla.org/r/178500/#review184422

Looks good; just needs some ESLint fixing.

::: security/manager/ssl/tests/unit/test_db_format_pref_new.js:13
(Diff revision 1)
> +// we create the sqlite-backed certificate and key databases.
> +
> +function run_test() {
> +  let profileDir = do_get_profile();
> +  Services.prefs.setBoolPref("security.use_sqldb", true);
> +  let psm = Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports);

> 13:7  error  'psm' is assigned a value but never used.  no-unused-vars (eslint)

::: security/manager/ssl/tests/unit/test_db_format_pref_old.js:14
(Diff revision 1)
> +// use the old BerkeleyDB format. This will change in bug 1377940.
> +
> +function run_test() {
> +  let profileDir = do_get_profile();
> +  Services.prefs.setBoolPref("security.use_sqldb", false);
> +  let psm = Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports);

Same thing here as well.
Attachment #8906775 - Flags: review?(cykesiopka.bmo) → review+
Comment on attachment 8906775 [details]
bug 1398932 - add a preference for enabling the sqlite-backed NSS databases

https://reviewboard.mozilla.org/r/178500/#review183586

Thanks!

> I know it's pedantic, but this would be a more valuable test if it checked that those files do not exist before asserting that setting the pref and starting the service created them. As-is, some other test could pollute this one.

Sounds good (as I understand it, each xpcshell test is in its own process and profile directory, but you're still right).
Comment on attachment 8906775 [details]
bug 1398932 - add a preference for enabling the sqlite-backed NSS databases

https://reviewboard.mozilla.org/r/178500/#review184422

Thanks!

> > 13:7  error  'psm' is assigned a value but never used.  no-unused-vars (eslint)

Ah - good catch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d60f7cb91b58 demonstrated that this introduced a warning (as an error) due to including nsThreadUtils.h (see bug 1399637). It's a silly warning, so I just disabled it - ni? to J.C. to confirm/deny this option.
Also, I disabled the tests on Android since it doesn't make much sense to have them there (because we unconditionally use the sqlite DBs).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=191105b0a51f413b68598c1bef06548c0402ad58
Flags: needinfo?(jjones)
Seems fine to disable the warning. Also, disabling Android tests make sense, too.
Flags: needinfo?(jjones)
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ad200a781d1
add a preference for enabling the sqlite-backed NSS databases r=Cykesiopka,jcj
https://hg.mozilla.org/mozilla-central/rev/7ad200a781d1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
If NSS changes its internal default to use the sql database by default,
  security.use_sqldb == false
will not work with your patch to use the older format,
and the test_db_format_pref_old.js fails.

If you want the ability to switch back to the pre sql format (dbm), you must prefix the directory with "dbm:".
Also, some tests embed the key3/key4 and cert8/cert9 filenames.

If you want to be able to run the tests in both preference configurations, the tests must be adjusted to dynamically use the correct filename based on the preference.
Attached patch dbm-prefix.patchSplinter Review
(In reply to Kai Engert (:kaie:) from comment #14)
> If you want the ability to switch back to the pre sql format (dbm), you must
> prefix the directory with "dbm:".
Attachment #8909732 - Flags: review?(dkeeler)
To confirm that the preference for switching back to dbm is working correctly, all tests should pass with the NSS patch from bug 1377940 applied, which changes the NSS default to sql.

You expect this to work, because you had added security/manager/ssl/tests/unit/test_db_format_pref_new.js

Here is a try run with the patch from comment 16 and from bug 1377940:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec0fd2d5a2082d959cc144dc3525283c95432d6b
(In reply to Kai Engert (:kaie:) from comment #15)
> Also, some tests embed the key3/key4 and cert8/cert9 filenames.
> 
> If you want to be able to run the tests in both preference configurations,
> the tests must be adjusted to dynamically use the correct filename based on
> the preference.
Attachment #8909754 - Flags: review?(dkeeler)
Hi Kai - "security.use_sqldb" is more of an opt-in mechanism than a way to choose which format to use. I'm expecting Firefox to iron out any issues and commit to using the sqlite format before we change the default in NSS, so there shouldn't be any need to deliberately use the Berkeley DBs after that. At that point we'll probably remove the two tests added in this patch since they won't be relevant any longer. The other tests that use the old formats will have to be updated as well, but again, that will happen when we make the switch (in a different bug). Thanks!
David, thanks for the clarification. If we don't expect that we'll be required to use this preference to switch back to dbm, that simplifies things.
You need to log in before you can comment on or make changes to this bug.