1435141 - Main thread IO in the parent process in nsNSSComponent::HasUserCertsInstalled

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Markus Stange [:mstange]]

In this profile, the check caused 64ms of main thread jank: https://perfht.ml/2s6qPdp

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Are there any downsides to calling nsNSSComponent::HasUserCertsInstalled(bool& result) and nsNSSComponent::HasActiveSmartCards(bool& result) off-main-thread?

I would like to use a MozPromise to check for certs off-main-thread, and CanEnableSpeculativeConnect would return false if the promise didn't complete, or actual value if it did.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

As of bug 1421084, I believe we can relax some of the main-thread-only requirements of nsNSSComponent, so yes, I think this would be doable.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1435141 - Check for user certificates on a background thread to avoid main thread IO

Review commit: https://reviewboard.mozilla.org/r/220038/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220038/

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8950797 [details]
Bug 1435141 - Check for user certificates on a background thread to avoid main thread IO

https://reviewboard.mozilla.org/r/220038/#review225862

The nsNSSComponent.cpp changes look fine with some minor comments. I'm not a necko peer, so you should get one to review the netwerk/ changes.

::: security/manager/ssl/nsNSSComponent.cpp
(Diff revision 1)
>  }
>  
>  nsresult
>  nsNSSComponent::HasActiveSmartCards(bool& result)
>  {
> -  MOZ_ASSERT(NS_IsMainThread(), "Main thread only");

We should also remove the comment at https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/security/manager/ssl/nsNSSComponent.h#83 (and again at line 135) since these are no longer main-thread-only.

::: security/manager/ssl/nsNSSComponent.cpp:1048
(Diff revision 1)
> -    return NS_ERROR_NOT_SAME_THREAD;
> -  }
> -
>  #ifndef MOZ_NO_SMART_CARDS
>    MutexAutoLock nsNSSComponentLock(mMutex);
>  

I guess technically we should add a mInitialized check here.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8950797 [details]
Bug 1435141 - Check for user certificates on a background thread to avoid main thread IO

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220038/diff/1-2/

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8950797 [details]
Bug 1435141 - Check for user certificates on a background thread to avoid main thread IO

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220038/diff/2-3/

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8950797 [details]
Bug 1435141 - Check for user certificates on a background thread to avoid main thread IO

https://reviewboard.mozilla.org/r/220038/#review225870

::: security/manager/ssl/nsNSSComponent.h:134
(Diff revision 2)
>  #endif
>  
>    NS_IMETHOD BlockUntilLoadableRootsLoaded() override;
>    NS_IMETHOD CheckForSmartCardChanges() override;
>  
>    // Main thread only

This one too please :)

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8950797 [details]
Bug 1435141 - Check for user certificates on a background thread to avoid main thread IO

https://reviewboard.mozilla.org/r/220038/#review225870

> This one too please :)

Heh - beat me to it.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8950797 [details]
Bug 1435141 - Check for user certificates on a background thread to avoid main thread IO

https://reviewboard.mozilla.org/r/220038/#review226180

::: netwerk/protocol/http/nsHttpHandler.cpp:439
(Diff revision 3)
>      }
>      mIOService = new nsMainThreadPtrHolder<nsIIOService>(
>        "nsHttpHandler::mIOService", service);
>  
> +    rv = NS_NewNamedThread("nsHttpHandler",
> +                           getter_AddRefs(mBackgroundThread));

could you use a lazy thread?  this will just waste resources forever having just a single startup function

a bit better name would be nice too, how about "HTTP Handler Background" ?

::: netwerk/protocol/http/nsHttpHandler.cpp:443
(Diff revision 3)
> +    rv = NS_NewNamedThread("nsHttpHandler",
> +                           getter_AddRefs(mBackgroundThread));
> +    if (NS_FAILED(rv)) {
> +        return rv;
> +    }
> +

hmm... little bit hard to fail here.  you do a null check later on it.

