Adding estark@, as one of the keepers of SSL things in Chromium.

Ok so tried the simple approach of removing the SSL certificate of old navigation entries after each commit: https://codereview.chromium.org/2600233003/#ps1

This doesn't work, as the two failures demonstrate:
SSLUITest.TestGoodFrameNavigation
SSLUITest.PushStateSSLState

When a renderer initiates a back navigation (and probably browser as well), we create NavigationHandle in RenderFrameHostImpl::OnDidCommitProvisionalLoad. NavigationHandleImpl::WillProcessResponse is never called on it to give it the SSLStatus information, so we do need to keep the certs around.



However I did some manual tests to see how the net::X509Certificate object is shared (since it's ref counted). It comes from the net/ layer, and while I didn't trace the code there, starting new tabs or going back and forth to the same origin gives the same certificate object for all of them. One of my original concerns was I was worried that each of these would have its own certificate.

It's hard to compare the memory usage before and after, especially without knowing details of how long net/ keeps SSL certs around. Before my changes, content/ would keep a container of all SSL certificates seen in a renderer process during its lifetime. In a "normal" case of a user with one tab and surfing across origins which creates new processes, we shouldn't have many entries in each renderer's container. With my changes, we now store a reference to the certificate in each NavigationEntry. In a degenerate scenario, a user could have many tabs open and each of them could have the max navigation controller limit of 50 history entries, each pointing to a different SSL certificate.

+Ryan & David for their thoughts

While there's always a degenerate scenario, in the common case, it's unlikely to result in any additional load.

All representations of the same X509Certificate share the same underlying backing memory in //net for as long as a handle to the cert exists (i.e. it pools memory).

The cert verification cache will keep successful verifications cached for 30 minutes with an MRU size of 256.

The socket pool will keep both the server-sent and client-verified cert chains around for as long as the socket is around. For an idle socket, this is 10 minutes, but for an active socket (or an H/2 or QUIC socket), this could be for as long as the socket is active.

So under normal navigation, this is unlikely to result in additional memory pressure, and probably not a high priority item in that it's unlikely to show any yields except under the very degenerate case that I understand (from UMA anecdata about various design decisions re: tabs) is extremely uncommon and unlikely.

I'm hoping other work will alleviate the memory concerns here, if that's the main motivation.

There's also work underway to even more aggressively deduplicate them (expect two tabs from the same origin to always share the objects in-memory rather than usually) and fix the really awful in-memory representation (expect something like 4x improvement on Android! OpenSSL's code is really *that* bad).

In practice, I expect the copy of the certificate the session cache far outlives the navigation entry copy anyway. I believe those two aren't currently deduplicated, but the work above will do that too.


They need to get retained for DevTools (or Page Info bubble on Android). If anything, we'll probably need to lean on the SSL state in there more rather than less to fix the problem where the Security Panel needs a reload to show much, which currently blocks our ability to report problems to developers. (Although, for my needs, I don't care about the certificate, just the various SSL parameters which are tiny.)

Oh yeah, to be clear in Comment #5 - David's referring to the TLS session cache ( https://cs.chromium.org/chromium/src/net/ssl/ssl_client_session_cache.h?rcl=1482855583&l=34 ) which retains the necessary metadata about the connection, such as the server-supplied certificate, in order to allow TLS session resumption.

The session cache there is set at 1024 entries

Thanks Ryan & David for the info.

Since
-it looks like we don't expect the change in behavior to cause extra memory usage
-normal browsing behavior reuses the certificates references
-we do need to keep the certs around to maintain functionality

It seems like there's no need to change the content/browser layer now.