1485087 - remove the option to use the session cache from nsITLSServerSocket

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P1)

Description

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

nsITLSServerSocket by default enables a session cache, but all code that uses it (except one test) disables this feature. Because it's an option, though, we have to jump through some hoops in PSM to a) make it work in the first place and b) have NSS not panic on shutdown (also c it means more memory use that is unnecessary for literally everyone). It turns out that if shutdown happens in the wrong order (PSM first, and then Necko), NSS can attempt to acquire a lock that's been deleted, which should crash but in general seems to hang. We should probably make it a bit less easy to shoot yourself in the foot with this in NSS, but that would take some time, and currently this is blocking bug 1479918, in which we discovered that the HSTS preload list script has been broken for months (the fix for which needs to be uplifted).

So I think we should remove this feature that no one is using and is only causing us problems.

Comment 1

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

Attachment: bug 1485087 - remove the option to use the TLS session cache from nsITLSServerSocket r?mayhemer,jryans

As initially implemented, nsITLSServerSocket by default enabled the use of the
TLS session cache provided by NSS. However, no consumers of nsITLSServerSocket
actually used it. Because it was an option, though, PSM had to jump through some
hoops to a) make it work in the first place and b) not have NSS panic on
shutdown. Furthermore, it meant increased memory usage for every user of Firefox
(and again, nothing actually used the feature, so this was for naught).

In bug 1479918, we discovered that if PSM shut down before Necko, NSS could
attempt to acquire a lock on the session cache that had been deleted, causing a
shutdown hang. We probably should make it less easy to make this mistake in NSS,
but in the meantime bug 1479918 needs uplifting and this workaround is the
safest, most straight-forward way to achieve this.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 9002909 [details]
bug 1485087 - remove the option to use the TLS session cache from nsITLSServerSocket r?mayhemer,jryans

J. Ryan Stinnett [:jryans] has approved the revision.

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to [:keeler] (use needinfo) from comment #0)
> nsITLSServerSocket by default enables a session cache, but all code that
> uses it (except one test) disables this feature. Because it's an option,
> though, we have to jump through some hoops in PSM to a) make it work in the
> first place and b) have NSS not panic on shutdown (also c it means more
> memory use that is unnecessary for literally everyone). It turns out that if
> shutdown happens in the wrong order (PSM first, and then Necko), NSS can
> attempt to acquire a lock that's been deleted, 

Do you have stacks?  I would be interested in what lock in necko is psm trying to access.  Thanks.

> which should crash but in
> general seems to hang. We should probably make it a bit less easy to shoot
> yourself in the foot with this in NSS, but that would take some time, and
> currently this is blocking bug 1479918, in which we discovered that the HSTS
> preload list script has been broken for months (the fix for which needs to
> be uplifted).
> 
> So I think we should remove this feature that no one is using and is only
> causing us problems.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 9002909 [details]
bug 1485087 - remove the option to use the TLS session cache from nsITLSServerSocket r?mayhemer,jryans

Honza Bambas (:mayhemer) has approved the revision.

Comment 5

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

Thanks for the reviews!

The issue isn't necessarily with PSM or Necko - it's with NSS and its session id cache locks. These are usually initialized lazily when you're running as a client (and will be properly cleaned up when NSS_Shutdown is called). However, since TLSServerSocket runs NSS as a server, if it's configured to use the session cache, it does so in a way that requires explicit initialization/cleanup rather than just relying on the implicit mechanism, which is why we added the calls to SSL_ConfigServerSessionIDCache and SSL_ShutdownServerSessionIDCache in PSM. When we moved the NSS_Shutdown call from PSM to XPCOM shutdown, it meant that in theory necko could still be running after PSM had been shut down (i.e. SSL_ShutdownServerSessionIDCache had been called), leading to trying to use those structures after they had been cleaned up. In theory we could move SSL_ShutdownServerSessionIDCache to XPCOM shutdown as well, but that can only be called if we know SSL_ConfigServerSessionIDCache has been called, and right now only PSM knows that. There's a lot that could be improved here, but since this feature isn't used, it doesn't seem worth it to me.

Comment 6

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

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

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to [:keeler] (use needinfo) from comment #5)
> Thanks for the reviews!

Thanks, agree that this is not necessary to keep around.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/214a7d48d11a
remove the option to use the TLS session cache from nsITLSServerSocket r=jryans,mayhemer

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/214a7d48d11a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This is too late for 62 (merged to release today for Monday's RC build), but we could still take it on ESR60 whenever.

Comment 11

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

Attachment: patch for esr

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: necessary to fix HSTS preload list automated script
User impact if declined: no new sites will be added to the HSTS preload list for ESR60
Fix Landed on Version: 63
Risk to taking this patch (and alternatives if risky): low - we're removing a feature that wasn't used
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 12

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

Er, to clarify, this bug blocks the actual fixing of the HSTS preload list updater in bug 1479918. This is a prerequisite, though, because if we don't do this the script will just hang.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9004275 [details] [diff] [review]
patch for esr

Needed to un-break pinning updates. Approved for ESR 60.2.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/247e07710c04

Comment 15

[Ionuț Goldan [:igoldan]]

Memory improvements noticed:

== Change summary for alert #15333 (as of Fri, 24 Aug 2018 16:26:19 GMT) ==

Improvements:

  7%  Heap Unclassified windows7-32 opt stylo     37,951,838.16 -> 35,412,446.71
  6%  Heap Unclassified windows7-32 pgo stylo     37,966,701.23 -> 35,652,818.52
  4%  Heap Unclassified windows10-64 opt stylo    64,278,387.94 -> 61,915,242.64
  3%  Heap Unclassified windows10-64 pgo stylo    64,144,824.42 -> 61,926,130.47
  3%  Heap Unclassified linux64 opt stylo         86,957,468.25 -> 84,089,756.78
  3%  Heap Unclassified linux64-stylo-sequential opt stylo-sequential86,262,376.94 -> 83,446,784.03

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=15333