::: netwerk/protocol/http/nsHttpHandler.cpp:594
(Diff revision 3)
>      nsCOMPtr<nsIParentalControlsService> pc = do_CreateInstance("@mozilla.org/parental-controls-service;1");
>      if (pc) {
>          pc->GetParentalControlsEnabled(&mParentalControlEnabled);
>      }
> +
> +    MaybeEnableSpeculativeConnect();

could you do this a bit sooner?

::: netwerk/protocol/http/nsHttpHandler.cpp:2479
(Diff revision 3)
> -  // Check if any 3rd party PKCS#11 module are installed, as they may produce
> +    // Check if any 3rd party PKCS#11 module are installed, as they may produce
> -  // client certificates
> +    // client certificates
> -  bool activeSmartCards = false;
> +    bool activeSmartCards = false;
> -  nsresult rv = component->HasActiveSmartCards(activeSmartCards);
> +    nsresult rv = component->HasActiveSmartCards(activeSmartCards);
> -  if (NS_FAILED(rv) || activeSmartCards) {
> +    if (NS_FAILED(rv) || activeSmartCards) {
> -    return false;
> +        return GenericPromise::CreateAndResolve(false, __func__);

btw, if we fail to find out (NS_FAILED(rv)) do we really want to disable?

::: netwerk/protocol/http/nsHttpHandler.cpp:2515
(Diff revision 3)
> +
> +    // Component must be initialized on the main thread.
> +    nsCOMPtr<nsINSSComponent> component(do_GetService(PSM_COMPONENT_CONTRACTID));
> +    if (!component) {
> +        NS_WARNING("nsHttpHandler::MaybeEnableSpeculativeConnect() no component");
> +        return;

enable specul conn here then.

::: netwerk/protocol/http/nsHttpHandler.cpp:2526
(Diff revision 3)
> +                    __func__,
> +                    [component]() { return CanEnableSpeculativeConnect(component); });
> +
> +    // Update mSpeculativeConnect on the main thread.
> +    mSpeculativePromise->Then(GetMainThreadSerialEventTarget(), __func__,
> +                              [self](bool aVal) { self->mSpeculativeConnectEnabled = false; },

= aVal;

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8950797 [details]
Bug 1435141 - Check for user certificates on a background thread to avoid main thread IO

https://reviewboard.mozilla.org/r/220038/#review226180

> could you use a lazy thread?  this will just waste resources forever having just a single startup function
> 
> a bit better name would be nice too, how about "HTTP Handler Background" ?

Didn't know about LazyIdleThread. Really cool. Thanks for the heads up.

The reason I didn't give the thread a longer name is that NS_NewNamedThread accepts a max of 16 characters. LazyIdleThread doesn't have this limitation.

> btw, if we fail to find out (NS_FAILED(rv)) do we really want to disable?

HasActiveSmartCards doesn't fail unless the component isn't initialized. Unlikely to be an issue.

> enable specul conn here then.

Fair enough.

> = aVal;

Ooops!

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8950797 [details]
Bug 1435141 - Check for user certificates on a background thread to avoid main thread IO

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220038/diff/3-4/

Comment 12

[Valentin Gosu [:valentin] (he/him)]

The try run showed some failures. It seems some tests hang in 
nsCOMPtr<nsINSSComponent> component(do_GetService(PSM_COMPONENT_CONTRACTID));
with the terminal showing the following message:
PID 28092 | [28092, Main Thread] WARNING: NSS will be initialized without a profile directory. Some things may not work as expected.: file /home/icecold/mozilla-central/security/manager/ssl/nsNSSComponent.cpp, line 1744

I'll try to touch up the patch to work around the issue.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Valentin Gosu [:valentin] from comment #12)
> The try run showed some failures. It seems some tests hang in 
> nsCOMPtr<nsINSSComponent> component(do_GetService(PSM_COMPONENT_CONTRACTID));
> with the terminal showing the following message:
> PID 28092 | [28092, Main Thread] WARNING: NSS will be initialized without a
> profile directory. Some things may not work as expected.: file
> /home/icecold/mozilla-central/security/manager/ssl/nsNSSComponent.cpp, line
> 1744

Usually this message means the test is running without a profile directory, or it doesn't have read/write access to the profile directory.

Comment 14

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bff35fa7f88ac9e6dbc3e2280e92480951381070

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8950797 [details]
Bug 1435141 - Check for user certificates on a background thread to avoid main thread IO

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220038/diff/4-5/

Comment 16

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ac0db943c4c272a95201c0802c9b5c29c9c9caf

Comment 17

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0133e0f4d8abf2c5289cad454b8dbb96164dc31e

Comment 18

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eae671f24a20e789bc98a301c54a22c266a285e1

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Here's another profile where this consumes about 300ms on start-up: https://perfht.ml/2VLGcTa

Comment 21

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c9ab7f59647ffdee9c6ef5ce9f297f8c66fec84

Comment 22

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=362d7b17a3ca4f563900ce71044d0b686cf565f8

Comment 23

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=204a7b073ff84b30cca8768fcc0a46f9c8d0d7ac

Comment 24

[Florian Quèze [:florian]]

Here is a profile of nsNSSComponent::HasUserCertsInstalled with main thread I/O markers: https://perfht.ml/2CX20Tc

I'm surprised we need to do that many read and stat calls. The stat calls are almost all on cert9.db-wal and cert9.db-journal. Would it be possible to have a copy of this file in memory to access it quickly without doing so much disk I/O?

Comment 25

[Dana Keeler (she/her) (use needinfo) [:keeler]]

See also bug 1478148. While it would be beneficial to improve that implementation, there are significant architectural constraints that make it difficult.

Comment 26

[Valentin Gosu [:valentin] (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #23)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=204a7b073ff84b30cca8768fcc0a46f9c8d0d7ac

In my latest try patch it seems we no longer hang without a profile like before, but sometimes we get:
MOZ_CRASH(NSS_Shutdown failed)
I'm not really sure why that would happen. Could just holding the nssComponent mutex cause NSS shutdown to fail?

Comment 27

[Dana Keeler (she/her) (use needinfo) [:keeler]]

My guess is it has something to do with that CanEnableSpeculativeConnect doesn't have an owning reference to the nsINSSComponent - there's nothing keeping that alive, from what I can tell, although the documentation says the thread will be joined at xpcom shutdown, which should happen strictly before trying to call NSS_Shutdown, so I don't really see how that would prevent NSS from shutting down successfully (I would just expect some uaf crash). It could be that that code isn't threadsafe in NSS and we're merely exercising an existing bug. I've tried to narrow it down to either checking for smartcards or checking for client certs, but only checking for one or the other doesn't seem to result in failures (although maybe I just haven't tried enough retriggers). I'll keep looking, but I don't see anything in this patch that would cause a new NSS shutdown failure (the most common cause of these is not releasing some NSS resource).

Comment 28

[Valentin Gosu [:valentin] (he/him)]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #27)

My guess is it has something to do with that CanEnableSpeculativeConnect doesn't have an owning reference to the nsINSSComponent - there's nothing keeping that alive, from what I can tell, although the documentation says the thread will be joined at xpcom shutdown, which should happen strictly before trying to call NSS_Shutdown, so I don't really see how that would prevent NSS from shutting down successfully (I would just expect some uaf crash). It could be that that code isn't threadsafe in NSS and we're merely exercising an existing bug. I've tried to narrow it down to either checking for smartcards or checking for client certs, but only checking for one or the other doesn't seem to result in failures (although maybe I just haven't tried enough retriggers). I'll keep looking, but I don't see anything in this patch that would cause a new NSS shutdown failure (the most common cause of these is not releasing some NSS resource).

The lambda that we pass to the thread holds a ref to the nsINSSComponent - it may not even run by the time we shutdown. That could be the cause.

Comment 29

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Looks like I just needed to run more tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91bdbc705eddb6fd6270b8f9004764049bc6895a indicates there may be a leak in CERT_FindUserCertsByUsage (maybe caused by a race?). Another thing I noticed is that NSS_IsInitialized() isn't protected by any kind of synchronization - we essentially race on the variable that backs it, so if nsINSSComponent is being held alive after xpcom shutdown somehow, then that check is essentially useless (but again I don't see how this could be happening because we've supposedly joined all threads at that point).

Stepping back, this mechanism is a bit fragile, as we've seen. Would it be possible to re-work things slightly so that when we're speculatively connecting, if the server asks for a client certificate, we just close the connection? (NSS doesn't directly expose a way to do this, I think, so we would have to essentially make a note that this happened in nsNSSSocketInfo that necko checks and then manually closes the connection.)

Comment 30

[Valentin Gosu [:valentin] (he/him)]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #29)

Looks like I just needed to run more tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91bdbc705eddb6fd6270b8f9004764049bc6895a indicates there may be a leak in CERT_FindUserCertsByUsage (maybe caused by a race?). Another thing I noticed is that NSS_IsInitialized() isn't protected by any kind of synchronization - we essentially race on the variable that backs it, so if nsINSSComponent is being held alive after xpcom shutdown somehow, then that check is essentially useless (but again I don't see how this could be happening because we've supposedly joined all threads at that point).

Is CERT_FindUserCertsByUsage supposed to be thread-safe? Because this happens in all of my try runs, so it might be very racy.

Stepping back, this mechanism is a bit fragile, as we've seen. Would it be possible to re-work things slightly so that when we're speculatively connecting, if the server asks for a client certificate, we just close the connection? (NSS doesn't directly expose a way to do this, I think, so we would have to essentially make a note that this happened in nsNSSSocketInfo that necko checks and then manually closes the connection.)

Would that actually work? Looking at bug 910207, it seems like something like this was suggested before, but it didn't work.
I think the problem was that by the time we actually connect, that speculative connection might already be used by a HTTP request.

Comment 31

[Dana Keeler (she/her) (use needinfo) [:keeler]]

It looks like what was missing from that original implementation was the part where necko says "ok, the server wanted a client certificate so we killed the speculative connection, and now that the user has actually gone to this site, we need to connect again and allow the client certificate dialog".

Comment 32

[Valentin Gosu [:valentin] (he/him)]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #31)

It looks like what was missing from that original implementation was the part where necko says "ok, the server wanted a client certificate so we killed the speculative connection, and now that the user has actually gone to this site, we need to connect again and allow the client certificate dialog".

I think if we had a way to pass a callback into NSS that can be used to choose if to show the cert chooser or fail the connection that would be great. I suspect that's not as easy as it sounds :)

Comment 33

[Valentin Gosu [:valentin] (he/him)]

Dana, do you think the patch could still be used/useful?
What could I do to work around the issues in comment 29?

Comment 34

[Dana Keeler (she/her) (use needinfo) [:keeler]]

It looks like that patch has bitrotted some, but I imagine it's salvageable. In particular, nsNSSComponent::mNSSInitialized doesn't exist any more, so you shouldn't need to acquire nsNSSComponent::mMutex at all. Also, the leaks may be fixed by first calling nsNSSComponent::BlockUntilLoadableRootsLoaded(); in each of HasUserCertsInstalled and HasActiveSmartCards (just to be safe).

Comment 35

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1435141 - Check for user certificates on a background thread to avoid main thread IO r=keeler!

Comment 36

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=818bc1542c8634316a7c3a84fb7765b774c3605d&selectedJob=232409679

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06e9cd204aac
Check for user certificates on a background thread to avoid main thread IO r=keeler

Comment 38

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/06e9cd204aac