gtestify security/manager/ssl/tests/compiled/*.cpp

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [psm-assigned])

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

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

3 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

3 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.
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

3 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

3 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.
Assignee

Comment 7

3 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

3 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

3 years ago
Attachment #8808469 - Attachment is obsolete: true
Priority: -- → P1
Whiteboard: [psm-assigned]

Comment 9

3 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

3 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+
Assignee

Comment 12

3 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?
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

3 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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/556c13784a9c
https://hg.mozilla.org/mozilla-central/rev/ae039aa6accf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.