load loadable roots on a background thread

RESOLVED FIXED in Firefox 57

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
2 months ago
9 days ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

The profile link in bug 1370834 shows that loading the loadable roots module (i.e. the built-in root CA list) takes ~60% of the time to initialize PSM. Since this currently happens on the main thread and since we don't actually need those roots until we start building a certificate chain, we can probably get a big startup time win by doing this work on a background thread.
(Assignee)

Updated

2 months ago
Depends on: 1375709
(Assignee)

Updated

a month ago
Blocks: 1370516
(Assignee)

Updated

a month ago
Depends on: 1381148
Comment hidden (mozreview-request)

Comment 2

17 days ago
mozreview-review
Comment on attachment 8892672 [details]
bug 1372656 - load loadable roots on a background thread

https://reviewboard.mozilla.org/r/152348/#review169080

Top level: `EnsureLoadableRootsLoaded()` doesn't sound to me like a method that would block execution, but it does. It might be helpful to put a comment in each place where it's called, maybe.

The thread sync code looks good, but I want to re-look at it before this lands, so hitting r- for the moment. Otherwise, just two issues, and my ask above re: comment.

::: security/manager/ssl/PKCS11.cpp:48
(Diff revision 1)
> +  // This is a workaround to attempt to avoid a deadlock in NSS. The issue is
> +  // not so much the loadable roots as the process of setting up the EV
> +  // information, which happens directly after loading the roots (that process
> +  // involves calling CERT_FindCertByIssuerAndSN on another thread, which can
> +  // deadlock with the call to SECMOD_DeleteModule here). See bug 1381148.

Bug 1381148 indicates that it's fixed... do we need this as a workaround then? Should we adjust the comment?

::: security/manager/ssl/nsNSSComponent.h:101
(Diff revision 1)
>  // Implementation of the PSM component interface.
>  class nsNSSComponent final : public nsINSSComponent
>                             , public nsIObserver
>  {
>  public:
> +  friend class LoadLoadableRootsTask;

Interesting... I had it in my head that we normally annotated why friends exist, but searching around the codebase doesn't find me many examples.

Also, googling "Why do friends exist?" is super extistential and makes me feel like I should go listen to Nightwish.

::: security/manager/ssl/nsNSSComponent.cpp:1216
(Diff revision 1)
> +
> +nsresult
> +LoadLoadableRootsTask::LoadLoadableRoots(
> +  const nsNSSShutDownPreventionLock& /*proofOfLock*/)
> +{
> +  // TODO: check isAlreadyShutDown()

leftover todo?

::: security/manager/ssl/tests/unit/test_pkcs11_module.js
(Diff revision 1)
>  }
>  
>  function checkModuleTelemetry(additionalExpectedModule = undefined) {
>    let expectedModules = [
>      "NSS Internal PKCS #11 Module",
> -    `${AppConstants.DLL_PREFIX}nssckbi${AppConstants.DLL_SUFFIX}`,

Why does this need to get removed? Is the module not loaded by the time the test checks?
Attachment #8892672 - Flags: review?(jjones) → review-
(Assignee)

Comment 3

16 days ago
mozreview-review-reply
Comment on attachment 8892672 [details]
bug 1372656 - load loadable roots on a background thread

https://reviewboard.mozilla.org/r/152348/#review169080

I could also name it something like `BlockUntilLoadableRootsLoaded()`?
For context, the thread sync code is largely based on the DataStorage implementation.

> Bug 1381148 indicates that it's fixed... do we need this as a workaround then? Should we adjust the comment?

Well, the change in NSS hasn't landed in mozilla-central yet, so we'll need it at least until then. I suspect there may be other deadlocks of a similar variety. I'll update this as necessary.

> Interesting... I had it in my head that we normally annotated why friends exist, but searching around the codebase doesn't find me many examples.
> 
> Also, googling "Why do friends exist?" is super extistential and makes me feel like I should go listen to Nightwish.

I can add a comment. I think friends exist (or at least are important) because they enrich our lives.

> leftover todo?

Ah - thanks.

> Why does this need to get removed? Is the module not loaded by the time the test checks?

It's because a) we gather the initial telemetry before the loadable roots (nssckbi) have been loaded and b) loading that module uses specialized code that doesn't collect the telemetry. Since that telemetry is more focused on 3rd party PKCS#11 modules, I didn't think it was important to keep it behaving exactly the same as before.
Comment hidden (mozreview-request)

Comment 5

16 days ago
mozreview-review
Comment on attachment 8892672 [details]
bug 1372656 - load loadable roots on a background thread

https://reviewboard.mozilla.org/r/152348/#review169420

Thanks, this looks good to me.
Attachment #8892672 - Flags: review?(jjones) → review+
> > Bug 1381148 indicates that it's fixed... do we need this as a workaround then? Should we adjust the comment?

> Well, the change in NSS hasn't landed in mozilla-central yet, so we'll need it at least until then. I suspect there may be other deadlocks of a similar variety. I'll update this as necessary.

I'm going to land the first 3.33 snapshot today, which hopefully fixes the issue. (We're finally on 57, yay.)

Comment 7

14 days ago
mozreview-review
Comment on attachment 8892672 [details]
bug 1372656 - load loadable roots on a background thread

https://reviewboard.mozilla.org/r/152348/#review170402

Cool.

::: security/certverifier/NSSCertDBTrustDomain.cpp
(Diff revision 2)
>  }
>  
>  bool
>  LoadLoadableRoots(const nsCString& dir, const nsCString& modNameUTF8)
>  {
> -  UniquePRLibraryName fullLibraryPath(

Looks like this was the only use of `UniquePRLibraryName`, so we should remove the corresponding ScopedNSSTypes.h entry as well.

Also, thumbs up for getting rid of another use of NSPR.

::: security/manager/ssl/ScopedNSSTypes.h:328
(Diff revision 2)
> +MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePRString,
> +                                      char,
> +                                      PR_Free)

Nit: The types in each section are generally alphabetically sorted, so this should probably go last.

::: security/manager/ssl/nsNSSComponent.cpp:1164
(Diff revision 2)
> +static nsresult
> +GetNSS3Directory(nsCString& result)
> +{
> +  UniquePRString nss3Path(
> +    PR_GetLibraryFilePathname(DLL_PREFIX "nss3" DLL_SUFFIX,
> +                              (PRFuncPtr)NSS_Initialize));

Nit: Let's make this a C++ cast while we're here.
Attachment #8892672 - Flags: review?(cykesiopka.bmo) → review+
(Assignee)

Comment 8

14 days ago
mozreview-review-reply
Comment on attachment 8892672 [details]
bug 1372656 - load loadable roots on a background thread

https://reviewboard.mozilla.org/r/152348/#review170402

Thanks!

> Looks like this was the only use of `UniquePRLibraryName`, so we should remove the corresponding ScopedNSSTypes.h entry as well.
> 
> Also, thumbs up for getting rid of another use of NSPR.

Ah - good call.

> Nit: The types in each section are generally alphabetically sorted, so this should probably go last.

Fixed.

> Nit: Let's make this a C++ cast while we're here.

Sounds good.
Comment hidden (mozreview-request)
Try looked good without the workaround, so I'm going ahead with this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae49c7bf1b8c
Thanks for the reviews!

Comment 11

10 days ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad20fd5faada
load loadable roots on a background thread r=Cykesiopka,jcj

Comment 12

10 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ad20fd5faada
Status: NEW → RESOLVED
Last Resolved: 10 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.