x509_certificate_model: remove unused code, move nss-only stuff out of public interface

x509_certificate_model: remove unused code, move nss-only stuff out of public interface

Fix x509_certificate_model::RegisterDynamicOids not getting called since gtk certificate viewer was removed.

BUG=338887, 371171
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270775

[mattm, 2014-05-07 23:04:18]

Stuff that was used by the gtk cert viewer & selector, but isn't used by the
webui / aura versions, plus a few things that don't need to be part of the
public header.

[Ryan Sleevi, 2014-05-08 00:34:38]

https://codereview.chromium.org/271753004/diff/1/chrome/common/net/x509_certi...
File chrome/common/net/x509_certificate_model_nss.cc (right):

https://codereview.chromium.org/271753004/diff/1/chrome/common/net/x509_certi...
chrome/common/net/x509_certificate_model_nss.cc:210: psm::RegisterDynamicOids();
This needs to be called before getting the subject/issuer name as well, since
there are EV subject attributes.

We should also (ideally) only call this once, as a lazy instance - avoids some
additional table searches within NSS.

[mattm, 2014-05-13 00:57:40]

updated

https://codereview.chromium.org/271753004/diff/1/chrome/common/net/x509_certi...
File chrome/common/net/x509_certificate_model_nss.cc (right):

https://codereview.chromium.org/271753004/diff/1/chrome/common/net/x509_certi...
chrome/common/net/x509_certificate_model_nss.cc:210: psm::RegisterDynamicOids();
On 2014/05/08 00:34:38, Ryan Sleevi wrote:
> This needs to be called before getting the subject/issuer name as well, since
> there are EV subject attributes.

Hm, seems easiest to just move it to GetOIDText, wdyt?

> We should also (ideally) only call this once, as a lazy instance - avoids some
> additional table searches within NSS.

The RegisterDynamicOids basically does that already, though not in a thread safe
way. I guess since the functions aren't otherwise thread unsafe I'll use the
lazy instance.

[Ryan Sleevi, 2014-05-13 01:12:12]

Still missing two places.

Otherwise, LGTM

https://codereview.chromium.org/271753004/diff/40001/chrome/third_party/mozil...
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

https://codereview.chromium.org/271753004/diff/40001/chrome/third_party/mozil...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:479: if (oid_tag
== ms_nt_principal_name) {
Need to call the lazy instance before here

https://codereview.chromium.org/271753004/diff/40001/chrome/third_party/mozil...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:1031: if
(oid_tag == ms_cert_ext_certtype)
Need to call the lazy instance before here

[mattm, 2014-05-13 01:52:32]

updated

https://codereview.chromium.org/271753004/diff/40001/chrome/third_party/mozil...
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

https://codereview.chromium.org/271753004/diff/40001/chrome/third_party/mozil...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:479: if (oid_tag
== ms_nt_principal_name) {
On 2014/05/13 01:12:12, Ryan Sleevi wrote:
> Need to call the lazy instance before here

The GetOIDText should do it already, so I'll just add a comment to note that.

https://codereview.chromium.org/271753004/diff/40001/chrome/third_party/mozil...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:1031: if
(oid_tag == ms_cert_ext_certtype)
On 2014/05/13 01:12:12, Ryan Sleevi wrote:
> Need to call the lazy instance before here

Ah, right. I guess it actually needs to be done before the SECOID_FindOIDTag
call, so I changed the ProcessExtensionData signature to allow that.

[mattm, 2014-05-14 22:28:03]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2014-05-14 22:29:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mattm@chromium.org/271753004/80001

[mattm, 2014-05-15 02:15:08]

+jochen for OWNERS

[commit-bot: I haz the power, 2014-05-15 02:23:43]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-15 02:27:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-15 02:27:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[jochen (gone - plz use gerrit), 2014-05-15 10:10:51]

https://codereview.chromium.org/271753004/diff/80001/chrome/third_party/mozil...
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

https://codereview.chromium.org/271753004/diff/80001/chrome/third_party/mozil...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:166: static
base::LazyInstance<DynamicOidRegisterer>::Leaky
i'm not sure I understand the reason for this change? we'll end up allocating
memory while the old code did not?

[Ryan Sleevi, 2014-05-15 13:12:59]

On 2014/05/15 10:10:51, jochen wrote:
>
https://codereview.chromium.org/271753004/diff/80001/chrome/third_party/mozil...
> File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):
> 
>
https://codereview.chromium.org/271753004/diff/80001/chrome/third_party/mozil...
> chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:166: static
> base::LazyInstance<DynamicOidRegisterer>::Leaky
> i'm not sure I understand the reason for this change? we'll end up allocating
> memory while the old code did not?

Same memory. Just moves from an explicit allocation (Init) to an implicit one
(LazyInstance).

[jochen (gone - plz use gerrit), 2014-05-15 14:36:52]

On 2014/05/15 13:12:59, Ryan Sleevi wrote:
> On 2014/05/15 10:10:51, jochen wrote:
> >
>
https://codereview.chromium.org/271753004/diff/80001/chrome/third_party/mozil...
> > File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/271753004/diff/80001/chrome/third_party/mozil...
> > chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:166: static
> > base::LazyInstance<DynamicOidRegisterer>::Leaky
> > i'm not sure I understand the reason for this change? we'll end up
allocating
> > memory while the old code did not?
> 
> Same memory. Just moves from an explicit allocation (Init) to an implicit one
> (LazyInstance).

LazyInstance will allocate the class, before there was no allocation

[Ryan Sleevi, 2014-05-15 14:54:15]

On 2014/05/15 14:36:52, jochen wrote:
> On 2014/05/15 13:12:59, Ryan Sleevi wrote:
> > On 2014/05/15 10:10:51, jochen wrote:
> > >
> >
>
https://codereview.chromium.org/271753004/diff/80001/chrome/third_party/mozil...
> > > File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/271753004/diff/80001/chrome/third_party/mozil...
> > > chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:166:
static
> > > base::LazyInstance<DynamicOidRegisterer>::Leaky
> > > i'm not sure I understand the reason for this change? we'll end up
> allocating
> > > memory while the old code did not?
> > 
> > Same memory. Just moves from an explicit allocation (Init) to an implicit
one
> > (LazyInstance).
> 
> LazyInstance will allocate the class, before there was no allocation

And?

We don't want to call into NSS and repeatedly do a full OID table scan to see if
something is registered., when we know it is.

We use LazyInstances all the time for this. Is there a new concern here? I'm not
sure if you're just asking a question.

[jochen (gone - plz use gerrit), 2014-05-15 15:27:08]

after some offline discussion lgtm

[mattm, 2014-05-15 19:27:05]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2014-05-15 19:27:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mattm@chromium.org/271753004/80001

[commit-bot: I haz the power, 2014-05-15 19:34:06]

Change committed as 270775