Closed
Bug 1034636
Opened 10 years ago
Closed 10 years ago
Remove mozilla::pkix::ScopedPLArenaPool and mozlla::pkix::ScopedCERTCertificate
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: briansmith, Assigned: briansmith)
References
Details
Attachments
(1 file)
41.91 KB,
patch
|
mmc
:
review+
|
Details | Diff | Splinter Review |
The bigger goalare are:
1. Remove mozilla::pkix::ScopedPtr completely and/or make it part of pkixtestutil.h, since soon mozilla::pkix will not need ScopedPtr.
2. Remove all uses of CERTCertificate from mozilla::pkix, as part of a bigger effort to stop depending on the mess of stuff in security/nss/lib/certdb/.
3. Make the non-mozilla::pkix code easier to read by removing the badness of having two different ScopedCERTCertificate classes.
We still need ScopedCERTCertList until I fix a couple more bugs. And, there are a couple of places where internally we need ScopedPtr<CERTCertificate, CERT_DestroyCertificate>, but those two uses will be removed very soon™. Similarly, I already have a patch in my queue for removing the last use of ScopedPLArenaPool from mozilla/pkix/lib.
Attachment #8451018 -
Flags: review?(mmc)
Comment 1•10 years ago
|
||
Hi Brian,
I don't understand this change. It looks like you are moving ScopedPLArenaPool from pkixtypes.h to nssgtest.h and ScopedNSSTypes.h, and defining PORT_FreeArena_false in multiple places so it can't be exported. My mxr fu is weak and I can only see one definition for ScopedCERTCertificate in pkixtypes.h. Where is the other one?
Thanks,
Monica
Flags: needinfo?(brian)
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #1)
> Hi Brian,
>
> I don't understand this change. It looks like you are moving
> ScopedPLArenaPool from pkixtypes.h to nssgtest.h and ScopedNSSTypes.h, and
> defining PORT_FreeArena_false in multiple places so it can't be exported.
Right now, we use ScopedPLArenaPool in PSM (outside mozilla::pkix), in the mozilla::pkix tests, and in CheckNameConstraints. In another bug, I will remove the use of PLArenaPool within CheckNameConstraints, so that ScopedPLArenaPool is used only within the mozilla::pkix tests and in parts of PSM outside of mozilla::pkix. I want to stop defining ScopedPLArenaPool within the non-test part of mozilla::pkix. (In fact, my goal is to remove ALL uses of ScopedPtr from the non-test part of mozilla::pkix, and I have patches that go most of the way to doing that.)
> My
> mxr fu is weak and I can only see one definition for ScopedCERTCertificate
> in pkixtypes.h. Where is the other one?
The original definition of ScopedCERTCertificate is in ScopedNSSTypes.h. mozilla::pkix doesn't use ScopedNSSTypes.h because it avoids depending on any Gecko stuff other than NSS and NSPR (and even those dependencies are getting eliminated through this series of bugs), so it defines its own ScopedCERTCertificate.
In other words, the duplication of PORT_FreeArena_false for CheckNameConstraints will go away when CheckNameConstraints lands, so that duplication is temporary. I want to eliminate the uses of mozilla::pkix::ScopedPLArenaPool from outside of mozilla::pkix now so that the non-mozilla::pkix code is disrupted less by my future patches, and so we can get rid of the non-header "mozilla::" and "mozilla::pkix::" namespace qualifiers in PSM, to make the code easier to read.
Flags: needinfo?(brian)
Comment 3•10 years ago
|
||
Comment on attachment 8451018 [details] [diff] [review]
remove-ScopedCERTCertificate-and-ScopedPLArenaPool.patch
Review of attachment 8451018 [details] [diff] [review]:
-----------------------------------------------------------------
Ok, thanks for the explanation. Tolerating some temporary duplication in this case is OK with me.
Attachment #8451018 -
Flags: review?(mmc) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Thanks for the quick reviews!
https://hg.mozilla.org/integration/mozilla-inbound/rev/3acf9162f52d
Comment 5•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•