Closed
Bug 1315869
Opened 8 years ago
Closed 8 years ago
gtestify security/manager/ssl/tests/compiled/*.cpp
Categories
(Core :: Security: PSM, defect, P1)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: [psm-assigned])
Attachments
(2 files, 2 obsolete files)
19.45 KB,
patch
|
Cykesiopka
:
review+
|
Details | Diff | Splinter Review |
28.15 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
For bug 1313141 we are converting GeckoCppUnitTests to gtests. This bug is about doing that for security/manager/ssl/tests/compiled/*.cpp.
![]() |
Assignee | |
Comment 1•8 years ago
|
||
I'm having trouble a failure within TestIsCertBuiltInRoot.cpp. Here's the
output.
> [RUN ] psm_IsCertBuiltinRoot.Test
> GetCertDER succeeded
> AddCertificate succeeded
> GetCertDER succeeded
> AddCertificate succeeded
> PreloadNSSCertDB succeeded
> got expected value for isBuiltInRoot
> built-in root with no modified trust considered built-in
> got expected value for isBuiltInRoot
> built-in root with modified trust considered built-in
> FindCertByDBKey says it succeeded but it clearly didn't
> ../../../../../../security/manager/ssl/tests/gtest/TestIsCertBuiltInRoot.cpp:313:
> Failure
> Value of: TestIsCertBuiltIn(sLetsEncryptCertDBKey, false)
> Actual: false
> Expected: true
> non-built-in root should not be considered built-in
Things are fine up to the TestIsCertBuiltIn(sLetsEncryptCertDBKey, false) call,
but that call fails because the FindCertByDBKey() call sets |cert| to null. I
don't know why, though I suspect it has something to do with my replacement of
ScopedXPCOM with ScopedProfD.
cykesiopka, do you know why this failure is happening? Thanks.
Attachment #8808469 -
Flags: feedback?(cykesiopka.bmo)
![]() |
Assignee | |
Comment 2•8 years ago
|
||
There's another problem with that test, which is that PreloadNSSCertDB() assumes it's part of a standalone test. But when this becomes a gtest and part of a single process with lots of other tests, I get this failure instead at the NSS_Initialize() calls:
> NSS shouldn't already be initialized, or part of this test is moot
Hmm... not sure how to deal with this.
![]() |
||
Comment 3•8 years ago
|
||
What this test is supposed to test is that if we've already run Firefox at least once and the user has a copy of a built-in certificate in the not-built-in-but-permanent certificate DB (in their profile), that whenever we get a handle on that certificate, we still consider it to be built-in. If there were a way to pre-seed an xpcshell test's profile directory with some files (before we even start the xpcshell instance), we could probably turn this into an xpcshell test.
![]() |
Assignee | |
Comment 4•8 years ago
|
||
Comment on attachment 8808469 [details] [diff] [review]
gtestify security/manager/ssl/tests/compiled/*.cpp
I worked out the LetsEncrypt problem -- my change had caused the profile directory to be used to be wrong. I have a fix for that locally.
However, making this test run in a non-standalone gtest fashion seems tricky. dkeeler has offered to convert it to an xpcshelltest.
Attachment #8808469 -
Flags: feedback?(cykesiopka.bmo)
![]() |
||
Comment 5•8 years ago
|
||
Comment on attachment 8808469 [details] [diff] [review]
gtestify security/manager/ssl/tests/compiled/*.cpp
Review of attachment 8808469 [details] [diff] [review]:
-----------------------------------------------------------------
I hadn't thought up a decent solution to the TestIsCertBuiltInRoot.cpp NSS init problem yet, but it looks like keeler has that covered.
I'm also unsure what the best way is to have TestCertDB.cpp still serve its original purpose (a crash test that requires XPCOM shutdown, IIUC). I'll keep thinking about it, but maybe keeler has ideas for what to do with TestCertDB.cpp as well.
Anyways, I noticed some minor nits.
::: security/manager/ssl/tests/gtest/TestSTSParser.cpp
@@ +6,1 @@
> #include <stdio.h>
If you feel like it, this would be a good time to separate out this include to
another include block to conform to the style guide.
::: security/manager/ssl/tests/gtest/moz.build
@@ +8,5 @@
> 'DataStorageTest.cpp',
> 'DeserializeCertTest.cpp',
> 'MD4Test.cpp',
> 'OCSPCacheTest.cpp',
> + 'TestCertDB.cpp',
Nit: These should probably be named *Test.cpp for consistency with the other files in the directory.
![]() |
||
Comment 6•8 years ago
|
||
Attachment #8809151 -
Flags: review?(cykesiopka.bmo)
![]() |
||
Updated•8 years ago
|
Attachment #8809151 -
Flags: review?(n.nethercote)
![]() |
Assignee | |
Comment 7•8 years ago
|
||
Comment on attachment 8809151 [details] [diff] [review]
1315869-TestIsCertBuiltInRoot-to-xpcshell.diff
Review of attachment 8809151 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me, as far as I can tell given that I don't know anything about security/... :)
Thank you for rewriting the test, that would have been difficult for me to do.
::: security/manager/ssl/tests/unit/test_cert_isBuiltInRoot_reload.js
@@ +93,5 @@
> + let veriSignCertDBKey = "AAAAAAAAAAAAAAAQAAAAzS+A/iOM" +
> + "DiIPSGcSKJGHrLMwgcoxCzAJBgNVBAYTAlVTMRcwFQYDVQQKEw5WZXJpU2lnbiwg" +
> + "SW5jLjEfMB0GA1UECxMWVmVyaVNpZ24gVHJ1c3QgTmV0d29yazE6MDgGA1UECxMx" +
> + "KGMpIDIwMDcgVmVyaVNpZ24sIEluYy4gLSBGb3IgYXV0aG9yaXplZCB1c2Ugb25s" +
> + "eTFFMEMGA1UEAxM8VmVyaVNpZ24gQ2xhc3MgMyBQdWJsaWMgUHJpbWFyeSBDZXJ0" +
Using backslash at the end of each line to do the multi-line string is arguably nicer than concatenation.
Alternatively, ES6 has template literals, which are delimited by backticks. I'm not sure if we can use that in xpcshelltests, but if we can, it's an even nicer alternative.
Attachment #8809151 -
Flags: review?(n.nethercote) → review+
![]() |
Assignee | |
Comment 8•8 years ago
|
||
I've addressed all the comments. This patch applies on top of dkeeler's patch.
Attachment #8809222 -
Flags: review?(cykesiopka.bmo)
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8808469 -
Attachment is obsolete: true
![]() |
||
Updated•8 years ago
|
Priority: -- → P1
Whiteboard: [psm-assigned]
![]() |
||
Comment 9•8 years ago
|
||
Comment on attachment 8809151 [details] [diff] [review]
1315869-TestIsCertBuiltInRoot-to-xpcshell.diff
Review of attachment 8809151 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good.
::: security/manager/ssl/tests/unit/test_cert_isBuiltInRoot_reload.js
@@ +78,4 @@
>
> +function run_test() {
> + let profile = do_get_profile();
> + let certDBName = AppConstants.platform == "android" ? "cert9.db" : "cert8.db";
Nit: Let's make this and `keyDBName` `const` instead.
@@ +90,4 @@
>
> + // This is a built-in root, but not one that was added to the preexisting
> + // certificate DB.
> + let veriSignCertDBKey = "AAAAAAAAAAAAAAAQAAAAzS+A/iOM" +
Nit: Let's make all these DB keys `const`.
Attachment #8809151 -
Flags: review?(cykesiopka.bmo) → review+
![]() |
||
Comment 10•8 years ago
|
||
Comment on attachment 8809222 [details] [diff] [review]
gtestify security/manager/ssl/tests/compiled/*.cpp
Review of attachment 8809222 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good, thanks.
Converting TestCertDB.cpp to a GTest does mean a bit of testing is lost, but I'll just file a follow-up, since I can't think of a reasonable solution that doesn't take a lot of work right now, and I don't want to block on this.
::: security/manager/ssl/tests/gtest/STSParserTest.cpp
@@ +26,2 @@
>
> + ASSERT_EQ(maxAge, expectedMaxAge) << "Did not correctly parse maxAge";
Let's make this `ASSERT_EQ` and ones after in this function `EXPECT_EQ` instead.
@@ +84,5 @@
> + TestSuccess("max-age=100;includeSubdomains", false, 100, true, sss);
> + TestSuccess("max-age=100\t; includeSubdomains", false, 100, true, sss);
> + TestSuccess(" max-age=100; includeSubdomains", false, 100, true, sss);
> + TestSuccess("max-age = 100 ; includeSubdomains", false, 100, true, sss);
> + TestSuccess("max-age = 100 ; includeSubdomains", false, 100, true, sss);
Nit: Let's try to keep lines to <= 80 chars if possible, here and below.
Attachment #8809222 -
Flags: review?(cykesiopka.bmo) → review+
![]() |
||
Comment 11•8 years ago
|
||
Thanks for the reviews!
Attachment #8809151 -
Attachment is obsolete: true
Attachment #8809898 -
Flags: review+
![]() |
Assignee | |
Comment 12•8 years ago
|
||
> Converting TestCertDB.cpp to a GTest does mean a bit of testing is lost, but
> I'll just file a follow-up, since I can't think of a reasonable solution
> that doesn't take a lot of work right now, and I don't want to block on this.
What testing is lost?
![]() |
Assignee | |
Comment 13•8 years ago
|
||
![]() |
||
Comment 14•8 years ago
|
||
Originally TestCertDB was added in response to nsNSSCertificateDB not implementing the NSS shutdown dance correctly. As far as I can tell, encountering the error was only ever possible in cases where there was no profile directory, which is generally not how Firefox is run. In any case, this specific implementation is unlikely to regress in a way that would be caught by the test, so I don't think we're losing too much by converting it to a gtest. Also, I'm currently developing a plan to remove the NSS shutdown framework entirely, so hopefully it won't even be relevant in the near-ish future.
Comment 15•8 years ago
|
||
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/556c13784a9c
convert TestIsCertBuiltInRoot to an xpcshell test r=Cykesiopka,njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae039aa6accf
gtestify security/manager/ssl/tests/compiled/*.cpp. r=cykesiopka.
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/556c13784a9c
https://hg.mozilla.org/mozilla-central/rev/ae039aa6accf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•