Closed Bug 1372656 Opened 7 years ago Closed 7 years ago

load loadable roots on a background thread

Categories

(Core :: Security: PSM, enhancement, P1)

Product:
Core
Component:
Security: PSM
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

bug 1372656 - load loadable roots on a background thread
59 bytes, text/x-review-board-request
Cykesiopka
: review+
jcj
: review+
Details 
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.
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-
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 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 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+
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.
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!
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad20fd5faada
load loadable roots on a background thread r=Cykesiopka,jcj
https://hg.mozilla.org/mozilla-central/rev/ad20fd5faada
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1439383
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: